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

[Screencast]: Disable context menu #555

Merged
merged 5 commits into from
Oct 13, 2021
Merged

Conversation

mliao95
Copy link

@mliao95 mliao95 commented Oct 12, 2021

This PR removes context menu from the screencast since we cannot easily replicate the OS generated context menu from the target.

We disable the context menu in two places:

  • from appearing on the screencast canvas by adding a context menu listener that calls event.preventDefault()
  • from appearing in the target browser by preventing mouse events with mouseEvent.button === 2 from being emitted to the page.

@mliao95 mliao95 requested review from bgoddar, antross and hkal October 12, 2021 23:32
@mliao95
Copy link
Author

mliao95 commented Oct 12, 2021

Do you all think we should disable the right click button to be emitted to the page as well? I had that change set up initially, but decided against it in case the target page overrides the right click functionality.

The main con is that the target page (not the screencast), will have a context menu open that the user cannot see or interact with from the screencast.

image

@antross
Copy link
Contributor

antross commented Oct 13, 2021

Do you all think we should disable the right click button to be emitted to the page as well?

That's my leaning. I don't think I'd expect to find the context menu in the browser, it won't really work for headless, and if I really want it I can interact with the real browser window directly.

@mliao95 mliao95 merged commit de054f3 into main Oct 13, 2021
@vidorteg vidorteg deleted the user/milia/disable-context-menu branch January 30, 2024 03:39
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.

3 participants