1343210 - Only move focus into identity popup when opened via keyboard

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Johann Hofmann [:johannh]]

In bug 1335018 we improved the a11y of the identity popup by moving focus into it. Turns out that on many platforms this causes the focus ring to show up when selecting the identity popup, which it should only on keyboard navigation.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1343210 - Only move focus into identity popup when opened via keyboard.

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

Comment 2

[Johann Hofmann [:johannh]]

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8841959 [details]
Bug 1343210 - Only move focus into identity popup when opened via keyboard.

https://reviewboard.mozilla.org/r/116016/#review117404

::: browser/base/content/browser.js:7364
(Diff revision 1)
> +    if (event.type == "keypress") {
> +      // Move focus to the next available element in the identity popup.
> +      // This is required by role=alertdialog and fixes an issue where
> +      // an already open panel would steal focus from the identity popup.
> +      this._identityPopup.addEventListener("popupshown",
> +        () => document.commandDispatcher.advanceFocusIntoSubtree(this._identityPopup),
> +        {once: true}
> +      );
> +    }

This introduces a subtle bug where, if a popupshowing handler stops the popup being shown, the listener will persist and then we'll have 2 listeners the next time the popup is shown.

It seems simpler (and would fix this bug) to simply store a property on every invocation here:

```js
this._wasTriggeredByKeyboard = event.type == "keypress";
```

and then check that property in the `onPopupShown` handler.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Is this something that needs tracking for Fx52 still given that we created the first RC build for it yesterday?

Comment 5

[Johann Hofmann [:johannh]]

Yes, 52 should ideally be fixed. I hope this is still possible. Sorry for the delay, I was traveling yesterday.

I'll get a revised patch up and we can hopefully request uplift tomorrow.

[Tracking Requested - why for this release]:
UI regression that we recently uplifted to beta. Patch should be fairly simple.

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8841959 [details]
Bug 1343210 - Only move focus into identity popup when opened via keyboard.

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

Comment 7

[Johann Hofmann [:johannh]]

Now with a test!

Talking to shorlander on IRC he said this is not worth an uplift [if the risk is too high or if we're too late in the process]. I tend to agree, though I'm not really happy about it. Gijs, what's your impression of this patch? Should we hold off from asking for a late uplift?

Comment 8

[:Gijs (he/him)]

Comment on attachment 8841959 [details]
Bug 1343210 - Only move focus into identity popup when opened via keyboard.

https://reviewboard.mozilla.org/r/116016/#review117594

::: browser/base/content/browser.js:7401
(Diff revision 2)
>  
> +    // Reset in case the popup is triggered somewhere else. We currently
> +    // only care about keypress events from the identity block.
> +    this._popupTriggeredByKeyboard = false;
> +
> +    if (event.target == this._identityPopup) {

Why does the focus call no longer live inside this if block? And why do we need to reset the prop to false?

::: browser/base/content/test/siteIdentity/browser_identityPopup_focus.js:14
(Diff revision 2)
> +    yield shown;
> +    isnot(Services.focus.focusedElement, document.getElementById("identity-popup-security-expander"));
> +  });
> +});
> +
> +// Access the identity popup via mouseclick. Focus should be moved inside.

Broken comment?

