Add an autofocus option to PopupNotifications.show

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Site Identity and Permission Panels
P2
normal
RESOLVED FIXED
9 months ago
3 months ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox53 wontfix, firefox54 fix-optional, firefox55 fixed)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

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

Attachments

(2 attachments)

(Assignee)

Description

9 months ago
We had a discussion on IRC about doorhanger focus and the question came up if doorhangers should steal focus on show. For reference, Chrome and Safari both steal focus. I guess if the doorhangers are supposed to be modal it makes sense to go all the way here.

Before implementing I'd like to be sure we're aware of possible concerns around this.
Marco, we'd like your input on this, particularly as it relates to screen readers.
What is the implication of doorhangers stealing focus?

The doorhanger may be popped up via user interaction or not (eg. geolocation).  We may want to only do focus in one of those scenarios.
Flags: needinfo?(mzehe)

Comment 2

9 months ago
tl;dr: Sorry, there isn't one, you'll have to muddle with me through this lengthy comment. :)

Oh dear... There have been discussions around this in several other bugs, at least one of which is still open and not fixed, and related to doorhangers, too:

bug 616136, which started making these accessible in the first place.

bug 653230, which is about autofocusing those doorhangers specifically. There's some discussion here.

And slightly unrelated, but still relevant for the overall usability: bug 653226, which has a lot of discussion, but no solution involved parties could agree on.

Doorhangers have been a constant point of complaint since Firefox 4.0, so if there's a new fresh good idea to fix this mess, I am all for it. Note, however, that there's conflicting interest here:

1. The point of doorhangers was to not be as intrusive as modal dialogs, which we had before. If these are now auto-focused, we return to an ideom that is close to modal dialogs. Although from feedback that I get from many, this is actually what they would expect if we ask them a question such as "Do you want to save the password?".

2. If focus is not stolen, there's currently a mechanism in place that, as soon as something on a page gets focus, the doorhanger goes away. Screen readers such as NVDA start reading a newly loaded page automatically, and taking the focus along to focusable elements as they go, to give a visual indication as to where they are reading currently. That, in turn, makes any possible doorhangers go away immediately, and one has to dig them up again from the address bar. And since many pages start with something like a link to their home page, screen readers are almost always going to quickly draw focus somewhere, making the doorhangers go away.

I am very grateful for this initiative, and I am hoping that this time, we can finally get these right!
Flags: needinfo?(mzehe)
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1335741
(Assignee)

Updated

9 months ago
Blocks: 1333437
Flags: qe-verify+
(Assignee)

Comment 4

9 months ago
Thanks for writing this up. It gives us a better understanding of what we need to consider.

Probably the best technical solution to this would be to add a focus option to PopupNotifications.show that can be set by the caller depending on the event. I would propose only giving focus to user-initiated web permission prompts for now and open up separate bugs in password manager and addons manager where doorhangers for these components can be considered individually.

I'll make this a P1 but leave for triage to confirm my thoughts.
Priority: -- → P1

Updated

9 months ago
Iteration: --- → 54.2 - Feb 20
(Assignee)

Updated

8 months ago
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
(Assignee)

Updated

8 months ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 6

8 months ago
This is only the API part for autofocus, since we still need to figure out when exactly to make use of it and show autofocus. I'm also preparing a patch to figure out if a user has triggered the permission request and that includes a lot of DOM code.

Comment 7

8 months ago
Comment on attachment 8839466 [details]
Bug 1334496 - Part 1 - Add an autofocus option to PopupNotifications.show.

For the moment, I don't have a clear idea of how things should work here.

I tend to think that "autofocus" shouldn't be a property of the individual notification, but rather we should pass a "focus" option to the method that opens the panel, independently from the notification.
Attachment #8839466 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 8

8 months ago
> I tend to think that "autofocus" shouldn't be a property of the individual notification, but rather we should pass a "focus" option to the method that opens the panel, independently from the notification.

Sorry, I don't understand. PopupNotifications.show is the last instance that shows a new notification for the consumers of the public API, AFAIK. How would e.g. the Geolocation code be able to specify that it wants to autofocus the popup?

Comment 9

8 months ago
I guess my unclear wording was part of not having a clear idea... basically the "focus" option should only take effect if we display the notification immediately as a result of calling the "show" method, and be ignored afterwards. When opening the panel from the iconbox, we should always focus it, which to me also means that screen readers should start reading the text of the panel.

