Closed Bug 1334496 Opened 7 years ago Closed 7 years ago

Add an autofocus option to PopupNotifications.show

Categories

(Firefox :: Site Identity, defect, P2)

Product:
Firefox
Component:
Site Identity
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.1 - Mar 20
Tracking Flags:
Tracking Status
firefox52 --- unaffected
firefox53 --- wontfix
firefox54 --- fix-optional
firefox55 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files)

Bug 1334496 - Part 1 - Add an autofocus option to PopupNotifications.show.
59 bytes, text/x-review-board-request
Paolo
: review+
Details
Bug 1334496 - Part 2 - Test that content elements stay focused when showing a popup notification.
59 bytes, text/x-review-board-request
Paolo
: review+
Details 
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)
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)
Blocks: 1333437
Flags: qe-verify+
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
Iteration: --- → 54.2 - Feb 20
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
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 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)
> 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?
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.
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 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)
(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.
(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.
(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 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+
Depends on: 1345424
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)
How about this? I hope that's safe :)
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+
> 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!
Iteration: 54.2 - Feb 20 → 55.1 - Mar 20
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
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
https://hg.mozilla.org/mozilla-central/rev/05c382c86b47
https://hg.mozilla.org/mozilla-central/rev/d1227a050b84
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 1354551
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)
(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.

Attachment

General

Creator:
Created:
Updated:
Size: