1382199 - If Shift+F10 is being processed by a web application like Gmail, the standard Windows system menu is invoked.

Status: VERIFIED FIXED

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

Description

[Marco Zehe (:MarcoZ)]

STR:
1. Log into Gmail and navigate to your Inbox.
2. Focus a message.
3. Press Shift+F10.

Expected: The context menu should open that Google provides for the individual message.

Actual: It does open, but at the same time, the Windows system menu opens (the one you usually invoke with Alt+SpaceBar), overlapping the actual menu. You have to close it with Escape first before you can navigate the Google-provided context menu for the focused message.

This does not happen for me with E10S off.

Screen Readers also get easily confused by this mix of menus.

Jim, since this is not a Disability Access APIs bug, but I suspect affects users without screen readers, too, I will put it in the Keyboard: Navigation component, but am NI'ing you since this will also affect screen reader users using Gmail. And that number is growing.

Comment 1

[Jim Mathies [:jimm]]

(In reply to Marco Zehe (:MarcoZ) from comment #0)
> STR:
> 1. Log into Gmail and navigate to your Inbox.
> 2. Focus a message.
> 3. Press Shift+F10.
> 
> Expected: The context menu should open that Google provides for the
> individual message.
> 
> Actual: It does open, but at the same time, the Windows system menu opens
> (the one you usually invoke with Alt+SpaceBar), overlapping the actual menu.

On Windows 7 and latest Nightly, I get the Firefox content context menu. Let see if we can get qa to test a bit.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

fwiw, I cannot reproduce this on Win10 latest nightly. The menu google provides for an individual message was shown.

Comment 3

[Marco Zehe (:MarcoZ)]

jimm, could you see if we're firing accessible menu events that actually don't correspond with what's happening visually? To me, it sounds like there is more going on on the screen reader event side than what you are seeing on the screen.

Comment 4

[u554753]

With the STR provided, I cannot reproduce this behavior on Windows 10 with the Newest Nightly. I'm doing exploratory testing to see if I can try and reproduce the issue.

Comment 5

[u554753]

Marco, what was your method for focusing the message in Step 2 of your STR?

Comment 6

[u554753]

I cannot seem to reproduce the issue on any of the low-end touch screen laptops.

Comment 7

[Marco Zehe (:MarcoZ)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #5)
> Marco, what was your method for focusing the message in Step 2 of your STR?

Well, for one, when logging into Gmail, NVDA goes into focus mode automatically, meaning it disables its virtual cursor and allows direct interaction with Firefox through the keyboard. I then just hit up and down arrows to focus a particular message (doesn't matter which). I then press Shift+F10. And what happens for me is that I get both the message's context menu, but in front of that the same system menu that I would get if I pressed Alt+SpaceBar. Subsequently, DownArrow opens that for me, and if focus also changes on the context menu items on the actual page, I do not know. For NVDA, the thing in focus is that system menu with the Restore, Minimize, Maximize etc. options. 100% reproducible on a Surface Book with Windows 10 Creators Update. No Insider builds running.

Comment 8

[u554753]

Was able to reproduce the bug in newest version of Nightly after updating Windows 10 to current build:

Steps to Reproduce:
1. Open NVDA screenreader
2. Open Nightly
3. Navigate to http://www.gmail.com/
4. In your inbox, hold the down arrow key until the cursor is over an email.
5. Press Shift + F10 (the menu with "Move to Tab", "Archive", "Mark as Read", etc is opened)
6. Press the down arrow key 

Expected Result:
The arrow keys navigate through the gmail choices.

Actual Result:
Windows system menu context window is opened up and flickers

Comment 9

[Jim Mathies [:jimm]]

I can repo this in non-e10s mode.

Comment 10

[James Teh [:Jamie]]

Attachment: Bug 1382199: On Windows, prevent the System menu bar from triggering when F10 is released.

Windows default behavior will trigger the System menu bar when F10 is released.
Among other things, this causes the System menu bar to appear when a web page overrides the contextmenu event.
We *never* want this default behavior, so eat this key (never pass it to Windows).

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

Comment 11

[James Teh [:Jamie]]

Some additional explanation:

Windows default behaviour is to trigger the native (Win32) menu bar when F10 is released (that's key up, *not* key down). if there's no menu bar, it still brings up the System menu bar. That is, it doesn't fully open the System menu like alt+space does, but it focuses the System menu such that pressing enter or down arrow will open the System menu. You can see this behaviour in the Run dialog:
1. Press Windows+r.
2. Tab to the OK button.
3. Press F10, hold it for half a second, then release it. Notice that it only triggers when the key is released.
4. Press down arrow. Notice that the System menu opens.

This applies to F10 even with modifiers. For example, repeat the steps above, but change F10 to shift+F10 in step 3.

In Gecko:
1. Something prevents this key up from hitting the OS when F10 is pressed to open the menu bar. I haven't yet worked out what prevents that, but I'm guessing it's something in nsMenuBarListener.
2. Something also prevents it from triggering when shift+f10 is pressed to open a Gecko context menu. Again, I don't know what exactly prevents this.
3. However, it is not prevented when a web page listens for the contextmenu event and calls preventDefault(). This is the cause of this bug.
4. It also isn't prevented if you press f10 while a Gecko context menu is already open.
5. It isn't prevented if Gecko is delayed in opening a Gecko context menu; e.g. if you press shift+f10 while a screen reader virtual buffer is rendering. My guess is that the check that prevents this for 2) doesn't work/apply in this case because the context menu isn't shown yet when F10 is released.

It's worth noting that 4) and 5) result in a set of circumstances which cause NVDA to crash Firefox due to a re-posted window message (probably caused by the System menu):
https://github.com/nvaccess/nvda/issues/8000
bp-0a765699-4c9c-4c59-9dcf-c581c0180215
While NVDA should protect against this, I thought it worth noting as an additional factor anyway.

We clearly already protect this case elsewhere for 1) and 2), and it could be argued that the correct fix for 3) and 4) belongs somewhere else in the code other than nsWindow. However, we really never ever want this default Windows behaviour, so it seems to make sense to deal with it at this level. Also, I'm not really sure how we'd fix the web page contextmenu case (3) without some obscure state tracking. We can't exactly ask the web page whether a context menu is open, so we'd have to track whether the key up was for the key down that triggered the contextmenu event.

