1597993 - Keyboard navigation gets trapped in the Login Item

Status: VERIFIED FIXED

(Firefox :: about:logins, defect, P1)

Description

[Simona Rosu [:srosu], Ecosystem QA]

Attachment: Tabbing.mp4

[Affected versions]:

• Latest Firefox Nightly 72.0a1 (Build ID: 20191120094758) (64-bit)
• Firefox Beta 71.0b11 (Build ID: 20191118154140) (64-bit)

[Affected Platforms]:

• Mac 10.14
• Windows 10 x64
• Arch Linux x64

[Prerequisites]:

• Have a Firefox profile with at least one saved login.

[Steps to reproduce]:

1. Open the Firefox browser with the profile from prerequisites.
2. Navigate to the “about:logins” page.
3. Press the “Tab” key until all elements from the page are focused.
4. Repeat the previous step and observe the behavior.

[Expected result]:

• All elements from the page are focused.

[Actual result]:

• The focus remains trapped in the Login Item mode.

[Regression]

• The issue is not reproducible with older Nightly 71.0a1 builds. Considering this, using mozregression tool I have found the issue that broke this in the latest Nightly build. Here are the results:
• Pushlog: https://tinyurl.com/y2benyvj
• Last good revision: 263936aecc1dcd15f9dadbf4e41f7e7eebad29a6
• First bad revision: ab46062892d418b37dbfa35fc3a61db5f7c9de59

[Additional Notes]:

• Attached a screen recording with the issue.

Comment 1

[Matthew N. [:MattN]]

[Tracking Requested - why for this release]: a11y regression

Bug 1586601 seems like the only likely culprit in that regression range but it's not immediately obvious what is going on. Maybe the markup change made some CSS apply differently.

Comment 2

[David Durst [:ddurst]]

(Interesting that changing direction and tabbing back breaks out of this)

Comment 3

[Pascal Chevrel:pascalc (PTO until April 26)]

We have shipped our last beta for 71, since this is a tracked P1,, I am setting the status as fix-optional for 71 in case there is a safe uplift possible in a potential dot release as a ride-along.

Comment 4

[David Durst [:ddurst]]

(Noting that the offending rule is the overflow: auto on :host in login-item.css.)

Comment 5

[Andy Bruère [:abruere]]

I filed a new bug 1604140 after digging into this one and successfully reproducing it with a simple component. If this new bug is confirmed, this would mean that current bug is not a regression due to Bug 1586601, but a long-standing shadow DOM-related bug it revealed.

@ddurst thank you for your comments that helped me to reproduce this focus trap.

Still willing to contribute to any effort if needed.

Comment 6

[Matthew N. [:MattN]]

Thank you very much!

(In reply to Andy Bruère from comment #5)

If this new bug is confirmed, this would mean that current bug is not a regression due to Bug 1586601, but a long-standing shadow DOM-related bug it revealed.

It can be both a regression due to bug 1586601 and a long-standing DOM issue :) Bug 1586601 changed about:logins to hit this issue so it's still a regression from the about:logins user standpoint. If we can achieve the same end-result while not hitting that bug then it's an option to consider, OR we can wait for your bug 1604140 to get fixed.

Comment 7

[Andy Bruère [:abruere]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #6)

Thank you very much!

(In reply to Andy Bruère from comment #5)

If this new bug is confirmed, this would mean that current bug is not a regression due to Bug 1586601, but a long-standing shadow DOM-related bug it revealed.

It can be both a regression due to bug 1586601 and a long-standing DOM issue :) Bug 1586601 changed about:logins to hit this issue so it's still a regression from the about:logins user standpoint. If we can achieve the same end-result while not hitting that bug then it's an option to consider, OR we can wait for your bug 1604140 to get fixed.

Indeed, thank you for taking the time to correct my statement!

I will keep digging into some existing bugs and offer my help without creating extra ones ;)

I had a look at the existing tests: I could help adding a few focus-related assertions (some of which would obviously fail for now) in https://searchfox.org/mozilla-central/source/browser/components/aboutlogins/tests/chrome/test_login_item.html similar to keyboard navigation tests for test_login_list, to prevent similar regressions.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Did bug 1607223 affect the behavior here at all? Note that even if it did help here, I'm not sure that's something we'd want to uplift to Beta for Fx73. If there's any sort of workaround we can land in the mean time, I think it would be worth doing so to fix this usability regression for keyboard-only users.

Comment 9

[Simona Rosu [:srosu], Ecosystem QA]

I have retested this issue using the latest Nightly 74.0a1 (Build ID: 20200113204142) and the issue is still reproducible using the steps described in comment 0. I have tested this on Windows 10 x64, Ubuntu 16.04 x64 and MacOS 10.14.
Unfortunately, I haven't observed any changes or improvements regarding this behavior using the latest Nightly 74.0a1 build that contains the fix for bug 1607223.

Comment 10

[Andy Bruère [:abruere]]

Attachment: Bug 1597993 - Rework login-item overflow rule to solve focus trap r=MattN

Comment 11

[Andy Bruère [:abruere]]

Attachment: Bug 1597993 - Add regression test for login-item focus trap r=MattN

Depends on D60257

Comment 12

[Andy Bruère [:abruere]]

I’ve just submitted a fix with a new test for focus trap. I simply moved the overflow: auto rule, waiting for Bug 1604140 (P2) to be fixed.

This new test fails with current login-item component as expected and should keep passing if https://phabricator.services.mozilla.com/D60257 is reverted once Bug 1604140 is fixed.

I’ll be happy to make any change needed so you can land this in the mean time.

Note: I had a browser test rather than a chrome test to be able to detect focus trap.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad51c58bb7b7
Rework login-item overflow rule to solve focus trap r=MattN
https://hg.mozilla.org/integration/autoland/rev/b7c6f1201537
Add regression test for login-item focus trap r=MattN

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ad51c58bb7b7
https://hg.mozilla.org/mozilla-central/rev/b7c6f1201537

Comment 15

[Simona Rosu [:srosu], Ecosystem QA]

I have verified this issue using the latest Nightly 74.0a1 (Build ID: 20200129093157) on Windows 10 x64, Mac 10.14, Ubuntu 16.04 x64.

• Keyboard navigation is no longer trapped in the Login Item.