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.

Sign up for GitHub

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

Jump to bottom

Allow browser context menu when possible #7369

Merged
merged 1 commit into from
Oct 5, 2018
Merged

Allow browser context menu when possible #7369

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/ui/bind_handlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,11 @@ export default function bindHandlers(map: Map, options: {interactive: boolean, c
contextMenuEvent = e;
}

e.preventDefault();
// prevent browser context menu when necessary; we don't allow it with rotation
// because we can't discern rotation gesture start from contextmenu on Mac
if (map.dragRotate.isEnabled() || map.listens('contextmenu')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the first part of this conditional but am I thinking about it correctly that map.listens('contextmenu') would be true if a contextmenu listener has been set up on the map? Why would having a listener prevent the event? It seems like odd behavior that I could disable drag rotate, set up a listener for contextmenu and then the context menu wouldn't actually open.

Also, the docs say that listens should return false if no listener is set up, but I actually get undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

The original reason for having Map contextmenu event is to allow specifying custom behavior on right click, such as opening your own HTML-styled app-specific context menu, and in this case it doesn't make sense to also show the browser default one.

Leaflet does the same thing and no one raised concerns about it yet, so I think it's safe to do the same logic here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok that makes sense. I tested this on Mac with Safari, Firefox and Chrome and they all worked as expected so I think this is good.

Copy link

Choose a reason for hiding this comment

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

This is not working in ios WKWebView.

e.preventDefault();
}
}

function onWheel(e: WheelEvent) {
Expand Down