Comment 12

[James Teh [:Jamie]]

Simpler repro:
1. Open this URL:
data:text/html,test<script>document.addEventListener("contextmenu", function (evt) { alert('hello'); evt.preventDefault(); });</script>
2. Press shift+F10.
3. press enter.
Expected: The alert should be dismissed.
Actual: The System menu opens.

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8951492 [details]
Bug 1382199: On Windows, prevent the System menu bar from triggering when F10 is released.

https://reviewboard.mozilla.org/r/220794/#review228476

Comment 14

[Jim Mathies [:jimm]]

This looks safe to me. cc'ing Masayuki who handles a lot of our widget keyboard input code.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ccb5c3d91f7
On Windows, prevent the System menu bar from triggering when F10 is released. r=jimm

Comment 16

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

Hmm, I think that the patch is not enough. We shouldn't break native application's behavior as far as possible.

In this case, it's okay to consume F10 key message *temporarily*. However, if the eKeyUp event is back from remote process, we should send the message to default wndproc if it's not consumed by web apps.

Comment 17

[James Teh [:Jamie]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> Hmm, I think that the patch is not enough. We shouldn't break native
> application's behavior as far as possible.

Can you explain a situation where we might want this native behaviour?

> In this case, it's okay to consume F10 key message *temporarily*. However,
> if the eKeyUp event is back from remote process, we should send the message
> to default wndproc if it's not consumed by web apps.

The problem (and perhaps I'm missing something that invalidates this) is that web apps (or anything else for that matter; e.g. XUL) might consume the key down, but they probably won't consume the key up... unless that somehow happens automatically when the key down is consumed. In the case of the contextmenu event, I think it's actually Windows consuming the key press and converting it to a context menu message, but I'm pretty sure it doesn't consume the key up event either. If it did, we wouldn't have hit this problem in the first place.

Comment 18

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

(In reply to James Teh [:Jamie] from comment #17)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> > Hmm, I think that the patch is not enough. We shouldn't break native
> > application's behavior as far as possible.
> 
> Can you explain a situation where we might want this native behaviour?

Hmm, I misunderstood a thing. On our window whose content is created by XUL without menubar, pressing F10 does NOT move focus to the system menu. I think that this is a bug of us.

On the other hand, always not sending F10 keyup message to OS is really bad. There is no guarantee that it won't become more important with newer Win10 builds.

Although I still don't understand fully of the cause. However, I'm still thinking that overriding the result in nsWindow is wrong approach.

> > In this case, it's okay to consume F10 key message *temporarily*. However,
> > if the eKeyUp event is back from remote process, we should send the message
> > to default wndproc if it's not consumed by web apps.
> 
> The problem (and perhaps I'm missing something that invalidates this) is
> that web apps (or anything else for that matter; e.g. XUL) might consume the
> key down, but they probably won't consume the key up...

Yeah, that's good point. However, why F10 keyup in most cases won't cause moving focus to the system menu even if there is no menubar like separated window of devtools and About (Firefox|Nightly) dialog?

> unless that somehow
> happens automatically when the key down is consumed. In the case of the
> contextmenu event, I think it's actually Windows consuming the key press and
> converting it to a context menu message, but I'm pretty sure it doesn't
> consume the key up event either. If it did, we wouldn't have hit this
> problem in the first place.

We already have similar hack. That is Alt keyup:
https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/widget/windows/KeyboardLayout.cpp#1855-1864
In that case, not-consumed message comes with WM_SYSKEYUP, but otherwise, comes with WM_KEYUP. Isn't similar behavior used in this case? So, it might be better to consume VK_F10 keyup message only when preceding eContextMenu or eKeyDown is consumed even if we need to handle it in nsWindow.

Anyway, I should give you some points which you didn't find:
> 1. Something prevents this key up from hitting the OS when F10 is pressed to open the menu bar. I haven't yet worked out what prevents that, but I'm guessing it's something in nsMenuBarListener.

https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/layout/xul/nsMenuBarListener.cpp#194,211,218,246-247,249

> 2. Something also prevents it from triggering when shift+f10 is pressed to open a Gecko context menu. Again, I don't know what exactly prevents this.

https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/layout/xul/nsXULPopupManager.cpp#2658,2661-2664,2666-2668,2674

Comment 19

[James Teh [:Jamie]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #18)
> Hmm, I misunderstood a thing. On our window whose content is created by XUL
> without menubar, pressing F10 does NOT move focus to the system menu. I
> think that this is a bug of us.

Perhaps we can't explain the code path, but I'm not sure I agree it's a bug. The default behaviour is designed for apps with native menu bars. The fact that it brings up the System menu bar is only because we don't have a native menu bar. Instead, we handle all of this ourselves.

> On the other hand, always not sending F10 keyup message to OS is really bad.
> There is no guarantee that it won't become more important with newer Win10
> builds.

I follow the logic, but it's worth noting that this is not unusual for modern apps. Certainly, the System menu bar does not ever appear (outside of common dialogs) when you press F10 in apps such as Explorer, Wordpad and Windows Store apps.

> Yeah, that's good point. However, why F10 keyup in most cases won't cause
> moving focus to the system menu even if there is no menubar like separated
> window of devtools and About (Firefox|Nightly) dialog?

Honestly, I don't know. However, I believe it to be the correct behaviour as noted above. Still, it's bad that we don't understand *how* this is working.

> So, it might
> be better to consume VK_F10 keyup message only when preceding eContextMenu
> or eKeyDown is consumed even if we need to handle it in nsWindow.

I can certainly try this. My concern is that we might not be consuming it in all cases where we should, resulting in this rather ugly (and no longer common) behaviour.

Essentially, there are two possible solutions here:
1. Deal with every specific case (XUL menu bar, XUL context menus, web context menus, web key down, XUL windows without menu bars, etc.) and make sure we consume F10 in every case, because in all of these cases, it's undesirable behaviour.
2. Just prevent the undesirable behaviour at the platform level, as it seems most modern apps do.

I went for the second approach because it's a lot easier to understand and means we have less platform specific stuff floating throughout the code base, as well as avoiding this happening again for future cases where it isn't desirable. That said, this is your domain, so it's ultimately your call and I'll do whatever is needed, though I may need a little help dealing with the edge cases. :)

Unfortunately, I already autolanded this; I didn't realise there might be further blocking feedback. Sorry about that. Do you want me to push a patch to back this out now and we can continue figuring it out?

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9ccb5c3d91f7

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This isn't a new regression and we're pretty late in the 59 development cycle, so I think we should let this ride the 60 train.

Comment 22

[Alexandru Simonca [:asimonca], Release Desktop QA]

Reproduced on Windows 10 x64 using Firefox 60.0a1 (id: 20180123102017). This issue is Verified Fixed in Firefox Beta 60.0b6 on Windows 10 x64 and Windows 10 x86.