1378016 - keyboard navigation broken on 'recent bookmarks' of Library Button

Status: VERIFIED FIXED

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

Description

[custom.firefox.lady [:tawn]]

Steps to Reproduce:
1. Click Menu (hamburger) button
2. Click Library
3. Click Bookmarks
4. Press down arrow on keyboard repeatedly

Results:
Highlight advances to Search Bookmarks, but will not advance to the Recently Bookmarked section

Expected result: Highlight continues to advance to next section

But not 100% reproducible, but fails more often than not for me. If you manage to get highlight to advance to a recently bookmarked item, press Enter. (The item does not load; also incorrect behavior.) Then try again.

Comment 1

[Erica Wright [:ewright]]

Attachment: Bug 1378016 - Fix keyboard navigation and selection on 'recent bookmarks'.

Review commit: https://reviewboard.mozilla.org/r/163480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163480/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8892543 [details]
Bug 1378016 - Fix keyboard navigation and selection on 'recent bookmarks'.

https://reviewboard.mozilla.org/r/163480/#review168890

::: browser/components/customizableui/PanelMultiView.jsm
(Diff revision 1)
> -        // If we've been here before, forget about it!
> -        if (button.hasAttribute("tabindex"))
> -          break;

Ahhh, this makes sense to me now. To avoid doing too much useless work and always re-setting this attribute, can we exclude items with a tabindex attribute in the `_getNavigableElements` code? It uses querySelectorAll() and so it should be easy to add a `:not([tabindex])` :-)

::: browser/components/customizableui/PanelMultiView.jsm:1085
(Diff revision 1)
> -        button.click();
> +        button.doCommand();
> +
> +        var clickEvent = event.target.ownerDocument.createEvent("MouseEvents")
> +        clickEvent.initMouseEvent("click", true);
> +        button.dispatchEvent(clickEvent);

Can you clarify why you changed this? I'm not sure I follow...

