1340112 - Bad focus behavior when navigation to popupnotifications via keyboard

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect)

Description

[Johann Hofmann [:johannh]]

Our current logic for focusing popupnotifications when they are selected via keyboard goes like this

- When the user switched from one notification to the other, we turn off noautofocus temporarily (which is in fact broken right now, try switching between notifications on https://permission.site).
- When the user re-selects a notification anchor, we focus the button because that's the only element we know is always present.

This is complex to handle, inconsistent, partly broken and I learned from bug 1335018 that it is important to immediately move focus into notification panels for screen readers to start reading the content.

To conclude, a better solution is to use document.commandDispatcher.advanceFocusIntoSubtree to focus the next available element in the panel as soon as it's open.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1340112 - Move focus into popupnotification panels when selected via keyboard.

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

Comment 2

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a11e202c295

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8838045 [details]
Bug 1340112 - Move focus into popupnotification panels when selected via keyboard.

https://reviewboard.mozilla.org/r/113058/#review114528

The patch worked fine for me during my testing on Mac. It may be worth testing on Windows where the Allow/Don't Allow button order is reversed.

::: browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js:78
(Diff revision 1)
>      },
>      onHidden(popup) { }
>    },
> +  // Test that you can switch between active notifications with the space key
> +  // and that the notification is focused on selection.
> +  { id: "Test#4",

I would like one of the notifications used in this test to have some content, so that the test ensures we aren't just focusing one of the action buttons.

It's probably as easy as having the 'remember' checkbox be visible for one of the two notifications.

::: toolkit/modules/PopupNotifications.jsm:1231
(Diff revision 1)
> -
>      // Avoid reshowing notifications that are already shown and have not been dismissed.
>      if (this.panel.state == "closed" || anchor != this._currentAnchorElement) {
> -      this._reshowNotifications(anchor);
> -    }
> +      // As soon as the panel is shown, focus the first element in the selected notification.
> +      this.panel.addEventListener("popupshown",
> +        () => this.window.document.commandDispatcher.advanceFocusIntoSubtree(this.panel), {once: true});

nit: line break before {once

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8838045 [details]
Bug 1340112 - Move focus into popupnotification panels when selected via keyboard.

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

Comment 5

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd738c37e2b0

Comment 6

[Johann Hofmann [:johannh]]

> The patch worked fine for me during my testing on Mac. It may be worth testing on Windows where the Allow/Don't Allow button order is reversed.

Thanks for pointing me to that. I hope that also would've been caught by the tests on try. Lacking a quick viable solution, I just removed the hideClose option and check if the closebutton is focused now.

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8838045 [details]
Bug 1340112 - Move focus into popupnotification panels when selected via keyboard.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f746373f4b1
Move focus into popupnotification panels when selected via keyboard. r=florian

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1f746373f4b1

Comment 10

[Hani Yacoub, Desktop QA]

I tested this on Firefox Nightly 54.0a1 (2017-02-23)(64-bit) on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64.

Once I switch from one notification to the other on https://permission.site from what I understood here was that the auto focus should be on the popup notification but it's not, but as you previously mentioned that in fact it's broken right now.
On the other hand, after I selected a notification anchor, the focus was not on the button as you mentioned before, in fact the focus was on the first element which can be clickable like "Microphone to share" or "Learn more".

It's not that clear what should we test here, and what are the expected behavior.

Comment 11

[Johann Hofmann [:johannh]]

> Once I switch from one notification to the other on https://permission.site from what I understood here was that the auto focus should be on the popup notification but it's not, but as you previously mentioned that in fact it's broken right now.

By "switch from one notification to the other" you mean you clicked on one of the buttons on the site to open a new permission popup? Yes, that currently doesn't auto-focus the permission popup.

> On the other hand, after I selected a notification anchor, the focus was not on the button as you mentioned before, in fact the focus was on the first element which can be clickable like "Microphone to share" or "Learn more".

That is exactly the behavior we want, selecting the very first item that's selectable. Sorry if that was unclear from reading the bug :)

Comment 12

[Hani Yacoub, Desktop QA]

Considering the previous comment I will mark this as Verified as fixed on Firefox Nightly 54.0a1 (2017-02-23)(64-bit) on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64.