Closed Bug 1366207 Opened 3 years ago Closed 2 years ago

Keep subview buttons selected after subview became visible

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.2 - Aug 29
Tracking Status
firefox57 --- verified

People

(Reporter: mikedeboer, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

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.
Flags: qe-verify+
Priority: P1 → P2
QA Contact: gwimberly
Priority: P2 → P3
Whiteboard: [photon-structure] → [reserve-photon-structure]
Duplicate of this bug: 1373963
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
Blocks: 1387512
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 57.2 - Aug 29
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.
Attachment #8897416 - Flags: review?(mdeboer) → review-
(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?
Flags: needinfo?(mdeboer)
(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?
Flags: needinfo?(mdeboer)
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.
Attachment #8897416 - Flags: review?(mdeboer) → review+
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
https://hg.mozilla.org/mozilla-central/rev/d7fbbbc99c39
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(kkumari)
Thanks! I verified this bug fix with the latest nightly and found it to be working as expected.
Status: RESOLVED → VERIFIED
Flags: needinfo?(kkumari)
Flags: qe-verify+
Depends on: 1425972
No longer depends on: 1425972
You need to log in before you can comment on or make changes to this bug.