keyboard navigation broken on 'recent bookmarks' of Library Button

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
a year ago
7 months ago

People

(Reporter: tawn, Assigned: ewright)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Depends on: 1354159
Whiteboard: [photon-structure]
Whiteboard: [photon-structure] → [photon-structure] [triage]

Updated

a year ago
Blocks: 1354144, 1354159
No longer depends on: 1354159
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon-structure] [triage] → [photon-structure]
(Assignee)

Updated

a year ago
Assignee: nobody → ewright
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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.
Attachment #8892543 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 3

a year ago
mozreview-review
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

a year ago
(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. :-)
(Assignee)

Comment 5

a year ago
(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.
(Assignee)

Comment 6

a year ago
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

a year ago
(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. :-)
(Assignee)

Comment 8

a year ago
(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

a year ago
(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?
(Assignee)

Comment 10

a year ago
(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.
(Assignee)

Comment 11

a year ago
mozreview-review-reply
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

a year ago
mozreview-review
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 hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
Comment on attachment 8892543 [details]
Bug 1378016 - Fix keyboard navigation and selection on 'recent bookmarks'.

https://reviewboard.mozilla.org/r/163480/#review169250
Attachment #8892543 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as fixed.
Flags: needinfo?(ewright)
Keywords: checkin-needed
(Assignee)

Comment 16

a year ago
(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.
Flags: needinfo?(ewright)

Comment 17

a year ago
Pushed by bwinton@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7d0e0ba501a
Fix keyboard navigation and selection on 'recent bookmarks'. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/d7d0e0ba501a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.1 - Aug 15

Comment 19

11 months ago
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
QA Whiteboard: [bugday-20170809]
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
Blocks: 1425972
No longer blocks: 1425972
You need to log in before you can comment on or make changes to this bug.