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

Use e.composedPath in addition to e.path for firefox compatibility #520

Merged
merged 4 commits into from
Sep 29, 2021

Conversation

ben-lu-uw
Copy link
Contributor

Fix for issues #519 and #465

Copy link
Member

@Malvoz Malvoz left a comment

Choose a reason for hiding this comment

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

I'm seeing an issue in mapml-viewer.html where Enter does not activate menu items (whereas Space does), although it works in featureLinks.html (<map>).

Additionally, in both maps Space and the V shortcut to View source is now blocked with the message: "Firefox prevented this site from opening a pop-up window.", although Enter does work to View source.

@ben-lu-uw
Copy link
Contributor Author

ben-lu-uw commented Sep 28, 2021

I'm seeing an issue in mapml-viewer.html where Enter does not activate menu items (whereas Space does), although it works in featureLinks.html (<map>).

  • I do not seem to be having these issues but I will continue to look into it

Additionally, in both maps Space and the V shortcut to View source is now blocked with the message: "Firefox prevented this site from opening a pop-up window.", although Enter does work to View source.

  • I guess Firefox is more pick about window.open and popups compared to Chrome. Will try to find a work around

@ben-lu-uw ben-lu-uw marked this pull request as draft September 28, 2021 15:22
@prushforth
Copy link
Member

Additionally, in both maps Space and the V shortcut to View source is now blocked with the message: "Firefox prevented this site from opening a pop-up window.", although Enter does work to View source.

  • I guess Firefox is more pick about window.open and popups compared to Chrome. Will try to find a work around

That is pretty odd that ff allows the "popup" (new tab) when one uses "Enter" vs the shortcut "V". Could it be that they are mapped to different ways of doing the action?

I actually have another issue in Chrome: when you activate the context menu via Shift-F10 (or right-click), the first menu item is not focused. In FF, the first menu item is focused when you Shift-F10, but not when you right-click. Maybe that's worthy of a separate issue.

@Malvoz
Copy link
Member

Malvoz commented Sep 28, 2021

I actually have another issue in Chrome: when you activate the context menu via Shift-F10 (or right-click), the first menu item is not focused. In FF, the first menu item is focused when you Shift-F10, but not when you right-click. Maybe that's worthy of a separate issue.

See #469. I don't think the expectation is for focus to be delegated on right click however.

@ahmadayubi
Copy link
Member

ahmadayubi commented Sep 28, 2021

I also had the issue where Space activated the menu items but Enter did not. I got the same behavior as @Malvoz

@ahmadayubi
Copy link
Member

One solution to this is to manually configure the behavior for Enter (keyCode 13), it'd have the same behavior of Space. You'd have to preventDefault though. (This is just one of many solutions, feel free to go with whatever you find best)

In ContextMenu.js line 528

let key = e.keyCode;
let path = e.path || e.composedPath();
if(key === 13) e.preventDefault(); //Add this line
if(key !== 16 && key!== 9 && !(!this._layerClicked && key === 67) && path[0].innerText !== "Copy Coordinates (C) >")
  this._hide();
switch(key){
  case 13: //And this line

@ben-lu-uw
Copy link
Contributor Author

Giving the enter key the same behavior as the space button resolves the menu issue however, hitting the enter button over the view source option leads to the "popup" being blocked, just like space and v. I believe its because its under the keydown event : https://stackoverflow.com/a/6132481

@ahmadayubi
Copy link
Member

ahmadayubi commented Sep 28, 2021

Right that was only for Enter, for the popup issue, if Firefox is blocking it one work around could be to add a <a href="link to goto"> to the DOM, click it using element.click() then removing that temporary <a> from the DOM.

A similar thing is done for copying to clipboard. Where a temporary element is added to the DOM, you interact with that elements API, then remove the temporary element.
https://github.com/Maps4HTML/Web-Map-Custom-Element/blob/9d9183097e746f6d4e1c0011eed4502795ff8edf/src/mapml/handlers/ContextMenu.js#L246

@ben-lu-uw
Copy link
Contributor Author

if Firefox is blocking it one work around could be to add a to the DOM, click it using element.click() then removing that temporary from the DOM.

Tried this and other work arounds but they all get blocked by Firefox when trying to open a new tab. From what I have read, the user has to explicitly click themselves in order for the new tab to open or the user has to allow popups for the site.

@ahmadayubi
Copy link
Member

I guess they're really strict about popups. The other idea I had in mind was moving that function to the mapml-extension. Web extensions have a contextmenu API to add to the built in context menu, it would be a big undertaking though.

But like you mentioned the user always has the option of loosening their popup blocking settings in Firefox.

Copy link
Member

@ahmadayubi ahmadayubi left a comment

Choose a reason for hiding this comment

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

You can actually merge cases in JavaScript, something like this works as well:

case 13:
case 32:
    if(....
    ...
break;

@ben-lu-uw ben-lu-uw marked this pull request as ready for review September 29, 2021 13:27
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.

Context menu keyboard interactions in Firefox do not work Escape key does not dismiss popup in Firefox
4 participants