Closed Bug 1490820 Opened 1 year ago Closed 1 year ago

Sequential focus handling issue in Shadow DOM

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files)

Load the https://www.familysearch.org/ web page.
Sign in (required, but an account is free).
Go to a Family Tree Person Page (for example https://www.familysearch.org/tree/person/M3BB-PWK )
In the "Vitals" section of the page, click "Edit" to the right of "Birth".
In the "Edit Birth" popup, press the <tab> key on your keyboard.
The "X" in the top right corner of the poup gets focus.
Press <tab> again, the Date of Birth gets focus.
Press <tab> again, nothing happens.

This bad behavior is present in 63.0b5 (64-bit) on Windows 10
and in current nightly, but the page works correctly in 62.0
Priority: -- → P2
Assignee: nobody → bugs
Attached file minimal testcase
Ronald, thanks a ton for reporting the issue!

Took a bit time to find the minimal testcase, but fix should be very simple.
We rely elsewhere that focusable element actually has nsIFrame, since for normal DOM we use frame traversal to find next focusable content. But shadow DOM DOM tree traversal is used (per shadow DOM spec).
Attachment #9010936 - Flags: review?(ehsan)
Comment on attachment 9010936 [details] [diff] [review]
sequential_focus_in_hidden_shadow_dom.diff

Review of attachment 9010936 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/test/file_bug1453693.html
@@ +126,5 @@
>          sr.appendChild(shadowInput);
>  
> +        var hiddenShadowButton = document.createElement("button");
> +        hiddenShadowButton.setAttribute("style", "display: none;");
> +        sr.appendChild(hiddenShadowButton);

Is there a wpt test that covers this?  It seems like having a wpt test for this would be valuable...
Attachment #9010936 - Flags: review?(ehsan) → review+
I don't know how to write wpt tests dealing with native input events.
There are some ways, but last time I asked, it was still limited.
jgraham told that is now possible
https://searchfox.org/mozilla-central/search?q=test_driver.send_keys&path=

I guess the test should be converted to wpt
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2718c0bfff1d
elements without frames shouldn't be focusable in shadow DOM, r=ehsan
Comment on attachment 9010936 [details] [diff] [review]
sequential_focus_in_hidden_shadow_dom.diff

Approval Request Comment
[Feature/Bug causing the regression]: New feature, shadow dom, bug 1205323
[User impact if declined]: sequential focus handling may be broken (tab key)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: I don't think so 
[List of other uplifts needed for the feature/fix]: NA
[Is the change risky?]: this shouldn't be very risky
[Why is the change risky/not risky?]: ... because this just skips elements which aren't visible when looking for the next element to focus in shadow DOM
[String changes made/needed]: NA
Attachment #9010936 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/2718c0bfff1d
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9010936 [details] [diff] [review]
sequential_focus_in_hidden_shadow_dom.diff

Low risk patch for a bug in a new 63 feature, uplift approved for 63 beta 9, thanks.
Attachment #9010936 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1518121
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.