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

RESOLVED FIXED in Firefox 59

Status

()

defect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Kwan, Assigned: Kwan)

Tracking

({access})

Trunk
Firefox 59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [photon-structure])

Attachments

(4 attachments)

(Assignee)

Description

a year ago
+++ 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)
(Assignee)

Updated

a year ago
Iteration: 55.6 - May 29 → ---
No longer depends on: 1353360, 1354144, 1378016
Priority: P1 → --
(Assignee)

Updated

a year ago
See Also: → 1425981
(Assignee)

Updated

a year ago
(Assignee)

Comment 2

a year ago
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.
Assignee: nobody → moz-ian
Status: NEW → ASSIGNED

Comment 3

a year ago
mozreview-review
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
Attachment #8937794 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
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

a year ago
mozreview-review
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
Attachment #8937794 - Flags: review+

Comment 9

a year ago
mozreview-review
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!
Attachment #8938045 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 10

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

https://reviewboard.mozilla.org/r/208782/#review214490
Attachment #8938046 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 11

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

https://reviewboard.mozilla.org/r/208778/#review214492
Attachment #8938044 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 12

a year ago
(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.
(Assignee)

Comment 13

a year ago
(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
(Assignee)

Updated

a year ago
Duplicate of this bug: 1425981

Comment 16

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.