STR: 1. Turn on tracking protection. 2. Open a site with trackers such as twitter.com. 3. Press the View site information button to open the identity popup. 4. Use the tab key to cycle through the tab order. Expected: Tabbing should move to the "Show connection details" and "Disable protection for this site" buttons. Actual: It doesn't. this seems to be a regression after Firefox 60. Interestingly, I don't quite understand why it worked in 60. My understanding is that PanelMultiView handles keyboard navigation itself, and it only includes elements with a class of subviewbutton (and subviewkeynav after bug 1454866). I guess PanelMultiView didn't handle keyboard navigation for the top level popup then? It does seem to now, as evident in the fact that the "Clear Cookies and Site Data" button *is* tabbable (and it has a class of subviewkeynav). This should be easily fixed by adding the subviewkeynav class to any buttons in this popup.

Hey James, I tried to narrow down a regression range and here is what I found: Last good revision: 96a6ea5ea3468b4c9e20ff8d9795a7ef136213a9 First bad revision: 59118f646da40f20320aadf81f7e9cd4cd92d70f Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=96a6ea5ea3468b4c9e20ff8d9795a7ef136213a9&tochange=59118f646da40f20320aadf81f7e9cd4cd92d70f I also tested the "last good revision" build manually so I can be sure it was actually good.

Johann, looks like Bug 1462469 regressed this. Wanna take a look?

Yeah, weird that this ever worked. Thank Jamie for analyzing this. I can fix it.

(In reply to Johann Hofmann [:johannh] from comment #3) > I can fix it. Famous last words. So the reason that this worked (I implemented it) is that we have code that advances the identity popup focus when it is opened via keyboard, "manually" so to say: https://searchfox.org/mozilla-central/rev/6a27ff48dda8376d1f819c534510eca7d9af5818/browser/base/content/browser-siteIdentity.js#860 This was always a hack to take care of what should be done by a11y APIs, and it has a test: https://searchfox.org/mozilla-central/rev/6a27ff48dda8376d1f819c534510eca7d9af5818/browser/base/content/test/siteIdentity/browser_identityPopup_focus.js#15 Now you might ask "Why isn't this test failing?" and I can answer I have no freaking clue. There's something very strange going on, where the focus is only successfully moved when in a mochitest environment. When I open the identity popup using the keyboard on a local build, it does not focus the expander button. However, when I do ./mach mochitest --no-autorun (that is, without even starting the test) and repeat the exact same steps IT JUST MAGICALLY WORKS. I'm more than a little confused. Is there any major difference between a11y/focus behavior in mochitests vs. regular profiles?

Comment on attachment 8985843 [details] Bug 1467385 - Use PanelMultiView APIs for keyboard navigation in the identity popup. https://reviewboard.mozilla.org/r/251324/#review257910 It looks like this is fixing several issues, so the change can be split into multiple changesets. I'm not quite sure why PanelMultiView doesn't use the platform focus advancing code but reimplements it own version, however, given that it does, it makes sense to use this system here. ::: browser/base/content/browser-siteIdentity.js:264 (Diff revision 1) > displaySecurityInfo(); > event.stopPropagation(); > PanelMultiView.hidePopup(this._identityPopup); > }, > > - showSecuritySubView() { > + async showSecuritySubView(event) { This is irrelevant given the comment below, but note that async functions shouldn't be used as DOM event handlers unless they catch and report all exceptions. ::: browser/base/content/browser-siteIdentity.js:268 (Diff revision 1) > - // Elements of hidden views have -moz-user-focus:ignore but setting that > - // per CSS selector doesn't blur a focused element in those hidden views. > - Services.focus.clearFocus(window); > + // Only focus the back button when we didn't use a mouse to open the subview, > + // to serve a11y needs but prevent flickering when the user moves their mouse. > + if (event.inputSource != MouseEvent.MOZ_SOURCE_MOUSE) { > + PanelView.forNode(this._identityPopupSecurityView).focusFirstNavigableElement(); > + } We don't focus any element in the subviews in the main menu, so I'd say we should continue not to focus the back button here. Anyways, this would go in a ViewShown handler, we wouldn't add a return parameter to showSubView. ::: browser/base/content/browser-siteIdentity.js:858 (Diff revision 1) > if (this._popupTriggeredByKeyboard) { > // Move focus to the next available element in the identity popup. > // This is required by role=alertdialog and fixes an issue where > // an already open panel would steal focus from the identity popup. > - document.commandDispatcher.advanceFocusIntoSubtree(this._identityPopup); > + PanelView.forNode(this._identityPopupMainView).focusFirstNavigableElement(); > } This should actually be moved to a ViewShown handler, to make sure the view is "active" before setting focus: https://dxr.mozilla.org/mozilla-central/rev/75a32b57132f8cba42779555662a057a0416a313/browser/components/customizableui/PanelMultiView.jsm#58-72 ::: browser/components/controlcenter/content/panel.inc.xul:50 (Diff revision 1) > when-ciphers="weak">&identity.weakEncryption;</description> > <description when-loginforms="insecure">&identity.insecureLoginForms2;</description> > </vbox> > </vbox> > <button id="identity-popup-security-expander" > - class="identity-popup-expander" > + class="identity-popup-expander subviewkeynav" We should actually remove support for the "subviewkeynav" class and just include all buttons, toolbarbuttons, and menuitems in the navigable elements. We can add a "nokeynav" attribute if needed, although I can't think of any case right now where we want focus but only when using the mouse. Given the chance for regressions with the change above, however, I'm fine with landing these added "subviewkeynav" classes for now, but please have a bug on file and a patch ready to remove them, that we can then land after the merge next week. ::: browser/components/customizableui/PanelMultiView.jsm:1395 (Diff revision 1) > /** > + * Focuses and moves keyboard selection to the first navigable element as > + * determined by getNavigableElements(). > + */ > + focusFirstNavigableElement() { > + this.selectedElement = this.getNavigableElements()[0]; > + this.focusSelectedElement(); > + } So, unfortunately the PanelMultiView code is still not ideal here. It appears that the "keyNavigation" method does some initialization on the buttons before starting key navigation, in particular by setting tabindex, that we likely need here too. We probably need to refactor that part into a "resettable" lazy getter for "this.buttons", containing the current implementation of getNavigableElements. The latter method is only ever used in regression tests, so we can also consider reimplementing it there and incorporating the code in the "this.buttons" getter. I'd specify in the comment that this does nothing if there are no navigable elements at the moment. I'd also add a unit test for this method because, even if this works correctly now, it's easy to miss in a refactoring.

(In reply to Johann Hofmann [:johannh] from comment #4) > So the reason that this worked (I implemented it) is that > we have code that advances the identity popup focus when it is opened via > keyboard, "manually" so to say: Sure. That broke in bug 1466998 and seems to be something to do with shadow DOM. However, that doesn't explain why the tab key stopped working too. It's like we didn't end up using the PanelMultiView keyboard navigation code in 60, but instead used normal focus handling. Now that programmatic focus advance is broken, we *do* use the PanelMultiView code (and thus we don't hit these buttons). That doesn't make sense to me. > This was always a hack to take care of what should be done by a11y APIs I still disagree with this. A11y APIs have nothing to do with this; they should never mess with focus unless a user requests setting focus via some means other than the keyboard. This is about keyboard navigation. Focus just shouldn't end up in limbo, especially if the user is interacting using the keyboard. > Now you might ask "Why isn't this test failing?" and I can answer I have no > freaking clue. Yeah, that's the unanswered question in bug 1466998. :(

(In reply to James Teh [:Jamie] from comment #8) > It's > like we didn't end up using the PanelMultiView keyboard navigation code in > 60, but instead used normal focus handling. Now that programmatic focus > advance is broken, we *do* use the PanelMultiView code (and thus we don't > hit these buttons). That doesn't make sense to me. Now that bug 1466998 is fixed, focus hits the Show connection details button as expected when you press space to show the pop-up. However, tabbing around still misses these buttons as described in comment 0. So, we still use the PanelMultiView key nav code now where we didn't in 60.

What do you think about Comment #9, Johann?

Comment on attachment 8985843 [details] Bug 1467385 - Use PanelMultiView APIs for keyboard navigation in the identity popup. https://reviewboard.mozilla.org/r/251324/#review257910 > This is irrelevant given the comment below, but note that async functions shouldn't be used as DOM event handlers unless they catch and report all exceptions. This isn't a DOM event handler, it's called inside one, no? :) > We should actually remove support for the "subviewkeynav" class and just include all buttons, toolbarbuttons, and menuitems in the navigable elements. We can add a "nokeynav" attribute if needed, although I can't think of any case right now where we want focus but only when using the mouse. > > Given the chance for regressions with the change above, however, I'm fine with landing these added "subviewkeynav" classes for now, but please have a bug on file and a patch ready to remove them, that we can then land after the merge next week. Ok, fair enough, I'd still like to do this in a follow up bug to be able to uplift this one. > So, unfortunately the PanelMultiView code is still not ideal here. It appears that the "keyNavigation" method does some initialization on the buttons before starting key navigation, in particular by setting tabindex, that we likely need here too. > > We probably need to refactor that part into a "resettable" lazy getter for "this.buttons", containing the current implementation of getNavigableElements. The latter method is only ever used in regression tests, so we can also consider reimplementing it there and incorporating the code in the "this.buttons" getter. > > I'd specify in the comment that this does nothing if there are no navigable elements at the moment. I'd also add a unit test for this method because, even if this works correctly now, it's easy to miss in a refactoring. I don't understand this. Why do we need to do this? Why is having the tabindex attribute set on the elements required for what we're trying to achieve (just focusing the first navigable element)?

NI for the last question above. Thanks! :)

Comment on attachment 8985843 [details] Bug 1467385 - Use PanelMultiView APIs for keyboard navigation in the identity popup. https://reviewboard.mozilla.org/r/251324/#review257910 > We don't focus any element in the subviews in the main menu, so I'd say we should continue not to focus the back button here. > > Anyways, this would go in a ViewShown handler, we wouldn't add a return parameter to showSubView. > We don't focus any element in the subviews in the main menu, so I'd say we should continue not to focus the back button here. This is fixing bug 1454865 but since you have reservations I'm happy to just remove it from this patch and make a new patch in the other bug.

(In reply to Johann Hofmann [:johannh] from comment #11) > > We should actually remove support for the "subviewkeynav" class and just include all buttons, toolbarbuttons, and menuitems in the navigable elements. We can add a "nokeynav" attribute if needed, although I can't think of any case right now where we want focus but only when using the mouse. I didn't do this originally because I wasn't sure whether there might be other arbitrary controls outside of that set that needed to be supported. However, that would certainly be better and makes it far easier for people to get keyboard navigation "for free" in future. Note that I've just noticed that the permission clear buttons aren't tabbable, so this change would fix that too. I'll file a bug for it anyway so we don't lose it.

(In reply to James Teh [:Jamie] from comment #15) > Note that I've just noticed > that the permission clear buttons aren't tabbable, so this change would fix > that too. I'll file a bug for it anyway so we don't lose it. Filed bug 1473199.

(In reply to Johann Hofmann [:johannh] from comment #11) > This isn't a DOM event handler, it's called inside one, no? :) Mostly an academic distinction, but certainly this lets you do oncommand="gIdentityHandler.showSecuritySubView().catch(Cu.reportError);", assuming "Cu" is available in that context. > I don't understand this. Why do we need to do this? Why is having the > tabindex attribute set on the elements required for what we're trying to > achieve (just focusing the first navigable element)? I may be wrong as I haven't tested it, but I think this makes some navigable elements focusable in the first place? Otherwise, I see no reason for that code to exist in the first place. This might be required only for certain elements, so even though the new API happens to work here, it may not work generically for all consumers of PanelMultiView, leading to subtle bugs when the function is called elsewhere. Anyways, given the need to uplift this, I'd be fine with landing what we have in this bug, but please file a bug to figure out the above and add a regression test. We may end up simplifying the code a bit, which is good.

Comment on attachment 8985843 [details] Bug 1467385 - Use PanelMultiView APIs for keyboard navigation in the identity popup. https://reviewboard.mozilla.org/r/251324/#review261682 ::: browser/components/customizableui/PanelMultiView.jsm:1366 (Diff revision 2) > * Retrieves the button elements that can be used for navigation using the > * keyboard, that is all enabled buttons including the back button if visible. > * > * @return {Array} > */ > getNavigableElements() { nit: can you rename this to _getNavigableElements, here and in the test? This makes it clear that it shouldn't be called from other modules. As mentioned, we may remove it later. ::: browser/components/customizableui/PanelMultiView.jsm:1396 (Diff revision 2) > + * Focuses and moves keyboard selection to the first navigable element as > + * determined by getNavigableElements(). Add the note about this doing nothing when there are no navigable elements, and remove "as determined by getNavigableElements" because it's an implementation detail.

(In reply to James Teh [:Jamie] from comment #15) > Note that I've just noticed that the permission clear buttons aren't tabbable Johann, this can also be addressed in this bug using the class if we plan to uplift.

(In reply to :Paolo Amadini from comment #19) > (In reply to James Teh [:Jamie] from comment #15) > > Note that I've just noticed that the permission clear buttons aren't tabbable > > Johann, this can also be addressed in this bug using the class if we plan to > uplift. Nevermind... just saw the patch in the other bug, which was originally filed for the generic approach. Please file a new bug for the latter!

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8a5c225c9937 Use PanelMultiView APIs for keyboard navigation in the identity popup. r=Paolo

Can you request uplift? From Jamie's comments in bug 1473199 sounds like we need this in beta 62. Thanks!

Comment on attachment 8985843 [details] Bug 1467385 - Use PanelMultiView APIs for keyboard navigation in the identity popup. Approval Request Comment [Feature/Bug causing the regression]: Bug 1462469, sort of [User impact if declined]: Not all elements in the identity popup are keyboard accessible [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No, I think a11y folks can verify this once it hits beta [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Medium to Low [Why is the change risky/not risky?]: This changes a couple of things in the identity popup and the more general code for all panels, but the area is well covered in tests and we seem to have done a decent amount of manual testing here as well. [String changes made/needed]: None

Comment on attachment 8985843 [details] Bug 1467385 - Use PanelMultiView APIs for keyboard navigation in the identity popup. Recent regression, should be verified in beta once it lands for beta 8.