A similar type of option is "dismissed", although the implementation then reuses this value to keep the current state of the notification, making the similarity less clear. Note how it doesn't make sense to specify "focus" and "dismissed" at the same time.
Comment hidden (mozreview-request)
(Assignee)

Comment 11

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b50e9fa5475
(Assignee)

Comment 12

8 months ago
Ah, good point. I had not considered this because currently all other methods to open the popup will autofocus, but you're right.  I hope this patch works better.

However, we seem to have a different problem. It seems like noautofocus is broken (it always focuses) on non-e10s. I can reproduce locally but I'd like to give it a closer look before making a bug for that, to make sure it's not just our PopupNotifications code being silly. In any case, this bug is kind of blocked on that.

Comment 13

8 months ago
mozreview-review
Comment on attachment 8839466 [details]
Bug 1334496 - Part 1 - Add an autofocus option to PopupNotifications.show.

https://reviewboard.mozilla.org/r/114104/#review117124

::: browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js:166
(Diff revision 2)
> +      let notification = showNotification(notifyObj);
> +      let popup = yield opened;
> +      checkPopup(popup, notifyObj);
> +
> +      EventUtils.synthesizeKey("VK_TAB", {});
> +      isnot(Services.focus.focusedElement, popup.childNodes[0].closebutton);

"isnot" doesn't seem robust to changes in the structure of the panel, although I see that there is a similar "is" condition before, but more importantly it doesn't guarantee that we didn't actually move focus elsewhere or away from the main document.

We should probably focus a well-known element, in the content document and/or in the browser window, and then check that the focus isn't stolen.

In fact, I see that the use cases that Marco mentioned are related to the interaction between reading the main document and displaying a notification, so that's probably what we should test.

I also imagine you may have been saving some of these test for part 2.

