1425972 - Keyboard navigation in panelmultiview subviews should use the same position for tabbing and arrowing

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Ian Moody [:Kwan] (UTC+1)]

+++ This bug was initially created as a clone of Bug #1354144 +++

Arrow keys and tabbing within menus seem to use two different values for current position, with a one way sync from arrow position -> tab position.

STR:
1) open the hamburger menu
2) press the down arrow until the Zoom - is highlighted
3) Hit tab twice to hightlight the Zoom +
4) Hit spacebar/enter as much as you want
5) Observe that zoom is decreased.
6) Hit the down arrow once and the current zoom percentage is highlighted
7) Hit tab again to highlight the + again (tab position was reset on down arrow)

Comment 1

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1425972 - Manually handle Tab navigation in PanelMultiView so it syncs with Arrow navigation.

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

Comment 2

[Ian Moody [:Kwan] (UTC+1)]

So I had two thoughts about how to fix this, the other being listening for focus(in?) events on the menu items and updating the stored selected value, but this seems simpler (and easier).  Also seems to fix bug 1425981.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8937794 [details]
Bug 1425972 - Manually handle Tab navigation in PanelMultiView so it syncs with Arrow navigation.

https://reviewboard.mozilla.org/r/208496/#review214298

Code-wise this looks fine, but can you please also update the automated test to exercise this code, and perhaps test for the bug(s) that this fixes? https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/test/browser_panel_keyboard_navigation.js

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1425972 - Add a test for tab navigation in the browser panel.

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

Comment 5

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1425972 - Add a test for interleaved arrow and tab navigation in the browser panel.

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

Comment 6

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1425972 - Add a test for subview navigation on space keydown.

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

Comment 7

[Ian Moody [:Kwan] (UTC+1)]

So I've taken a first pass at the tests.  There might be some redundancy there, which is part of why they are currently three commits instead of one, for easy deleting.  Can fold before checking.

(A couple of the assertion messages in testUpDownKeys() seem wrong/misleading, and I initially puzzled at the lack of a looping ArrowUp check, but I guess testEnterKeyBehaviors() includes one)

So I think the reason for bug 1425981 is that commandDispatcher.focusedElement isn't the tabbed to element in that case (in fact I think it's no element, since arrows haven't been used).  And I'd guess it works on keyup because of some inherent XUL behaviour on the button.  Not sure why enter and space are different in that case, guess they have different behaviour on plain XUL buttons?
I also wonder if there are different platform conventions around control activation on keyup vs. keydown, and if Firefox is following them. (Certainly checkboxes in about:preferences behave differently to web <input type="checkbox">, at least on Linux, when using the spacebar)

Comment 8

[:Gijs (he/him)]

Comment on attachment 8937794 [details]
Bug 1425972 - Manually handle Tab navigation in PanelMultiView so it syncs with Arrow navigation.

https://reviewboard.mozilla.org/r/208496/#review214486

Comment 9

[:Gijs (he/him)]

Comment on attachment 8938045 [details]
Bug 1425972 - Add a test for interleaved arrow and tab navigation in the browser panel.

https://reviewboard.mozilla.org/r/208780/#review214488

Clever, I like it!

Comment 10

[:Gijs (he/him)]

Comment on attachment 8938046 [details]
Bug 1425972 - Add a test for subview navigation on space keydown.

https://reviewboard.mozilla.org/r/208782/#review214490

Comment 11

[:Gijs (he/him)]

Comment on attachment 8938044 [details]
Bug 1425972 - Add a test for tab navigation in the browser panel.

https://reviewboard.mozilla.org/r/208778/#review214492

Comment 12

[:Gijs (he/him)]

(In reply to Ian Moody [:Kwan] (UTC+0) from comment #7)
> So I think the reason for bug 1425981 is that
> commandDispatcher.focusedElement isn't the tabbed to element in that case
> (in fact I think it's no element, since arrows haven't been used).  And I'd
> guess it works on keyup because of some inherent XUL behaviour on the
> button.  Not sure why enter and space are different in that case, guess they
> have different behaviour on plain XUL buttons?

Yep. One thing to perhaps doublecheck is that if we do something explicitly on keydown, we don't do the same thing twice in cases where `keyup` would normally activate things?

(I don't know how you'd automated-test for that, our XUL keyup handling could very well do magic that naive event synthesis wouldn't trigger and that'll just make things a pain.)

> I also wonder if there are different platform conventions around control
> activation on keyup vs. keydown, and if Firefox is following them.
> (Certainly checkboxes in about:preferences behave differently to web <input
> type="checkbox">, at least on Linux, when using the spacebar)

If there is I'm not aware of it. I know that e.g. context menus appear on mousedown/mouseup on different platforms (cf. bug 1360278) but I dunno about keyboard stuff. It's also possible the HTML behaviour is either specced "wrong" compared to the OS, or that XUL is doing something "wrong" (because XUL).

I wouldn't worry too much about it here.

Comment 13

[Ian Moody [:Kwan] (UTC+1)]

(In reply to :Gijs from comment #12)
> (In reply to Ian Moody [:Kwan] (UTC+0) from comment #7)
> > So I think the reason for bug 1425981 is that
> > commandDispatcher.focusedElement isn't the tabbed to element in that case
> > (in fact I think it's no element, since arrows haven't been used).  And I'd
> > guess it works on keyup because of some inherent XUL behaviour on the
> > button.  Not sure why enter and space are different in that case, guess they
> > have different behaviour on plain XUL buttons?
> 
> Yep. One thing to perhaps doublecheck is that if we do something explicitly
> on keydown, we don't do the same thing twice in cases where `keyup` would
> normally activate things?

As far as I can tell it's good.  I think if we did, we'd already notice menu items doing things twice on keyboard activation.  And if I set it up so it breaks; after the stop() [1] and does nothing on keydown, then the keyup still doesn't activate the menu items. And the only reason space and enter worked at all before after tabbing was because it breaks before the stop() if there's no navMap.selected.
I guess this is because buttons seem to respond to keypress [2], and calling preventDefault() on keydown stops keypress ever firing (at least in HTML...).  But that doesn't explain how space waited for keyup before :S  Oh well.

[1] https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/browser/components/customizableui/PanelMultiView.jsm#1086
[2] https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/toolkit/content/widgets/button.xml#146

Comment 14

[Ian Moody [:Kwan] (UTC+1)]

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4a28bdc5337db8df4a97b1c0d55e862b1b005a3

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c19f8172cdf1
Manually handle Tab navigation in PanelMultiView so it syncs with Arrow navigation. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/55512efaf108
Add a test for tab navigation in the browser panel. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f3338a8c66a9
Add a test for interleaved arrow and tab navigation in the browser panel. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/17e993269858
Add a test for subview navigation on space keydown. r=Gijs

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c19f8172cdf1
https://hg.mozilla.org/mozilla-central/rev/55512efaf108
https://hg.mozilla.org/mozilla-central/rev/f3338a8c66a9
https://hg.mozilla.org/mozilla-central/rev/17e993269858