1791873 - sync-setup-container element is not accessible via keyboard navigation

Status: VERIFIED FIXED

(Firefox :: Firefox View, defect, P1)

Description

[Tim Giles [:tgiles]]

STR:

1. Make sure you are not signed in to a Firefox Account
2. Open Firefox View
3. Navigate to tab-pickup section via keyboard
4. Expand the tab pickup list section via keyboard
5. Hit "Tab" to navigate to next element

Expected:

• The "Continue" button within the "Switch seamlessly between devices" is focused

Actual:

• The "Try Colorways" (or next focusable element in the DOM) is focused instead

Considering there are alternate ways to sign into your Firefox Account, this may be a low severity issue...but it does break the tab pickup list setup flow if navigating with only keyboard/screen reader so that might tip severity in the other direction.

Comment 1

[Ray Fambro]

@gijs -- this one feels pretty important. Let's discuss in triage today.

Comment 2

[:Gijs (out for now; he/him)]

Emilio, any idea why this broke as a result of the reimplementation of summary/details? Do we need to do something special to get keyboard a11y to work inside the <details> bits?

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1308080

Comment 4

[:Gijs (out for now; he/him)]

Hm, I think my regression analysis was wrong and this actually broke in bug 1789885, which is kind of surprising but mozregression shouldn't lie...

Comment 5

[Tim Giles [:tgiles]]

I think this may be a regression from Bug 1790651, since removing the tabindex="-1" on the sync-setup-container fixes the keyboard navigation issue for me. Not sure what else I'm missing though.

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Tim Giles [:tgiles] from comment #5)

I think this may be a regression from Bug 1790651, since removing the tabindex="-1" on the sync-setup-container fixes the keyboard navigation issue for me. Not sure what else I'm missing though.

That looks more plausible, and mozregression --repo autoland --launch <rev> on that and its grandparent (parent was DONTBUILD so no builds) confirms that's the regressor. I'm still confused why mozregression blamed Sam's change; I guess merges confused it? :-(

Comment 7

[:Gijs (out for now; he/him)]

Dan, I'm pretty confused by the patch you landed - you mention you were concerned about getting tabIndex=-1 in "all the right places" - but it's on a bunch of <h1> tags which should never be focusable in the first place (so should be a no-op, I think?) and on the <named-deck>, where I'm not sure what exactly it's accomplishing. Certainly that last one is now breaking expected keyboard functionality. What breaks for the feature callout work if we get rid of it?

Comment 8

[:Gijs (out for now; he/him)]

Oops, looks like Ray overwrote this by accident.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I'm confused about the regressing bug too fwiw. tabIndex is the wrong attribute name. The right one is all lowercase.

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

I'm confused about the regressing bug too fwiw. tabIndex is the wrong attribute name. The right one is all lowercase.

HTML attributes are lowercased so it doesn't end up mattering, and the attribute is applied anyway.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to :Gijs (he/him) from comment #7)

Dan, I'm pretty confused by the patch you landed - you mention you were concerned about getting tabIndex=-1 in "all the right places" - but it's on a bunch of <h1> tags which should never be focusable in the first place (so should be a no-op, I think?) and on the <named-deck>, where I'm not sure what exactly it's accomplishing. Certainly that last one is now breaking expected keyboard functionality. What breaks for the feature callout work if we get rid of it?

This was originally motivated by this bit of bug 1790651 comment 0:

But if the user wandered around, the best place to return the focus would be the last focusable element before the onboarding popup was injected in the DOM or on the topmost element right after it, i.e. when the last popup is dismissed, the focus could be placed on the aside section for Colorways, for instance with the tabindex="-1" on the Independent Voices heading, so the user does not miss any piece of the content after the dismissal"

Re-reading that, however, makes it clear that the tabindex stuff would only make sense if the "on the topmost element right after it" implementation had been chosen.

Since the patch used the "return focus to where it was" implementation, I suspect the right fix is simply to back out the tabindex changes.

Looking at the details of this a bit more, then will write an integration test and an implementation patch this afternoon (i.e. now).

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Bug 1791873 - fix FxView sync-setup-container kbd access,r?gijs,sfoster

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/424b1b1c6566
fix FxView sync-setup-container kbd access,r=Gijs

Comment 14

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/424b1b1c6566

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:dmosedale, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox106 to wontfix.

For more information, please visit auto_nag documentation.

Comment 16

[:Gijs (out for now; he/him)]

Comment on attachment 9295895 [details]
Bug 1791873 - fix FxView sync-setup-container kbd access,r?gijs,sfoster

Beta/Release Uplift Approval Request

• User impact if declined: Broken keyboard access in Firefox View
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0
• List of other uplifts needed: N/A
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Straightforward part-reversal of an earlier patch that just drops tabindex=-1 attributes from this specific page, and has an automated test.
• String changes made/needed: Nope
• Is Android affected?: No

Comment 17

[Emil Ghitta, QA [:emilghitta]]

This is verified fixed using Firefox 107.0a1 (BuildId:20220927213841) on Windows 10 64bit, macOS 11 and Ubuntu 22.

Leaving the qe-verify+ label until this gets uplifted and verified.

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9295895 [details]
Bug 1791873 - fix FxView sync-setup-container kbd access,r?gijs,sfoster

Approved for 106.0b6, thanks.

Comment 19

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/0126d7f24196

Comment 20

[Emil Ghitta, QA [:emilghitta]]

This is verified fixed using Firefox 106.0b6(BuildId:20220929195234) on Windows 10 64bit, Ubuntu 22 and macOS 11.