Skip to content
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

Merged
merged 9 commits into from
Jul 21, 2023

Conversation

tymmesyde
Copy link
Member

@tymmesyde tymmesyde commented Jul 18, 2023

The context menu was not working on Firefox because the contextmenu event is not a PointerEvent on Firefox and we were using the pointerEvent property to check if it was from a mouse
https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event#browser_compatibility

The contextmenu event is managed by the browser, on desktop it should trigger on right click, on mobile on long press
So we don't really need to use the onLongPress callback

We ended up using mouseup instead as it's a MouseEvent and check if right click was pressed
onMouseUp for desktop and onLongPress for mobile

@unclekingpin
Copy link
Contributor

There must be a better way to solve this because that will break UX on mobile safari.
Can we check if the nativeEvent is instance of PointerEvent and if its not then threat it as a mouse event
cc: @dexter21767-dev

@unclekingpin
Copy link
Contributor

unclekingpin commented Jul 19, 2023

     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]);

Copy link
Member

@TRtomasz TRtomasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this indeed fix context menu but i noticed this in the console
image
Also for some reason i have no streams for movies using this build, not sure if it got broken with some other change

@tymmesyde
Copy link
Member Author

this indeed fix context menu but i noticed this in the console image Also for some reason i have no streams for movies using this build, not sure if it got broken with some other change

The error was introduced in this PR: #375
It's a prop check error so nothing too alarming

@unclekingpin unclekingpin self-requested a review July 20, 2023 19:52
unclekingpin
unclekingpin previously approved these changes Jul 20, 2023
@tymmesyde tymmesyde merged commit 3bc10fc into development Jul 21, 2023
1 check passed
@elpiel elpiel deleted the fix/video-context-menu branch July 21, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants