As mentioned by Gijs in bug 1354144, comment 7: "[...] if you go back from a subview to mainview, the selected item in there is lost. I think we should forget 'forward' selected items (so if you re-enter the same subview, you should start at the top again) but not the selected item in the 'previous' view." This should make list navigation/ traversal through various subviews more efficient.

To be clear, I switched to keeping a ref rather than an index because the index was stale after going: open hamburger open library navigate in library go back This seems to be because we flip the enabled/disabled state of the cut/copy/paste buttons in some situations. Once the index is stale, there's no recovery possible, because we have no idea how many buttons were enabled and now aren't (or got removed or whatever). So I switched to just keeping a ref, and if the originally-referenced button is no longer enabled/visible, finding an adjacent focusable button by iterating over *all* the toolbarbuttons in the panelview. Hopefully the patch is reasonably clear, let me know if you have other questions. :-)

Comment on attachment 8897416 [details] Bug 1366207 - remember previous view's selection when keyboard navigating panels, https://reviewboard.mozilla.org/r/168730/#review174074 Cool stuff! The small refactor was kinda scary to me at first, but I quickly realized it's not. Thanks for taking this on! I think it's almost there, but there are enough things to update to warrant another look. ::: browser/components/customizableui/PanelMultiView.jsm:274 (Diff revision 2) > }); > > this._panel.addEventListener("popupshowing", this); > this._panel.addEventListener("popuphidden", this); > this._panel.addEventListener("popupshown", this); > + this._panel.addEventListener("ViewShown", this); I'd rather you just added `this._updateKeyboardFocus();` near line 677 (for one, because 'ViewShown' is also emitted for non-photon panels). ::: browser/components/customizableui/PanelMultiView.jsm:1000 (Diff revision 2) > + } > + > + /** > + * Based on going up or down, select the previous or next focusable button > + * in the current view. > + * @param {Object} navMap the navigation keyboard map object for the view nit: missing newline between description and params. ::: browser/components/customizableui/PanelMultiView.jsm:1025 (Diff revision 2) > + buttonIndex = maxIdx; > + newButton = buttons[buttonIndex]; > + } else { > + // The previously selected item is no longer selectable. Find the next item: > + let allButtons = lastSelected.closest("panelview").querySelectorAll("toolbarbutton"); > + let maxAllButtonIdx = allButtons.length; Technically, this should be `let maxAllButtonIdx = allButtons.length - 1;`. Means that you'll need to change the operator in the loop below from `<` to `<=` as well. ::: browser/components/customizableui/PanelMultiView.jsm:1151 (Diff revision 2) > * @param {panelview} view View to reset the key navigation attributes of. > - * Defaults to `this._currentSubView`. > + * If no view is passed, all navigation attributes for > + * this panelmultiview are cleared. > */ > - _resetKeyNavigation(view = this._currentSubView) { > - let navMap = this._keyNavigationMap.get(view); > + _resetKeyNavigation(view) { > + let viewToBlur = view || this._currentSubView; Uuh, this effectively does the same as before, as in this line equivalent to `resetKeyNavigation(view = this._currentSubView) {`, thus making the documentation change above a bit unclear.

(In reply to Mike de Boer [:mikedeboer] from comment #5) > ::: browser/components/customizableui/PanelMultiView.jsm:1151 > (Diff revision 2) > > * @param {panelview} view View to reset the key navigation attributes of. > > - * Defaults to `this._currentSubView`. > > + * If no view is passed, all navigation attributes for > > + * this panelmultiview are cleared. > > */ > > - _resetKeyNavigation(view = this._currentSubView) { > > - let navMap = this._keyNavigationMap.get(view); > > + _resetKeyNavigation(view) { > > + let viewToBlur = view || this._currentSubView; > > Uuh, this effectively does the same as before, as in this line equivalent to > `resetKeyNavigation(view = this._currentSubView) {`, thus making the > documentation change above a bit unclear. No, we blur items either in the view passed or the current view, *but* importantly we don't reset (clear out) the key nav map unless a view was explicitly passed in. Using default parameters we can't tell whether the view was or wasn't passed in. I guess I could make it more explicit by using an additional parameter, if that seems better to you?

(In reply to :Gijs from comment #6) > No, we blur items either in the view passed or the current view, *but* > importantly we don't reset (clear out) the key nav map unless a view was > explicitly passed in. Err, unless a view *wasn't* explicitly passed in. Sigh.

(In reply to :Gijs from comment #6) > No, we blur items either in the view passed or the current view, *but* > importantly we don't reset (clear out) the key nav map unless a view was > explicitly passed in. Using default parameters we can't tell whether the > view was or wasn't passed in. I guess I could make it more explicit by using > an additional parameter, if that seems better to you? Ah, gotcha. Makes sense; can you then add a comment before `if (view) {` that says something along the lines of `// We clear the entire key nav map only when no view was passed in.` for über-clarity?

Comment on attachment 8897416 [details] Bug 1366207 - remember previous view's selection when keyboard navigating panels, https://reviewboard.mozilla.org/r/168730/#review174162 Fantastic! Nice improvement, Gijs. ::: browser/components/customizableui/PanelMultiView.jsm:1104 (Diff revision 3) > + else if (!isDown && buttonIndex < 0) > + buttonIndex = maxIdx; > + newButton = buttons[buttonIndex]; > + } else { > + // The previously selected item is no longer selectable. Find the next item: > + let allButtons = lastSelected.closest("panelview").querySelectorAll("toolbarbutton"); Aside, would `.getElementsByTagName("toolbarbutton")` be in fact more efficient? ::: browser/components/customizableui/PanelMultiView.jsm:1237 (Diff revision 3) > + let navMap = this._keyNavigationMap.get(viewToBlur); > + if (navMap && navMap.selected && navMap.selected.get()) { > + navMap.selected.get().blur(); > + } > + > + // We clear the entire key navigation map ONLY if *no* view was passed in. Oh, you went über-über-complete! Didn't know that existed, until today ;-) ::: browser/components/customizableui/PanelMultiView.jsm:1269 (Diff revision 3) > }); > } > > /** > + * Focus the last selected element in the view, if any. > + * @param {panelview} view the view in which to update keyboard focus. Nit: please add a newline between the description and the params.

Comment on attachment 8897416 [details] Bug 1366207 - remember previous view's selection when keyboard navigating panels, https://reviewboard.mozilla.org/r/168730/#review174162 > Aside, would `.getElementsByTagName("toolbarbutton")` be in fact more efficient? Yes. Changed this one, but not the other one, which is `querySelectorAll(".subviewbutton:not([disabled])")` because I dunno if it'd be more efficient there, too. Probably not really user-noticeable in the common case.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d7fbbbc99c39 remember previous view's selection when keyboard navigating panels, r=mikedeboer

To be clear,If I perform following steps: 1. Go to Hamburgur menu>> library 2. Click library button 3. In Library panel, click History 4. Go back to library panel subview Should expected result be: History button should be selected ? My observation: If I use keyboard for right/left/up/down navigation then History button stays selected but if I use mouse click then it doesn't. Please let me know the expectation here.

(In reply to Kanchan Kumari QA from comment #15) > To be clear,If I perform following steps: > > 1. Go to Hamburgur menu>> library > 2. Click library button > 3. In Library panel, click History > 4. Go back to library panel subview > > Should expected result be: History button should be selected ? No, the selection only persists for keyboard navigation. When using the mouse to activate items, it's not really clear what the point of a 'selection' would be - you'd need to click it to activate it, which would be enough irrespective of a 'selection'... > My observation: If I use keyboard for right/left/up/down navigation then > History button stays selected but if I use mouse click then it doesn't. > Please let me know the expectation here. This sounds correct to me.

Thanks! I verified this bug fix with the latest nightly and found it to be working as expected.