1518121 - Focus seems broken when `overflow` is set in native Shadow DOM

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

## Description
To better illustrate the problem, I've created a simple reduced test case at Glitch. I realized that tab order does not work as expected by when tabbing thru focusable elements in a document when there is an element with `overflow` set to either `auto` or `scroll` (or as long as there is an overflow content in between focusable elements). The tab order will not focus elements below the overflow content.

I don't think this is an expected behavior because it breaks logical tab order of a document. The demo works fine on browsers with native Shadow DOM v1 supported like Chrome but not tested on Safari (tabbing does not work on Safari of my machine for some reasons).

FWIW, It affects FF63 and above and not tested without Shadow DOM (if it does not work without SD, it simple means tab order is totally broken.)

## Additional info

Demo URL - https://pumped-seat.glitch.me
Full source code - https://glitch.com/edit/#!/pumped-seat?path=views/broken-focus.js:1:0


Actual results:

Tab order looks broken. Focus will be restricted to focusable elements above overflow content.


Expected results:

Tab order should work as expected on browsers with native Shadow DOM v1 supported like Chrome. Focusable elements below overflow content should be focusable thru tabbing.

Comment 1

[Daniel_A[:daschilean] [Please contact - [e-mail]]]

I verified this issue on Windows 10 x64 with the latest Firefox release version 64.0.2 (64-bit), Firefox Nightly 66.0a1 (2019-01-10) (64-bit) and I cannot reproduce it.
Can you please test this in safe mode? Here is a link that can help you: https://goo.gl/AR5o9d.
If you still have the issue please create a new profile, you have the steps here: https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles?redirectlocale=en-US&redirectslug=Managing-profiles#w_starting-the-profile-manager
Also, can you please create a videoscreen capture when you can and if you have time? To be sure that we understand the same steps to reproduce this issue.

Comment 2

[[e-mail]]

Daniel, I've recorded a simple video to demonstrate the issue at https://youtu.be/qettFyfY3Fs.

From the video, you can see that focus never moves to the button below the overflow content as I was tabbing thru.

Comment 3

[[e-mail]]

Also, I was running Firefox Nightly 66 with the latest update on Windows x64 (with latest update too) in safe mode while recording. Hope this helps in the investigation.

Comment 4

[Timea Cernea [:tbabos][inactive]]

Thanks a lot for the testcase and for the report!
Reproduced the issue on Firefox Nightly 67, Beta 66 and Release 65 on Windows 10 x64

Mozzregression also revealed that this is a regression caused by shadowDOM:

Last good revision: 1879856ca1ab0def40038be7482a9bc2e140cfee
First bad revision: 2718c0bfff1d6e9cf07e28212cf7d1dd5b09485e

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1879856ca1ab0def40038be7482a9bc2e140cfee&tochange=2718c0bfff1d6e9cf07e28212cf7d1dd5b09485e

Regression introduced by: Bug 1490820

Olli, can you please take a look at it? Not sure if other bug reports highlighted this regression or not.

Comment 5

[[e-mail]]

Thanks for updating this bug report. Good to hear that the bug has been verified and confirmed as a regression.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

:wchen, could you look in to this given Olli's backlog?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

I flagged :wchen as the reviewer of another patch for shadow DOM that mentioned focus (bug 1079236).

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

wchen isn't actively around.

Comment 9

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

Jared, can we find another owner for this bug and prioritize it with regards to our upcoming releases? Is that something we want to fix before the next ESR or can it remain in the backlog until we have cycles to fix it? Thanks

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Pascal Chevrel:pascalc from comment #9)

Jared, can we find another owner for this bug and prioritize it with regards to our upcoming releases? Is that something we want to fix before the next ESR or can it remain in the backlog until we have cycles to fix it? Thanks

I believe Edgar is able to provide inputs here. :)

Comment 11

[Edgar Chen [:edgar]]

Attachment: test.html

reduced test script.

Comment 12

[Edgar Chen [:edgar]]

So the issue here is that nsIFrame::IsFocusable returns true with tabIndex = 0 for a scrollable frame [1], so we could move foucus on it. But in [2], we just get tabIndex from the content which returns -1 in this case, then we could not able to find next tabbable content correctly.

[1] https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/layout/generic/nsFrame.cpp#9529-9530
[2] https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/dom/base/nsFocusManager.cpp#3147-3151

Comment 13

[Edgar Chen [:edgar]]

(In reply to Pascal Chevrel:pascalc from comment #9)

Jared, can we find another owner for this bug and prioritize it with regards to our upcoming releases? Is that something we want to fix before the next ESR or can it remain in the backlog until we have cycles to fix it? Thanks

I think fixing this is not something complicated, we could try to fix this before next ESR (68).

Comment 14

[Edgar Chen [:edgar]]

Attachment: Test 2.html

Found another case that Shift+Tab doesn't work as expected when 'overflow' is set on slot element.

Steps to reproduce:

1. Click input text has blue border to move focus on it.
2. Press Shift+Tab

Actual result:
The focus moves to input text with value=3.

Expect result:
The focus should move to input text with value=4.

Comment 15

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=80d15fa3267a6f8812354afa8e555bec2ccc1e70

Comment 16

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=6eb09f82dd818c50466ecf6ed6cd56b1e8a0aa85&group_state=expanded

Comment 17

[Edgar Chen [:edgar]]

Attachment: Bug 1518121 - Part 1: Check isFocusable from frame first if content has frame;

This is for the scrollable frame, because nsIFrame::IsFocusable of a scrollable
frame returns true with tabIndex = 0 even if the content has no tabIndex
attribute.

Comment 18

[Edgar Chen [:edgar]]

Attachment: Bug 1518121 - Part 2: Update ExplicitChildIterator correctly when we are at the end of child list of a slot parent;

The mIndexInInserted keeps pointing to the last assigned node of a slot parent
when we are at the end of child list. So when we calles GetPreviousChild when we
are at the end of child list, ExplicitChildIterator will skip last assigned node.

Comment 19

[Edgar Chen [:edgar]]

Attachment: Bug 1518121 - Part 3: Add tests for scrollable frame in Shadow DOM;

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfeedf139126
Part 1: Check isFocusable from frame first if content has frame; r=smaug
https://hg.mozilla.org/integration/autoland/rev/db9ccc4f8c6e
Part 2: Update ExplicitChildIterator correctly when we are at the end of child list of a slot parent; r=smaug
https://hg.mozilla.org/integration/autoland/rev/8a65241e7c3d
Part 3: Add tests for scrollable frame in Shadow DOM; r=smaug

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/dfeedf139126
https://hg.mozilla.org/mozilla-central/rev/db9ccc4f8c6e
https://hg.mozilla.org/mozilla-central/rev/8a65241e7c3d

Comment 22

[[e-mail]]

Awesome! Thank you for the great work.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is this something which should be nominated for Beta approval or can it ride the trains?