1338369 - Shift+F10 not throwing contextmenu event in javascript

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Microsoft Office Web Apps]

Attachment: Repro page with instructions and seen results on 51.0.1-bit

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

On an html page that has an input element and javascript that attaches a custom function that prevents the default behavior to handle the "contextmenu" events in the input element type Shift+F10 as suggested in https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/PopupGuide/ContextMenus to run the handler function of the context menu.


Actual results:

The handler function is not called so the default behavior is not prevented.
The default browser's menu is displayed in the input element.


Expected results:

The handler function should have been called, preventing the default behavior of the browser.

Comment 1

[Andrew Overholt [:overholt]]

Olli, are we just not intercepting the key events like we should?

Comment 2

[Microsoft Office Web Apps]

The Shift+F10 combination is displaying the default context menu of the browser instead of throwing the "contextmenu" event so it can be handled/prevented by the registered handler in the html page that's being displayed which should be the seen behavior according to the ContextMenus page at Mozilla's developer web site.

Yes, the browser is not intercepting the key events like it should.

Comment 3

[Microsoft Office Web Apps]

(In reply to Andrew Overholt [:overholt] from comment #1)
> Olli, are we just not intercepting the key events like we should?

Andrew, actually replying to your comment:

The Shift+F10 combination is displaying the default context menu of the browser instead of throwing the "contextmenu" event so it can be handled/prevented by the registered handler in the html page that's being displayed which should be the seen behavior according to the ContextMenus page at Mozilla's developer web site.

Yes, the browser is not intercepting the key events like it should.

Comment 4

[Andrew Overholt [:overholt]]

Thanks for clarifying.

Masayuki knows lots about this kind of thing so maybe he can chime in.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Silly me, I thought this was e10s specific, but looks like not, happens in non-e10s too

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Ah, we must hit this path:
https://dxr.mozilla.org/mozilla-central/rev/32dcdde1fc64fc39a9065dc4218265dbc727673f/layout/base/PresShell.cpp#8035,8041-8042

I think that when Shift+F10 causes context menu event, its Shift key state should be consumed before dispatch like when Ctrl/Alt+foo causes inputting text.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02fb3453fdf5

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1338369 part.1 nsWindow for Windows should consume Shift key state at dispatching eContextMenu event if it's caused by Shift+F10

In PresShell, eContextMenu event is marked as dispatched only in chrome when its shiftKey state is true. However, Shift+F10 causes the context menu, it should not be marked as so because this is standard shortcut key to open context menu on Windows.

This patch consumes Shift key state if previous key message is WM_SYSKEYDOWN of F10 before dispatching eContextMenu event.  So, user cannot block to prevent its default at opening context menu with Shift+F10, we should discuss this later.

Review commit: https://reviewboard.mozilla.org/r/115346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115346/

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Attachment: Bug 1338369 part.2 nsWindow for GTK should consume Shift key state of eContextMenu event if it's caused by Shift+F10

Shift+F10 is also well-known shortcut key on Linux.  So, it should behave same as pressing ContextMenu key.  So, for allowing web page to prevent its default, nsWindow for GTK needs to consume Shift key state at dispatching eContextMenu key.

Additionally, we should allow to open context menu with Shift+ContextMenu because only ContextMenu key press can be prevented its default by web page.  Therefore, we should allow users to open context menu even with keyboard even if web content doesn't want it.

Note that Ctrl+Shift+F10 or Alt+Shift+F10 should behave same as Shift+ContextMenu key, but we should discuss later.

Review commit: https://reviewboard.mozilla.org/r/115348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115348/

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8840966 [details]
Bug 1338369 part.1 nsWindow for Windows should consume Shift key state at dispatching eContextMenu event if it's caused by Shift+F10

https://reviewboard.mozilla.org/r/115346/#review116886

I guess we can do this. I would perhaps just changed the presshell code a bit, but I don't object this approach.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8840967 [details]
Bug 1338369 part.2 nsWindow for GTK should consume Shift key state of eContextMenu event if it's caused by Shift+F10

https://reviewboard.mozilla.org/r/115348/#review116888

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #10)
> I would perhaps just changed the presshell code a bit

Well, I though that if we fix this in PresShell side, we can do:

* Ignore Shift key immediately after F10
 - This makes PresShell member too messy and not good approach because Shift+F10 is platform specific.
* Always ignore Shift key if eContextMenu caused by a key
 - This makes users inconvenient if they use only keyboard.

Therefore, (I didn't want to change each widget, though,) I changed each widget.

But there is another issue.

ContextMenu key may not be in keyboard. For example, HHK (*1) doesn't have ContextMenu key but it's major product especially for such users who like to navigate browser without pointing device. And also MacBook series don't have ContextMenu key, so, when user installs Win or Linux to Mac, they cannot use ContextMenu key too. Thus, some users may open context menu only with Shift+F10, but they cannot override web pages which block to open context menu.

Do you have a good idea to solve this issue? Ctrl+Shift+F10 or Alt+Shift+F10 might be an option to fix this issue. But perhaps, these combinations are not easy to press and I worry about conflict with DevTools because DevTools team like Ctrl+Shift+Foo.

1: https://www.google.co.jp/search?q=HHK&client=firefox-b&source=lnms&tbm=isch&sa=X&ved=0ahUKEwid_e2ckKrSAhXMzLwKHc0TD0YQ_AUICCgB&biw=1296&bih=759#imgrc=cebh-lk5_Pk03M:

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Masayuki Nakano [:masayuki] from comment #12)

> Do you have a good idea to solve this issue? Ctrl+Shift+F10 or Alt+Shift+F10
> might be an option to fix this issue. But perhaps, these combinations are
> not easy to press and I worry about conflict with DevTools because DevTools
> team like Ctrl+Shift+Foo.
Indeed. And also, how does anyone know about these shortcut keys.
But if devtools use Ctrl+Shift+foo rather often, perhaps Alt is fine.

How do other browsers deal with this? I couldn't figure out how to prevent contextmenu event (and just show the context menu) in Chrome/linux

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to Olli Pettay [:smaug] (high review load, queue closed for couple of days) from comment #13)
> (In reply to Masayuki Nakano [:masayuki] from comment #12)
> 
> > Do you have a good idea to solve this issue? Ctrl+Shift+F10 or Alt+Shift+F10
> > might be an option to fix this issue. But perhaps, these combinations are
> > not easy to press and I worry about conflict with DevTools because DevTools
> > team like Ctrl+Shift+Foo.
> Indeed. And also, how does anyone know about these shortcut keys.

Honestly, Shift+Right-Click isn't so major. When I tweeted it, I see some tweets which are surprised at such function.

> But if devtools use Ctrl+Shift+foo rather often, perhaps Alt is fine.

Yeah, anyway, 3 keys are necessary though.

> How do other browsers deal with this? I couldn't figure out how to prevent
> contextmenu event (and just show the context menu) in Chrome/linux

I installed Chromium into my Ubuntu 16.10. However, I cannot override the context menu with mouse nor keyboard. (Same with Google Chrome on Windows)

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

jimm, karlt:

ping... If you don't want to review each patch or don't have much time, I'll land them without your review.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8840967 [details]
Bug 1338369 part.2 nsWindow for GTK should consume Shift key state of eContextMenu event if it's caused by Shift+F10

https://reviewboard.mozilla.org/r/115348/#review120298

::: widget/gtk/nsWindow.cpp:3199
(Diff revision 1)
> +    if (contextMenuEvent.IsControl() || contextMenuEvent.IsMeta() ||
> +        contextMenuEvent.IsAlt()) {
> +        return false;
> +    }
> +
> +    // If the key is ContextMenu key, we should not check its Shift state

Do you mean "we should not *change* its Shift state"?

::: widget/gtk/nsWindow.cpp:3200
(Diff revision 1)
> +    // because when it's pressed without Shift state, web page should be able
> +    // to prevent its default.  Otherwise, with Shift state, web page shouldn't
> +    // be able to prevent its default.

Can you name the function that implements this behavior in a comment here, please?

Otherwise someone might read this comment to mean that the code below implements this.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 8840967 [details]
Bug 1338369 part.2 nsWindow for GTK should consume Shift key state of eContextMenu event if it's caused by Shift+F10

https://reviewboard.mozilla.org/r/115348/#review120298

> Do you mean "we should not *change* its Shift state"?

Or perhaps "If the key is ContextMenu, then an eContextMenu mouse event is dispatched regardless of the state of the Shift modifier.  When it is pressed without the Shift modifier, a web page can prevent the default context menu action.  When pressed with the Shift modifier, the web page cannot prevent the default context menu action."

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 8840967 [details]
Bug 1338369 part.2 nsWindow for GTK should consume Shift key state of eContextMenu event if it's caused by Shift+F10

https://reviewboard.mozilla.org/r/115348/#review120416

::: widget/gtk/nsWindow.cpp:3199
(Diff revision 1)
> +    if (contextMenuEvent.IsControl() || contextMenuEvent.IsMeta() ||
> +        contextMenuEvent.IsAlt()) {
> +        return false;
> +    }
> +
> +    // If the key is ContextMenu key, we should not check its Shift state

Thank you, I'll take your suggestion!

::: widget/gtk/nsWindow.cpp:3200
(Diff revision 1)
> +    // because when it's pressed without Shift state, web page should be able
> +    // to prevent its default.  Otherwise, with Shift state, web page shouldn't
> +    // be able to prevent its default.

I'll add |(PresShell::HandleEventInternal() sets mOnlyChromeDispatch to true.)| here.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 8840966 [details]
Bug 1338369 part.1 nsWindow for Windows should consume Shift key state at dispatching eContextMenu event if it's caused by Shift+F10

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115346/diff/1-2/

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Comment on attachment 8840967 [details]
Bug 1338369 part.2 nsWindow for GTK should consume Shift key state of eContextMenu event if it's caused by Shift+F10

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115348/diff/1-2/

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Okay, no response from jimm. He must be busy and I usually hack around keyboard layout on Windows too. So, I bet he doesn't have any objection about this patch. If there were, it should be fixed in follow up bug.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6413896b93ff
part.1 nsWindow for Windows should consume Shift key state at dispatching eContextMenu event if it's caused by Shift+F10 r=smaug
https://hg.mozilla.org/integration/autoland/rev/c20235d9b540
part.2 nsWindow for GTK should consume Shift key state of eContextMenu event if it's caused by Shift+F10 r=karlt,smaug

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6413896b93ff
https://hg.mozilla.org/mozilla-central/rev/c20235d9b540