1200038 - Active tab shouldn't change when I navigate in context menu opened for <tab> with Up and Down keys

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[arni2033]

Attachment: video - Active tab shouldn't change when I navigate in context menu opened for tab with Up and Down keys.webm

STR:   (Win7_64, Nightly 43, 32bit, ID 20150828030205, new profile, HWA off)
0. Open New Tab.
1. Press Ctrl+L to focus location bar.
2. Press Shift+Tab twice to focus tab (like said on SuMO https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly#w_windows-tabs ).
3. Press Shift+F10 to open context menu for tab.
4. Switch between menuitems with Up and Down keys.

Result:       [watch video] Tabs are switching
Expectations: Active tab shouln't change, just like when I right-click <tab> instead of Steps 1-3
              and perform Step 4.

Comment 1

[arni2033]

This was regressed between Fx31 and Fx32. Not sure if the regression window would be helpful here

Comment 2

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=62c85e7fc6ad&tochange=ccc885c8f4ac

REGRESSED BY: Bug 1008772

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

I guess that we need to use keydown events to navigate menu items instead of keypress events.

Comment 4

[Marco Zehe (:MarcoZ)]

This is still a problem in 47 nightlies. I suspect the KeyPress events need to be prevented from propagating outside the context menu. Gijs, have someone on the team who can take a look at this soon'ish? It is really irritating and produces a level of unpredictability that isn't funny. :-(

Comment 5

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> I guess that we need to use keydown events to navigate menu items instead of
> keypress events.

That's not really a trivial change, but would also be a core change rather than one in the tabbrowser... Is this the best solution? If so, can someone from platform look at this?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> > I guess that we need to use keydown events to navigate menu items instead of
> > keypress events.
> 
> That's not really a trivial change, but would also be a core change rather
> than one in the tabbrowser... Is this the best solution? If so, can someone
> from platform look at this?

Hmm, my investigation may be wrong:
http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp?rev=03fc467e1002&mark=2561-2561#2555

We've already used keydown event handler for handling keyboard events at navigating focus in menus. And the keydown event listener listens at the XUL document node in capture phase:
http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp?rev=03fc467e1002&mark=1892-1892,1897-1897#1885

So, the actual cause of this bug is, tabbox.xml doesn't check defaultPrevented value before handling keyboard events. However, it shouldn't stop handling keyboard events if they are consumed by web contents. After bug 111030, chrome JS will be possible to check if each event has been consumed by chrome or content, though.

But I still not sure about one thing. Looks like nsXULPopupManager consumes *and* stops propagation:
http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp?rev=03fc467e1002&mark=2271-2271,2273-2279,2342-2346#2252
Therefore, I'm not sure why the keyboard event is handled by tabbox again...

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(Away: -9/28)]

Sigh, I meant bug 1110030.

Comment 8

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> > > I guess that we need to use keydown events to navigate menu items instead of
> > > keypress events.
> > 
> > That's not really a trivial change, but would also be a core change rather
> > than one in the tabbrowser... Is this the best solution? If so, can someone
> > from platform look at this?
> 
> Hmm, my investigation may be wrong:
> http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.
> cpp?rev=03fc467e1002&mark=2561-2561#2555
> 
> We've already used keydown event handler for handling keyboard events at
> navigating focus in menus. And the keydown event listener listens at the XUL
> document node in capture phase:
> http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.
> cpp?rev=03fc467e1002&mark=1892-1892,1897-1897#1885
> 
> So, the actual cause of this bug is, tabbox.xml doesn't check
> defaultPrevented value before handling keyboard events. However, it
> shouldn't stop handling keyboard events if they are consumed by web
> contents. After bug 111030, chrome JS will be possible to check if each
> event has been consumed by chrome or content, though.
> 
> But I still not sure about one thing. Looks like nsXULPopupManager consumes
> *and* stops propagation:
> http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsXULPopupManager.
> cpp?rev=03fc467e1002&mark=2271-2271,2273-2279,2342-2346#2252
> Therefore, I'm not sure why the keyboard event is handled by tabbox again...

AIUI tabbrowser.xml uses system-level event listeners, and it seems like the XUL browser code uses "normal" event listeners, which presumably explains the difference.

Comment 12

[James Teh [:Jamie]]

This bug drives me insane. Taking.

Comment 13

[James Teh [:Jamie]]

Attachment: Bug 1200038: Don't use the system group for XUL tabbox keydown handlers.

Bug 1008772 moved all key handlers related to browser tabs to the system group to prevent websites from stealing tab switching keys.
However, this is only necessary for global tab switching keys; e.g. Control+Tab, Control+PageUp.
It is not necessary for keys which only operate when the tab bar is focused; e.g. Up/DownArrow.
This is because when the browser chrome is focused (including the tab bar), content key handlers can't intercept key presses.
Furthermore, having these in the system group gives them precedence over XUL context menus.
This causes tab switches to occur when the arrow keys are pressed while the context menu is open, which is highly undesirable.
Moving these (non-global) keydown handlers into the normal group fixes this.

Comment 14

[James Teh [:Jamie]]

Is there a particular try syntax/fuzzy thingy recommended for something like this? It'd make sense to run everything in browser/base/content/test and toolkit/content/tests/chrome/test_tabbox.xul on Windows/Mac/Linux, but I don't know how to specify those specific tests.

Comment 15

[:Gijs (he/him)]

(In reply to James Teh [:Jamie] from comment #14)
> Is there a particular try syntax/fuzzy thingy recommended for something like
> this? It'd make sense to run everything in browser/base/content/test and
> toolkit/content/tests/chrome/test_tabbox.xul on Windows/Mac/Linux, but I
> don't know how to specify those specific tests.

try: -b od -t none -p win64,macosx64,linux64,linux -u mochitests --artifact

should work.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 9005992 [details]
Bug 1200038: Don't use the system group for XUL tabbox keydown handlers.

Dão Gottwald [::dao] has approved the revision.

Comment 17

[James Teh [:Jamie]]

Thanks!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3657579c1d6acfca14e34b04ad01caadb91fcef0

Comment 18

[James Teh [:Jamie]]

The test was failing on Mac because tabs aren't focusable by default their. So, I had to set accessibility.tabfocus to 7. However, even with that set, I get failures. It seems that when you press escape to leave the tab context menu on Mac, focus doesn't return to the tab; the focus goes to the window. If I call tab1.focus() after dismissing the context menu, it behaves.

Dao, is this expected behaviour for context menus on Mac? Even if it isn't, is it acceptable to just work around it with tab1.focus() after dismissing the context menu, given that this isn't actually behavior we're testing here? I don't have a Mac, so am unable to debug this properly. However, this would be an unrelated issue that just happens to affect this test.

Comment 19

[Dão Gottwald [:dao]]

Calling tab1.focus depending on AppConstants.platform == "macosx" sounds good to me.

Comment 20

[James Teh [:Jamie]]

Confirmed the test is now passing on Mac with the patch I'm about to land:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba522293cbb6f7972603424400b0293ac9aa7535&selectedJob=197339736

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d81f22a74ce
Don't use the system group for XUL tabbox keydown handlers. r=dao

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0d81f22a74ce

Comment 23

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 43.0a1 (2015-08-30) on WIndows 10, 64 Bit!
This bug's fix is verified with latest Nightly!

Build ID 	20180926220146
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0