::: browser/components/customizableui/PanelMultiView.jsm:1093
(Diff revision 1)
> +        clickEvent.initMouseEvent("click", true);
> +        button.dispatchEvent(clickEvent);
>          break;
>        }
> +      case "Space": {
> +        stop();

Likewise, not 100% sure what's going on here.

Comment 3

[Erica Wright [:ewright]]

Comment on attachment 8892543 [details]
Bug 1378016 - Fix keyboard navigation and selection on 'recent bookmarks'.

https://reviewboard.mozilla.org/r/163478/#review168892

::: browser/components/customizableui/PanelMultiView.jsm
(Diff revision 1)
>        buttons = navMap.buttons = this._getNavigableElements(view);
>        // Set the 'tabindex' attribute on the buttons to make sure they're focussable.
>        for (let button of buttons) {
>          if (button.classList.contains("subviewbutton-back"))
>            continue;
> -        // If we've been here before, forget about it!

This was exiting when the permanent members of the dropdown had already had "tabindex" attrivutes, but the dynamic items did not.

::: browser/components/customizableui/PanelMultiView.jsm:1085
(Diff revision 1)
>            break;
>          stop();
> +
>          // Unfortunately, 'tabindex' doesn't not execute the default action, so
>          // we explicitly do this here.
> -        button.click();
> +        button.doCommand();

`click()` was executing a "click" event then a "command" event. the "command" event was exacuted too late and the menu - and event listeners - were already destroyed. 
So i needed to manually call `doCommand()`, then make a custom click event so that it only exacutes the "click" event, and not a second "command" event.

::: browser/components/customizableui/PanelMultiView.jsm:1092
(Diff revision 1)
> +        var clickEvent = event.target.ownerDocument.createEvent("MouseEvents")
> +        clickEvent.initMouseEvent("click", true);
> +        button.dispatchEvent(clickEvent);
>          break;
>        }
> +      case "Space": {

This is to prevent space from behaving like a enter keypress. This might have been from a regression, since the odd behaviour is not on release.
There is likely a better way to do this..., like getting to the bottom of why it is happening in the first place.

Comment 4

[:Gijs (he/him)]

(In reply to Erica from comment #3)
> Comment on attachment 8892543 [details]
> Bug 1378016 - Fix keyboard navigation and selection on 'recent bookmarks'.
> 
> https://reviewboard.mozilla.org/r/163478/#review168892
> 
> ::: browser/components/customizableui/PanelMultiView.jsm
> (Diff revision 1)
> >        buttons = navMap.buttons = this._getNavigableElements(view);
> >        // Set the 'tabindex' attribute on the buttons to make sure they're focussable.
> >        for (let button of buttons) {
> >          if (button.classList.contains("subviewbutton-back"))
> >            continue;
> > -        // If we've been here before, forget about it!
> 
> This was exiting when the permanent members of the dropdown had already had
> "tabindex" attrivutes, but the dynamic items did not.
> 
> ::: browser/components/customizableui/PanelMultiView.jsm:1085
> (Diff revision 1)
> >            break;
> >          stop();
> > +
> >          // Unfortunately, 'tabindex' doesn't not execute the default action, so
> >          // we explicitly do this here.
> > -        button.click();
> > +        button.doCommand();
> 
> `click()` was executing a "click" event then a "command" event. the
> "command" event was exacuted too late and the menu - and event listeners -
> were already destroyed. 
> So i needed to manually call `doCommand()`, then make a custom click event
> so that it only exacutes the "click" event, and not a second "command" event.

Do we actually *need* to run the click event in addition to the doCommand() one in the keyboard case? I would sort of expect not...

> ::: browser/components/customizableui/PanelMultiView.jsm:1092
> (Diff revision 1)
> > +        var clickEvent = event.target.ownerDocument.createEvent("MouseEvents")
> > +        clickEvent.initMouseEvent("click", true);
> > +        button.dispatchEvent(clickEvent);
> >          break;
> >        }
> > +      case "Space": {
> 
> This is to prevent space from behaving like a enter keypress. This might
> have been from a regression, since the odd behaviour is not on release.
> There is likely a better way to do this..., like getting to the bottom of
> why it is happening in the first place.

Space activating the entry is correct, I think. It works the same way in platform menus (e.g. the toplevel menubar on OS X, and similar menus on Windows) as well as "normal" buttons in webpages, so I don't think we should change it here. :-)

Comment 5

[Erica Wright [:ewright]]

(In reply to :Gijs from comment #4)
> Do we actually *need* to run the click event in addition to the doCommand() one in the keyboard case? I would sort of expect not...

yes, otherwise the page will navigate, but the menu will remain open.

Comment 6

[Erica Wright [:ewright]]

Also, if Space activating the entry is correct. Then the issue with it is that it doesn't close the menu after navigating. My other idea, provided it is desired behaviour, was to move it up to where the `case "Enter": {` code is and treating them the same.

Comment 7

[:Gijs (he/him)]

(In reply to Erica from comment #6)
> Also, if Space activating the entry is correct. Then the issue with it is
> that it doesn't close the menu after navigating. My other idea, provided it
> is desired behaviour, was to move it up to where the `case "Enter": {` code
> is and treating them the same.

That sounds sensible.

(In reply to Erica from comment #5)
> (In reply to :Gijs from comment #4)
> > Do we actually *need* to run the click event in addition to the doCommand() one in the keyboard case? I would sort of expect not...
> 
> yes, otherwise the page will navigate, but the menu will remain open.

In that case, should we manually call .hidePopup() instead? That would avoid running whatever click code runs here twice. :-)

Comment 8

[Erica Wright [:ewright]]

(In reply to :Gijs from comment #7)

> > > Do we actually *need* to run the click event in addition to the doCommand() one in the keyboard case? I would sort of expect not...
> > 
> > yes, otherwise the page will navigate, but the menu will remain open.
> 
> In that case, should we manually call .hidePopup() instead? That would avoid
> running whatever click code runs here twice. :-)

I could. I was attempting to as closely as possibly emulate the exact process that happens when a "real" click happens. And this does that. The "real" mouse click triggers a command, then a click event.

Comment 9

[:Gijs (he/him)]

(In reply to Erica from comment #8)
> (In reply to :Gijs from comment #7)
> 
> > > > Do we actually *need* to run the click event in addition to the doCommand() one in the keyboard case? I would sort of expect not...
> > > 
> > > yes, otherwise the page will navigate, but the menu will remain open.
> > 
> > In that case, should we manually call .hidePopup() instead? That would avoid
> > running whatever click code runs here twice. :-)
> 
> I could. I was attempting to as closely as possibly emulate the exact
> process that happens when a "real" click happens. And this does that. The
> "real" mouse click triggers a command, then a click event.

Uh, interesting. Does the command do anything? Doesn't the simulated click trigger a second command event that then doesn't work?

Comment 10

[Erica Wright [:ewright]]

(In reply to :Gijs from comment #9)

So the "command" event makes the navigation happen. And the "click" event closes the library.
calling `.click()` was causing both events to happen, but in the wrong order. 
with the `button.dispatchEvent(clickEvent)` route, it only triggers the "click" event, and not any extra "command" event. 

I did some looking at learned that these simulated click events, like `.click()` are not the same as a real mouse click. Though it has been tough to figure out why and how. Since I couldn't create a real event which would send both triggers in the correct order, I am instead sending them individually. 

if it helps you see how the real event compares to what I've written, add `aEvent.target.ownerGlobal.console.log(aEvent);` into `/browser/components/customizableui/CustomizableUI.jsm`  into the top of `handleEvent` at line (currently) 1246.

Comment 11

[Erica Wright [:ewright]]

Comment on attachment 8892543 [details]
Bug 1378016 - Fix keyboard navigation and selection on 'recent bookmarks'.

https://reviewboard.mozilla.org/r/163480/#review168890

> Ahhh, this makes sense to me now. To avoid doing too much useless work and always re-setting this attribute, can we exclude items with a tabindex attribute in the `_getNavigableElements` code? It uses querySelectorAll() and so it should be easy to add a `:not([tabindex])` :-)

I can't add :not([tabindex]) to `_getNavigableElements`, since it is used in other things as well which need the complete list of elements.
I can do:
```
if (button.hasAttribute("tabindex"))
  continue;
```
or something like:
```
buttons = buttons.filter(function (button) {
  return !button.hasAttribute("tabindex");
});
```
from within `_keyNavigation`

Comment 12

[:Gijs (he/him)]

Comment on attachment 8892543 [details]
Bug 1378016 - Fix keyboard navigation and selection on 'recent bookmarks'.

https://reviewboard.mozilla.org/r/163480/#review168946

::: browser/components/customizableui/PanelMultiView.jsm
(Diff revision 1)
> -        // If we've been here before, forget about it!
> -        if (button.hasAttribute("tabindex"))
> -          break;

Ugh, I should have checked that, sorry.

Can we combine the check with an inversion of the previous if statement, so we get:

```js
if (!button.classList.contains("subviewbutton-back") &&
    !button.hasAttribute("tabindex")) {
  button.setAttribute("tabindex", 0);
}
```

that might be the best we can do here, I guess.

::: browser/components/customizableui/PanelMultiView.jsm:1083
(Diff revision 1)
>          // Unfortunately, 'tabindex' doesn't not execute the default action, so
>          // we explicitly do this here.

Can you fix the double negation in this comment while we're here? :-)

::: browser/components/customizableui/PanelMultiView.jsm:1087
(Diff revision 1)
> +        var clickEvent = event.target.ownerDocument.createEvent("MouseEvents")
> +        clickEvent.initMouseEvent("click", true);
> +        button.dispatchEvent(clickEvent);

OK, let's do this, then, but can you add a comment explaining we're mirroring the behaviour of a 'normal' click?

Nit: we should use `let`, and `new Event` style event creation over the deprecated `createEvent`:

```js
let clickEvent = new event.target.ownerGlobal.MouseEvent("click");
button.dispatchEvent(clickEvent);
```

::: browser/components/customizableui/PanelMultiView.jsm:1093
(Diff revision 1)
> +        clickEvent.initMouseEvent("click", true);
> +        button.dispatchEvent(clickEvent);
>          break;
>        }
> +      case "Space": {
> +        stop();

OK, so we can fix this by just adding a:

```js
case "Space":
// fall through
```
before the `case "Enter"` above, so the two keys do the same. Check that eslint is happy with that - there's a rule about deliberate fall throughs, I don't know offhand if it's enabled here - there should be a comment syntax that makes it happy if so.

Comment 13

[Erica Wright [:ewright]]

Comment on attachment 8892543 [details]
Bug 1378016 - Fix keyboard navigation and selection on 'recent bookmarks'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163480/diff/1-2/

Comment 14

[:Gijs (he/him)]

Comment on attachment 8892543 [details]
Bug 1378016 - Fix keyboard navigation and selection on 'recent bookmarks'.

https://reviewboard.mozilla.org/r/163480/#review169250

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as fixed.

Comment 16

[Erica Wright [:ewright]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Autoland can't push this until all pending issues in MozReview are marked as
> fixed.

Done, didn't know that.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7d0e0ba501a
Fix keyboard navigation and selection on 'recent bookmarks'. r=Gijs

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d7d0e0ba501a

Comment 19

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 56.0a1 (2017-07-03) on Windows 7, 64 Bit!

This bug's fix is verified with latest Nightly!


Build  ID  : 20170810100255 
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 20

[u554753]

Verified on Windows, Mac, and Ubuntu.