Comment 9

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #7)
> Now with a test!
> 
> Talking to shorlander on IRC he said this is not worth an uplift [if the
> risk is too high or if we're too late in the process]. I tend to agree,
> though I'm not really happy about it. Gijs, what's your impression of this
> patch? Should we hold off from asking for a late uplift?

We're building release candidates at this point, and this is a polish issue, so I don't think we should bother with uplifting to 52, no.

Comment 10

[Johann Hofmann [:johannh]]

Alright then. Thanks!

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8841959 [details]
Bug 1343210 - Only move focus into identity popup when opened via keyboard.

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

Comment 12

[:Gijs (he/him)]

Comment on attachment 8841959 [details]
Bug 1343210 - Only move focus into identity popup when opened via keyboard.

https://reviewboard.mozilla.org/r/116016/#review117754

r=me given that it kind of works, but given that we now have some time, can you please check with Marco that this works for him? IIRC a11y tools sometimes simulate clicks when activating items. Not sure though.

Really, the correct behaviour here would be to always put focus in the popup but not show the focus rectangle in the mouse case. The fact that the focus rectangle is being shown is presumably a consequence of the implementation of advanceFocusIntoSubtree effectively 'triggering' keyboard focus rings, which it shouldn't do. I expect that the 'right' fix would be to add a parameter to that API that allows passing the focus method. Neil Deakin would know for sure.

Comment 13

[Johann Hofmann [:johannh]]

> Really, the correct behaviour here would be to always put focus in the popup but not show the focus rectangle in the mouse case.

An alternative that Marco mentioned would be to allow focusing the popup (without a focus ring) instead of setting focusedElement to null when a popup appears.

Neil, according to Marco the default keyboard focus story around popups needs some slight improvements. Does one of these suggestions sound viable to you?

Comment 14

[Johann Hofmann [:johannh]]

Marco, this patch makes it so that focus is only moved into the site identity popup if the user accessed it via keyboard (because we presume that is sufficient for a11y). Is that a safe assumption?

Comment 15

[Neil Deakin]

The right fix is for the accessibility code to just set its own notion of focus and send any internal events it needs correctly for panels, and not require application developers to add workarounds, the same way that menus and tooltips just work.

Comment 16

[Johann Hofmann [:johannh]]

Okay, taking a step back I agree that we're going overboard with workarounds for things that should obviously work on platform/a11y side. Nonetheless, since this bug is strictly an improvement to the current situation I'll go ahead and land it. In the next days I'll try to talk to a11y folks about what's really missing in panel functionality (not exclusively the security UI part) and come up with a solution/a new bug that doesn't require frontend hacks.

With that said, I think it's good to have a workaround in place until this is solved since it should be easy for screen readers to get access to the site identity popup right now.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0fed4c802a8
Only move focus into identity popup when opened via keyboard. r=Gijs

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you're ready to do so.

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8841959 [details]
Bug 1343210 - Only move focus into identity popup when opened via keyboard.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1335018
[User impact if declined]: Users will see a focus ring in the site identity panel without navigating with the keyboard, which is distracting
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Small, straightforward frontend patch that is covered by tests.
[String changes made/needed]: None

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8841959 [details]
Bug 1343210 - Only move focus into identity popup when opened via keyboard.

Minor fix for focus on site identity popups.  
This should land for beta 2.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3322424b416a

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dc8c26683d20

Comment 24

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Johann's assessment on manual testing needs (see Comment 20) and the fact that this fix has automated coverage.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8841959 [details]
Bug 1343210 - Only move focus into identity popup when opened via keyboard.

See comment 20.

Comment 26

[Julien Cristau [:jcristau]]

Comment on attachment 8841959 [details]
Bug 1343210 - Only move focus into identity popup when opened via keyboard.

focus tweak for site identity popup, esr52+

Comment 27

[Carsten Book [:Tomcat]]

does not apply cleanly to esr52 so need rebasing

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/ed9521749d6f

Comment 29

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Johann's assessment on manual testing needs and the fact that this fix has automated coverage (see Comment 20).

Comment 30

[James Teh [:Jamie]]

Was there a follow up discussion/bug concerning the more general situation with panel focus?

I don't follow how a11y code can possibly solve this without frontend hacks/workarounds/whatever.
1. This is not just related to a11y code; the problem is also relevant for keyboard only users, which is not a11y code. So, even if we could solve this in a11y code, frontend would still have to solve it for keyboard users.
2. I don't follow why it makes sense for a11y to have its own notion of focus as suggested in comment 15. By most definitions, keyboard focus *is* inside the panel, since pressing tab moves to the next item in the panel. To put this another way, why does it make sense for focus to be null when the panel is opened? I don't follow how having focus in limbo ever makes sense. I also don't understand why we would want to have two separate notions of focus. At the very least, that's super confusing.

I raise this again because we have another problem in that site identity panel. When you press the "Show connection details" button, focus again goes into limbo and a screen reader doesn't know what to report. Putting a11y clients aside, a keyboard user would have to press tab after pressing the button, which is pretty non-standard behaviour. I'll file a separate bug for this, but wanted to check whether I was missing a more generic bug/discussion.

Comment 31

[Neil Deakin]

Please file another bug, yes. I don't recall how it relates to this bug, but there is a general issue as the keyboard focus is not always the same as the active accessible item, as there are elements that cannot receive the keyboard focus yet can be the active accessible.