-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(Video): context menu was not working on Firefox #428
Conversation
There must be a better way to solve this because that will break UX on mobile safari. |
const popupLabelOnContextMenu = React.useCallback((event) => {
if (!event.nativeEvent.togglePopupPrevented && !event.nativeEvent.ctrlKey) {
event.preventDefault();
if (typeof event.nativeEvent.pointerType !== 'string' || event.nativeEvent.pointerType === 'mouse') {
toggleMenu();
}
}
}, [toggleMenu]);
const popupLabelOnLongPress = React.useCallback((event) => {
if (!event.nativeEvent.togglePopupPrevented && typeof event.nativeEvent.pointerType === 'string' && event.nativeEvent.pointerType !== 'mouse') {
toggleMenu();
}
}, [toggleMenu]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error was introduced in this PR: #375 |
The context menu was not working on Firefox because thecontextmenu
event is not a PointerEvent on Firefox and we were using the pointerEvent property to check if it was from a mousehttps://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event#browser_compatibility
Thecontextmenu
event is managed by the browser, on desktop it should trigger on right click, on mobile on long pressSo we don't really need to use the
onLongPress
callbackWe ended up using
mouseup
instead as it's a MouseEvent and check if right click was pressedonMouseUp
for desktop andonLongPress
for mobile