::: toolkit/modules/PopupNotifications.jsm:248
(Diff revision 2)
>      let doc = this.window.document;
> -    let activeElement = doc.activeElement;
> +    let focusedElement = Services.focus.focusedElement;
>  
>      // If the chrome window has a focused element, let it handle the ESC key instead.
> -    if (!activeElement ||
> -        activeElement == doc.body ||
> -        activeElement == this.tabbrowser.selectedBrowser ||
> +    if (!focusedElement ||
> +        focusedElement == doc.body ||
> +        focusedElement == this.tabbrowser.selectedBrowser ||
>          // Ignore focused elements inside the notification.
> -        getNotificationFromElement(activeElement) == notification ||
> -        notification.contains(activeElement)) {
> +        getNotificationFromElement(focusedElement) == notification ||
> +        notification.contains(focusedElement)) {
>        this._onButtonEvent(aEvent, "secondarybuttoncommand", notification);
>      }

The rest of the patch looks good to me, but actually I don't understand:

- Why this has changed from document.activeElement to focus.focusedElement
- Whether keeping this unchanged would make the new test fail
- Why the new test for focusing from the API uses focusedElement
- Why the existing tests for focusing from the URL bar use activeElement
- Why some methods of focusing require the TAB key and others don't

Clearing review for now, although there might not be actual changes needed.
Attachment #8839466 - Flags: review?(paolo.mozmail)

Comment 14

8 months ago
(In reply to :Paolo Amadini from comment #13)
> - Why some methods of focusing require the TAB key and others don't

To clarify, the keyboard and screen reading experience for notifications should likely be identical whether a panel is shown as a result of activating the icon from the URL bar or activating a JavaScript link from a webpage which results in the API being called with the "autofocus" parameter.

Maybe, there could be a difference regarding which element is focused again after the notification has been accepted or dismissed.
(Assignee)

Comment 15

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c023597893bd
(Assignee)

Comment 16

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e783f3b7b39
Comment hidden (mozreview-request)
(Assignee)

Comment 18

8 months ago
(In reply to :Paolo Amadini from comment #13)
> Comment on attachment 8839466 [details]
> Bug 1334496 - Part 1 - Add an autofocus option to PopupNotifications.show.
> 
> https://reviewboard.mozilla.org/r/114104/#review117124
> 
> :::
> browser/base/content/test/popupNotifications/
> browser_popupNotification_keyboard.js:166
> (Diff revision 2)
> > +      let notification = showNotification(notifyObj);
> > +      let popup = yield opened;
> > +      checkPopup(popup, notifyObj);
> > +
> > +      EventUtils.synthesizeKey("VK_TAB", {});
> > +      isnot(Services.focus.focusedElement, popup.childNodes[0].closebutton);
> 
> "isnot" doesn't seem robust to changes in the structure of the panel,
> although I see that there is a similar "is" condition before, but more
> importantly it doesn't guarantee that we didn't actually move focus
> elsewhere or away from the main document.
> 
> We should probably focus a well-known element, in the content document
> and/or in the browser window, and then check that the focus isn't stolen.
> 

Good point, I changed the test to check if the urlbar is focused.
I also can't reproduce the non-e10s related problem after a rebase anymore, so, huh.

> In fact, I see that the use cases that Marco mentioned are related to the
> interaction between reading the main document and displaying a notification,
> so that's probably what we should test.

You're right, we should also have a test for when content elements are focused. Maybe we can make it a follow-up bug?

> 
> I also imagine you may have been saving some of these test for part 2.
> 
> ::: toolkit/modules/PopupNotifications.jsm:248
> (Diff revision 2)
> >      let doc = this.window.document;
> > -    let activeElement = doc.activeElement;
> > +    let focusedElement = Services.focus.focusedElement;
> >  
> >      // If the chrome window has a focused element, let it handle the ESC key instead.
> > -    if (!activeElement ||
> > -        activeElement == doc.body ||
> > -        activeElement == this.tabbrowser.selectedBrowser ||
> > +    if (!focusedElement ||
> > +        focusedElement == doc.body ||
> > +        focusedElement == this.tabbrowser.selectedBrowser ||
> >          // Ignore focused elements inside the notification.
> > -        getNotificationFromElement(activeElement) == notification ||
> > -        notification.contains(activeElement)) {
> > +        getNotificationFromElement(focusedElement) == notification ||
> > +        notification.contains(focusedElement)) {
> >        this._onButtonEvent(aEvent, "secondarybuttoncommand", notification);
> >      }
> 
> The rest of the patch looks good to me, but actually I don't understand:
> 
> - Why this has changed from document.activeElement to focus.focusedElement
> - Whether keeping this unchanged would make the new test fail
> - Why the new test for focusing from the API uses focusedElement
> - Why the existing tests for focusing from the URL bar use activeElement

Sorry, I should have clarified that with the original patch. So far I always used document.activeElement but focusedElement
returns null (instead of the focused window, like document.activeElement) if no element inside the focused chrome window is focused.
That saves me another line of comparison and getting the focused window to compare against. That's also the reason why I use it in the test.

> - Why some methods of focusing require the TAB key and others don't

In my view, ideally, focus-after-user-selection (e.g. pressing the space bar on the notification anchor) should not not require TABing to the next focus element while autofocus should require a TAB so that the user does not accidentally (or maliciously engineered by the website) interact with the permission notification.

There's also the aspect that I would like to hold off from working around a11y issues with panels and try to find out how to solve focus behavior for a11y at a lower level (see bug 1343210 comment 16).

We should keep in mind that the behavior is different here but I'd like to keep it this way for now.

Comment 19

8 months ago
(In reply to Johann Hofmann [:johannh] from comment #18)
> > In fact, I see that the use cases that Marco mentioned are related to the
> > interaction between reading the main document and displaying a notification,
> > so that's probably what we should test.
> 
> You're right, we should also have a test for when content elements are
> focused. Maybe we can make it a follow-up bug?

If that's too complicated for part 2 of this bug, I'm fine with a follow-up.

> > - Why some methods of focusing require the TAB key and others don't
> 
> In my view, ideally, focus-after-user-selection (e.g. pressing the space bar
> on the notification anchor) should not not require TABing to the next focus
> element while autofocus should require a TAB so that the user does not
> accidentally (or maliciously engineered by the website) interact with the
> permission notification.

Agreed, and in cases where we focus the button, screen readers should still read the question and not skip directly to just reading the button text.

> There's also the aspect that I would like to hold off from working around
> a11y issues with panels and try to find out how to solve focus behavior for
> a11y at a lower level (see bug 1343210 comment 16).

I think that still reading the question is related to the "accessibility focus" that Neil mentioned in bug 1343210, and that sounds definitely like something we can handle more comprehensively elsewhere.

> We should keep in mind that the behavior is different here but I'd like to
> keep it this way for now.

Sound good.

Comment 20

8 months ago
mozreview-review
Comment on attachment 8839466 [details]
Bug 1334496 - Part 1 - Add an autofocus option to PopupNotifications.show.

https://reviewboard.mozilla.org/r/114104/#review119650
Attachment #8839466 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Depends on: 1345424

Comment 23

8 months ago
mozreview-review
Comment on attachment 8844841 [details]
Bug 1334496 - Part 2 - Test that content elements stay focused when showing a popup notification.

https://reviewboard.mozilla.org/r/118174/#review120008

::: browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js:182
(Diff revision 2)
> +    *run() {
> +      let id = this.id;
> +      yield BrowserTestUtils.withNewTab("about:blank", function*(browser) {
> +        let notifyObj = new BasicNotification(id);
> +        yield ContentTask.spawn(browser, {}, function() {
> +          content.document.body.innerHTML = "<input id='test-input' />";

You may try to open a "data:" URI instead, but if it doesn't work with notifications for some reason, then modifying "about:blank" is fine.

::: browser/base/content/test/popupNotifications/browser_popupNotification_keyboard.js:190
(Diff revision 2)
> +        // Check that the browser is still the focused element in the chrome window.
> +        is(Services.focus.focusedElement, browser);

This is actually different in e10s versus non-e10s.

An alternative may be to check the focused element in a frame script, using Components.classes to access the focus service, or importing Services.jsm. I haven't tried, but if this returns null when the content window does not have focus, then it's the only check you have to make to ensure the keyboard input target is preserved.

Alternatively you can simulate a key press after showing the notification, and check that the value of the input field changes. I'm not sure if this works either, or if our key press simulation moves focus in any way or ignores it.
Attachment #8844841 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 24

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d75da9ad7f5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

7 months ago
How about this? I hope that's safe :)

Comment 28

7 months ago
mozreview-review
Comment on attachment 8844841 [details]
Bug 1334496 - Part 2 - Test that content elements stay focused when showing a popup notification.

https://reviewboard.mozilla.org/r/118174/#review120980

Ah, you noticed my bluff. :-) Yes, that's the first solution that came to my mind, though it's still a shortcut relying on undocumented implementation details.

One failure mode I see here is that we change some code to focus the entire browser window, and for some reason document.activeElement stays the same but keyboard input is directed to the parent process where the browser window is still focused. This is, admittedly, as far as I know, unlikely to happen in practice.

The other is that there may be an unsafe CPOW usage if the first test fails, which may make an intermittent more difficult to categorize.

So, if you're going for this solution it may be better to check whether e10s is enabled beforehand, and do one check or the other based on that.
Attachment #8844841 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

7 months ago
> Ah, you noticed my bluff. :-) Yes, that's the first solution that came to my mind, though it's still a shortcut relying on undocumented implementation details.

Heh, yeah, sorry for that :D

> One failure mode I see here is that we change some code to focus the entire browser window, and for some reason document.activeElement stays the same but keyboard input is directed to the parent process where the browser window is still focused. This is, admittedly, as far as I know, unlikely to happen in practice.

Hm, I guess that's valid but I'm not sure if we need to be so sophisticated about the test. I added a check for e10s and I think and hope the test will be solid enough in practice.

Thanks!

Updated

7 months ago
Iteration: 54.2 - Feb 20 → 55.1 - Mar 20
(Assignee)

Updated

7 months ago
status-firefox53: affected → fix-optional
status-firefox54: affected → fix-optional
Priority: P1 → P2
(Assignee)

Comment 32

7 months ago
I'm changing the scope of this because I'd like to get the API functionality checked in to prevent bitrot and this bug getting forgotten due to other priorities. The patch has r+ and tests so I think that's warranted. I'll make a followup bug to consider adding autofocus to notifications.
status-firefox53: fix-optional → wontfix
status-firefox55: --- → affected
No longer depends on: 1345424
Summary: PopupNotifications do not autofocus on showing → Add an autofocus option to PopupNotifications.show
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 35

7 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05c382c86b47
Part 1 - Add an autofocus option to PopupNotifications.show. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/d1227a050b84
Part 2 - Test that content elements stay focused when showing a popup notification. r=Paolo

Comment 36

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/05c382c86b47
https://hg.mozilla.org/mozilla-central/rev/d1227a050b84
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

7 months ago
Blocks: 1354551
(Assignee)

Updated

6 months ago
Depends on: 1354761
Can you please give us some examples of which notifications are affected by this fix and which are not?
Flags: needinfo?(jhofmann)
(Assignee)

Comment 38

3 months ago
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #37)
> Can you please give us some examples of which notifications are affected by
> this fix and which are not?

Hey, this is purely an API change and doesn't need to be tested. The user-facing change would arrive in bug 1354551. Sorry for not updating the flag.
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(jhofmann)
You need to log in before you can comment on or make changes to this bug.