Closed Bug 1436086 Opened 7 years ago Closed 6 years ago

Make all groups of buttons within our toolbars keyboard accessible and reachable via tab and shift+tab.

Categories

(Firefox :: Toolbars and Customization, enhancement, P3)

Product:
Firefox
Component:
Toolbars and Customization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: MarcoZ, Assigned: Jamie)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 1 obsolete file)

Toolbar keyboard navigation POC (r2)
(deleted), patch
yzen
: feedback+
Details | Diff | Splinter Review
Bug 1436086: Implement keyboard navigation for the main and Bookmarks toolbars.
(deleted), text/x-phabricator-request
Details
As discussed in bug 1418973, our toolbars play a much more important role nowadays, and since the Quantum release, are now under our full control. So we can make them consistently accessible via the keyboard. Each group of buttons, starting with 1 button, must be accessible via the tab key. This is for the groups of buttons to the left and right of the awesome bar in the main toolbar, the bookmarks toolbar, and others. This also applies to the groups of buttons to the left and right of the actual address field. Within each group: a) Have one tab stop per group only. b) Make other buttons within that group reachable via left and right arrow keys. Within a toolbar, this is standard keyboard behavior in many applications across platforms. c) At either end of the group, wrap around to the other end. d) Enter or Space activate the currently focused button. Things that provide a border to the group are either the beginning or end of the tool bar, or fields such as the address field, search box (if enabled) or other text fields that cause arrow keys to be required to be used for editing/reviewing text. In essence, if focused on the Site Info and other buttons to the left of the address field, the arrow keys should not move into the address field. Instead, pressing tab should. Also: Add tests that make sure future patches don't break this keyboard model.
Blocks: 1436089
Blocks: 1436090
Component: Keyboard Navigation → Toolbars and Customization
Priority: -- → P3
Assignee: nobody → jteh
Attached patch Toolbar keyboard navigation POC (obsolete) (deleted) — — Splinter Review
This proof of concept patch implements the toolbar keyboard navigation proposed in this bug. There are some rough edges and the code certainly needs some polish (if not total refactoring), but it does work for the most part. Known bugs: 1. With the Bookmarks toolbar visible, pressing space/enter on the buttons does nothing. element.click() doesn't seem to work on these, nor does doCommand or dispatching mousedown/up events. 2. When the Bookmarks toolbar is hidden, its buttons are still in the tab order, since they use CSS visibility. I need to work out how to handle this case. I can probably do something with toolbarvisibilitychange. 3. Contrary to the recommendation in bug 1418973 comment 4, the buttons on the left and right of the address bar input are at the same tab stop. I don't really know how to fix this, since the input itself is inside an XBL binding and I can't get its effective position in the document from the DOM. 4. This breaks focus when pressing space/enter on the Show site information button, since it overrides the special keyPress handler on that button. 5. I didn't consider/test RTL, so probably need to fix some things. Difference from proposal: The arrow keys do not wrap when they hit the edge. I'm honestly not sure if we want to do this. On one hand, the arrow keys do wrap in menu bars and some tool bars elsewhere. On the other hand, they don't wrap in our Dev Tools toolbar, nor do they wrap on the tab strip. I'd personally prefer they didn't wrap, but the proposal suggested they should and I didn't think to question this at the time. Implementation questions: 1. Is CustomizableUI the right place to put this? If not, where? 2. I had to delay the tab order building to get the Bookmarks toolbar to work. It seems that toolbar gets re-populated when it is shown, but I don't know how to detect this in a sane way. Also, I need to wait until the toolbar is rendered when a new window opens. The load event seems to solve that problem, but is that acceptable? 3. Currently, the tab order is rebuilt from scratch whenever a widget gets added, removed or moved. I can probably fixed the remove case - I already have some code for that - but handling additions/moves is a lot harder. Is this a serious issue? 4. I have to use a MutationObserver to watch for changes to the disabled and showing attributes in order to detect disabling of the Back button, notification anchors appearing/disappearing, etc. Is there a better way to do this? I don't think it's ideal to have to call this code from every place where one of these things could change, but I don't see a way to globally watch for such changes to toolbar controls.
Attachment #8972780 - Flags: feedback?(yzenevich)
This fixes the bug where the Bookmarks toolbar buttons remain tabbable when the Bookmarks toolbar is hidden. It also gets rid of the ugly unreliable init delay. To achieve this, the MutationObserver is now used for all mutations; we no longer use onWidgetAfterDOMChange as fired by CustomizableUI. This is necessary because the Bookmarks toolbar doesn't use the CustomizableUI framework to add/remove bookmarks; it manages the DOM nodes directly. The added benefit is that all mutations are handled in a unified way. I've just realised there's another bug: the "Show site information" button (identity-icon) is tabbable even for a page like about:blank, where it isn't visible. I'm not quite sure how this works. I can see CSS which sets display: none for identity-icon-labels when urlbar[pageproxystate=invalid], but not for identity-icon. Still, somehow, identity-icon gets visually hidden, even though it still has clientWidth > 0. In any case, the only way I can see to fix this is to special case this by watching the pageproxystate attribute on #urlbar.
Attachment #8973114 - Flags: feedback?(yzenevich)
Attachment #8972780 - Attachment is obsolete: true
Attachment #8972780 - Flags: feedback?(yzenevich)
(In reply to James Teh [:Jamie] from comment #1) > Difference from proposal: > The arrow keys do not wrap when they hit the edge. I'm honestly not sure if > we want to do this. On one hand, the arrow keys do wrap in menu bars and > some tool bars elsewhere. On the other hand, they don't wrap in our Dev > Tools toolbar, nor do they wrap on the tab strip. I'd personally prefer they > didn't wrap, but the proposal suggested they should and I didn't think to > question this at the time. That's fine. We should be consistent with ourselves, so not wrapping is totally OK, too.
(In reply to James Teh [:Jamie] from comment #3) > Created attachment 8973114 [details] [diff] [review] > Toolbar keyboard navigation POC (r2) > > This fixes the bug where the Bookmarks toolbar buttons remain tabbable when > the Bookmarks toolbar is hidden. It also gets rid of the ugly unreliable > init delay. > To achieve this, the MutationObserver is now used for all mutations; we no > longer use onWidgetAfterDOMChange as fired by CustomizableUI. This is > necessary because the Bookmarks toolbar doesn't use the CustomizableUI > framework to add/remove bookmarks; it manages the DOM nodes directly. The > added benefit is that all mutations are handled in a unified way. > > I've just realised there's another bug: the "Show site information" button > (identity-icon) is tabbable even for a page like about:blank, where it isn't > visible. I'm not quite sure how this works. I can see CSS which sets > display: none for identity-icon-labels when urlbar[pageproxystate=invalid], > but not for identity-icon. Still, somehow, identity-icon gets visually > hidden, even though it still has clientWidth > 0. In any case, the only way > I can see to fix this is to special case this by watching the pageproxystate > attribute on #urlbar. I've tried the patch, I see some visual differences or missing focus styling (without the screen reader), will try with in the next couple of days.
Still looking at the patch, it seems like it is reasonable to have this placed in CustomizableUI, however a Firefox front end peer would give you a definitive answer. Regarding XUL controls with tabindex=-1 not working, have you figured out why that is the case? I wonder if it's worth more investigation, it would simplify the whole setTabOrder logic, iiic. I like the catch all of mutation observer, I wonder what Firefox front end folks think about this approach. Another one (which is probably something you thought of) would've been identifying all relevant events that CustomizeUI fires and calling something like ToolbarKeyboardNavigator.setTabOrderIfNecessary to update it. Here's a quick fix of the focus styling for toolbarbuttons that folks who are trying the patch out might find useful - https://pastebin.mozilla.org/9084896
(In reply to Yura Zenevich [:yzen] from comment #6) > Regarding XUL controls with tabindex=-1 not working, have you figured out > why that is the case? It seems to just be "by design" for XUL: > • the value of -moz-user-focus is overriden for HTML elements if they have a tabindex attribute > • the value of -moz-user-focus is overriden for an enabled XUL element that implements nsIDOMXULControlElement if its tabIndex property returns a value greater or equal to zero. ( https://wiki.mozilla.org/XUL:Focus_Behaviour ) To make matters worse, we can't even just set tabindex to 0 because some of the elements we need to make focusable don't implement nsIDOMXULControlElement; e.g. the notification anchors are <image> elements with role="button". From that same page: > The following XUL elements implement nsIDOMXULControlElement: > button, caption, checkbox, colorpicker, description, label, listbox, listitem, menu, menuitem, menulist, radio, radiogroup, richlistbox, richlistitem, tab, tabs, textbox, tree That's why I have to mess with -moz-user-focus. I should probably comment that. :) > I wonder if it's worth more investigation, it would > simplify the whole setTabOrder logic, iiic. I don't think it'd simplify setTaborder. It'd just allow those controls to take focus when focus is set programmatically; e.g. via screen reader review commands. You could also just call .focus() for left/right arrow navigation instead of resetting tabindex, but then the user's last position wouldn't be "remembered" when you tab or shift+tab away, and that is potentially quite a nice thing to have. > I like the catch all of mutation observer, I wonder what Firefox front end > folks think about this approach. Another one (which is probably something > you thought of) would've been identifying all relevant events that > CustomizeUI fires and calling something like > ToolbarKeyboardNavigator.setTabOrderIfNecessary to update it. That was my original implementation. The problem is that CustomizableUI doesn't know about some of the changes we need to care about. Notably, it doesn't know when controls such as the Back and Forward buttons get disabled, nor does it know anything about bookmarks on the Bookmarks toolbar. > Here's a quick fix of the focus styling for toolbarbuttons that folks who > are trying the patch out might find useful - > https://pastebin.mozilla.org/9084896 I guess I'm going to need something like this in the final patch? Should I just pull in your changes for now? I have no clue how this works visually. :)
(In reply to James Teh [:Jamie] from comment #7) > (In reply to Yura Zenevich [:yzen] from comment #6) > > Regarding XUL controls with tabindex=-1 not working, have you figured out > > why that is the case? > > It seems to just be "by design" for XUL: > > > • the value of -moz-user-focus is overriden for HTML elements if they have a tabindex attribute > > • the value of -moz-user-focus is overriden for an enabled XUL element that implements nsIDOMXULControlElement if its tabIndex property returns a value greater or equal to zero. > ( https://wiki.mozilla.org/XUL:Focus_Behaviour ) Urgh, yeah I remember now. > > To make matters worse, we can't even just set tabindex to 0 because some of > the elements we need to make focusable don't implement > nsIDOMXULControlElement; e.g. the notification anchors are <image> elements > with role="button". From that same page: > > > The following XUL elements implement nsIDOMXULControlElement: > > button, caption, checkbox, colorpicker, description, label, listbox, listitem, menu, menuitem, menulist, radio, radiogroup, richlistbox, richlistitem, tab, tabs, textbox, tree > > That's why I have to mess with -moz-user-focus. I should probably comment > that. :) > > > I wonder if it's worth more investigation, it would > > simplify the whole setTabOrder logic, iiic. > > I don't think it'd simplify setTaborder. It'd just allow those controls to > take focus when focus is set programmatically; e.g. via screen reader review > commands. You could also just call .focus() for left/right arrow navigation > instead of resetting tabindex, but then the user's last position wouldn't be > "remembered" when you tab or shift+tab away, and that is potentially quite a > nice thing to have. Yes, it looks like navigation around the URL bar is suffering from that. > > > I like the catch all of mutation observer, I wonder what Firefox front end > > folks think about this approach. Another one (which is probably something > > you thought of) would've been identifying all relevant events that > > CustomizeUI fires and calling something like > > ToolbarKeyboardNavigator.setTabOrderIfNecessary to update it. > > That was my original implementation. The problem is that CustomizableUI > doesn't know about some of the changes we need to care about. Notably, it > doesn't know when controls such as the Back and Forward buttons get > disabled, nor does it know anything about bookmarks on the Bookmarks toolbar. Gotcha, it makes sense then to ask for an early feedback about going this route from a front end peer. > > > Here's a quick fix of the focus styling for toolbarbuttons that folks who > > are trying the patch out might find useful - > > https://pastebin.mozilla.org/9084896 > > I guess I'm going to need something like this in the final patch? Should I > just pull in your changes for now? I have no clue how this works visually. :) Yes, however this was a WIP, there are other things that are broken (for example controls around the url bar).
(In reply to Yura Zenevich [:yzen] from comment #8) > > You could also just call .focus() for left/right arrow navigation > > instead of resetting tabindex, but then the user's last position wouldn't be > > "remembered" when you tab or shift+tab away, and that is potentially quite a > > nice thing to have. > Yes, it looks like navigation around the URL bar is suffering from that. Suffering from what? Do you mean the user's position (as navigated with left/right arrows) isn't remembered? Can you provide steps to repro? Note that when one of those buttons is added or removed (e.g. when changing URLs), the position will be lost. This is because we have to rebuild the tab order when such a change occurs. I guess I could try to come up with logic to remember it in that case. Is that what you're referring to?
(In reply to James Teh [:Jamie] from comment #9) > (In reply to Yura Zenevich [:yzen] from comment #8) > > > You could also just call .focus() for left/right arrow navigation > > > instead of resetting tabindex, but then the user's last position wouldn't be > > > "remembered" when you tab or shift+tab away, and that is potentially quite a > > > nice thing to have. > > Yes, it looks like navigation around the URL bar is suffering from that. > > Suffering from what? Do you mean the user's position (as navigated with > left/right arrows) isn't remembered? Can you provide steps to repro? > > Note that when one of those buttons is added or removed (e.g. when changing > URLs), the position will be lost. This is because we have to rebuild the tab > order when such a change occurs. I guess I could try to come up with logic > to remember it in that case. Is that what you're referring to? Here are some of the issues that I've noticed: - When keyboard is focused on the urlbar, there is a "Show history" button that is only shown when url bar is in focus. It is not reachable with the keyboard however, pressing down arrow will trigger the same behaviour (e.g. open the dropdown) - One confusing bit I see, though it is the right behaviour imo, is when the focus was left on the identity-box (which visually comes before the url-bar textarea) and we tab away from the url-bar and then come back the tab order changes FROM out-of-url-bar element -> url-bar text area -> url-bar buttons TO out-of-url-bar element -> url-bar buttons -> url-bar text area. I guess that happens because of tab order being re-set? It almost feels like it would be more consistent with just tabbable controls that are part of url-bar.
(In reply to Yura Zenevich [:yzen] from comment #10) > - When keyboard is focused on the urlbar, there is a "Show history" button > that is only shown when url bar is in focus. It is not reachable with the > keyboard however, pressing down arrow will trigger the same behaviour (e.g. > open the dropdown) > - One confusing bit I see, though it is the right behaviour imo, is when the > focus was left on the identity-box (which visually comes before the url-bar > textarea) and we tab away from the url-bar and then come back the tab order > changes FROM out-of-url-bar element -> url-bar text area -> url-bar buttons > TO out-of-url-bar element -> url-bar buttons -> url-bar text area. Both of these issues are difficult to solve because some of the controls in the URL bar, including the actual <input> and the "Show history" button, are part of an XBL binding and are thus not exposed as part of the DOM from the toolbar's perspective. To make matters worse, the binding doesn't just add children on one side of the DOM children; it adds children on either side. I can use document.getAnonymousNodes to get the binding children, but that doesn't tell me where they land with respect to the DOM children. I'm not too worried about the Show history button; as you say, that's already keyboard accessible in another way. However, the tab order changing is kinda weird. In that case, what happens is that some of the buttons are before the <input> and some of them are after it, so where the <input> lands in the tab order depends on which side we're on. > It almost feels like it > would be more consistent with just tabbable controls that are part of > url-bar. It would be more consistent, yes, but IMO a far worse keyboard UX, since we'd end up with a huge number of tab stops.
Comment on attachment 8973114 [details] [diff] [review] Toolbar keyboard navigation POC (r2) Review of attachment 8973114 [details] [diff] [review]: ----------------------------------------------------------------- I think overall, it looks good. Some questions suggestions: There is no reason this new component needs to live in CustomizableUI.jsm, I think it might be worthwhile putting it in it's own jsm file and then loading/initializing it along with customizableUI. You should be able to load CustomizableUI object in the ToolbarKeyboardNavigator module to add/remove listeners and for other static things. I think focus styling should be a separate bug/patch and I am happy to pick it up. My changes that I pasted earlier only fix toolbars, url-bar styling is still missing and I would want to do quite a bit more testing before landing. Oh one thing I noticed (which could be a regression or exposed existing bug), STR: * Navigate (tab to) the toolbar that has hamburger main menu (PanelUI-menu-button) * Navigate (arrow) to the hamburger main menu button * Press tab to move forward (in most cases content frame) Expected: Focus switches to content frame, nothing gets activated Observed: Focus switches to content frame, main menu is activated and opened when navigating the toolbar that has the hamburger main menu (PanelUI-menu-button) and arrowing to that menu (e.g. focusing on it)
Attachment #8973114 - Flags: feedback?(yzenevich) → feedback+
urgh ignore the last bit i did not delete ("when navigating the toolbar that has the hamburger main menu (PanelUI-menu-button) and arrowing to that menu (e.g. focusing on it")
Depends on: 1506510
I spoke to Gijs at length about this, first at the SF All Hands and then a few days ago on IRC. He is very concerned about performance with regard to the MutationObserver. Even if we had some other unified way to reliably detect changes, constantly rebuilding the tab order is a performance concern itself, as well as being hideously complicated. The new plan is to add "fake", invisible tab stops to the toolbar for each "group" of buttons. The focus handler for these tab stops will bounce the focus to the appropriate button. Bouncing the focus like this is perhaps a little quirky, but I think it's far less ugly than the alternative. As a bonus, the code is *much* simpler. I have a working patch for this now, but I still need to write tests.
Try run for all latest work (including patches needed from other bugs for full functionality): https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9899cf5c0b69c2bb45f740a81411bbfc31e6ee9
Depends on: 1506787
Blocks: 1515543
Having separate tab stops for every toolbar control results in an unmanageable number of tab stops. Therefore, we group several buttons under a single tab stop and allow movement between them using left/right arrows. However, text inputs use the arrow keys for their own purposes, so they need their own tab stop. There are also groups of buttons before and after the URL bar input which should get their own tab stop. The subsequent buttons on the toolbar are then another tab stop after that. Tab stops for groups of buttons are set using the <toolbartabstop/> element. This element is invisible, but gets included in the tab order. When one of these gets focus, it redirects focus to the appropriate button. This avoids the need to continually manage the tabindex of toolbar buttons in response to toolbarchanges. Navigation to for the View site information button and notification anchors is now managed by this new framework. As such, they no longer need their own position in the tab order and the CSS has been tweaked accordingly.
Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a14a11bf2d6f Implement keyboard navigation for the main and Bookmarks toolbars. r=Gijs
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d16f7174737e Backed out changeset a14a11bf2d6f for eslint failure on browser_toolbarButtonKeyPress.js. CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=a14a11bf2d6f834dbf2ebcb45e9d1d61e1af3aaf&selectedJob=228146846

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228146846&repo=autoland&lineNumber=284

Bakout link: https://hg.mozilla.org/integration/autoland/rev/d16f7174737e

[task 2019-02-13T13:26:33.521Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2019-02-13T13:26:33.521Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2019-02-13T13:26:33.521Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2019-02-13T13:26:33.521Z]
[task 2019-02-13T13:26:33.521Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2019-02-13T13:32:20.616Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js:196:7 | 'sidebar' is not defined. (no-undef)
[taskcluster 2019-02-13 13:32:20.977Z] === Task Finished ===
[taskcluster 2019-02-13 13:32:20.978Z] Unsuccessful task run with exit code: 1 completed in 651.934 seconds

Flags: needinfo?(jteh)
Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/76e2c2df0ce0 Implement keyboard navigation for the main and Bookmarks toolbars. r=Gijs

Ug. Sorry. I don't know how I missed that. Fixed and re-landed.

Flags: needinfo?(jteh)

https://hg.mozilla.org/mozilla-central/rev/76e2c2df0ce0

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Depends on: 1527922
Depends on: 1528090
Depends on: 1519226
Blocks: 287743
Depends on: 1536521
No longer depends on: 1537706

While this was designed to be as intuitive as possible, because we don't just shove all of the toolbar buttons in the tab order, it'd be good to document the keyboard navigation user experience for toolbars, probably on SuMo. This could either be done in the existing keyboard shortcuts article or a new article (perhaps linked from the existing one).

Joni, could you please advise as to the process for getting this documented and your thoughts on where it should be placed? I guess we should also get this into release notes as well, probably with a link to the newly written content.

Flags: needinfo?(jsavage)

Hi Jamie, we can help you get this documented. I suggest we have a draft finalize 3 weeks before the change goes to GA. Do you already have a draft or notes in a Google Doc that you can share with me? It doesn't have to be polished as I can edit it from there.

Flags: needinfo?(jsavage) → needinfo?(jteh)

Here's a Google Doc with a short few paragraphs I just put together:
https://docs.google.com/document/d/1HLqty6fXPGzPOplZI7SEQ4k8o8MNkczfmoj8eFElZ64/edit?usp=sharing

Note that there is some existing content in the Firefox Keyboard Shortcuts article which might need to be adjusted. In particular:

Some of these shortcuts require the currently selected tab to be "in focus." Currently, the only way to do this is to select an adjacent object and "tab into" the current tab, for instance, by hitting Alt + D to select the address bar, and then Shift + Tab twice.

"Shift + Tab twice" is no longer correct. This will normally be "Shift + Tab thrice", but could sometimes be twice or more than thrice depending on toolbar customisation. (In fact, the existing advice has never been entirely correct. For example, in a blank tab in Firefox 66, you only shift+tab once to access the browser tabs.) I would suggest we instead change this to say that you press Shift + Tab until you reach the browser tab bar or similar.

Flags: needinfo?(jteh)

I've edited the existing Firefox Keyboard shortcuts article to change the part about shift + tab twice/thrice/once and used your suggested to press it until you reach the browser tab bar. Thanks!

I've also edited the Google Doc. Please take a look and let me know if it's still correct/good to publish.

No longer depends on: 1536521
Regressions: 1536521
Blocks: 1545382
Regressions: 1554485
Blocks: 1568749
No longer blocks: 1568749
Regressions: 1602870
Regressions: 1608554
Blocks: 1775129
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: