Focus unexpectedly moves into shadow dom if the first element of shadow dom has same tabindex with current focused element
Categories
(Core :: DOM: Core & HTML, defect, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox74 | --- | fixed |
People
(Reporter: edgar, Assigned: edgar)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
While debugging bug 1604140, I found this issue in the frame traversal logic around https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/dom/base/nsFocusManager.cpp#3356-3569.
|
Assignee | |
Comment 1•4 years ago
•
|
||
(In reply to Edgar Chen [:edgar] from comment #0)
While debugging bug 1604140, I found this issue in the frame traversal logic around https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/dom/base/nsFocusManager.cpp#3356-3569.
What happens here is that: when we traverse to the first element inside the shadow dom, the currentContent points to the element and oldTopLevelScopeOwner is nullptr, so we do https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/dom/base/nsFocusManager.cpp#3362 which sets currentTopLevelScopeOwner to the shadow host. And then we run into the if-block in https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/dom/base/nsFocusManager.cpp#3431-3448, but do nothing, because the tabindex of shadow host doesn't match with the index we are looking for.
And then we unexpected check the currentContent again, which is wrong, because we have already checked the whole scope in the above if-block https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/dom/base/nsFocusManager.cpp#3431-3448. And we mean to check the currentContent only when it is in top-level-scope, https://searchfox.org/mozilla-central/rev/9b99e1d9c6cf83539674cb016c7373f549ba59ca/dom/base/nsFocusManager.cpp#3366-3380 should do the filter job.
|
|
Assignee | |
Comment 2•4 years ago
|
||
After bug 1544826, FindOwner returns only shadow host or slot, update the
comment and rename it to FindScopeOwner to reflect the current behavior.
|
|
Assignee | |
Comment 3•4 years ago
|
||
Currently if we call GetTopLevelScopeOwner with a <slot> which is in top-level-scope, e.g. <body><slot></slot></body>.
It returns <slot> itself, but it should returns nullptr per design.
|
|
Assignee | |
Comment 4•4 years ago
|
||
This patch contains three changes, but we just simpliy the logic, the result is the same,
1). s/oldTopLevelScopeOwner/currentTopLevelScopeOwner/ in line
at this point, oldTopLevelScopeOwner is equal to currentTopLevelScopeOwner, we can use either of them,
but I prefer using currentTopLevelScopeOwner given that we change the value of currentTopLevelScopeOwner in the if-block.
2). remove else-block
We run into this else-block when aForward && oldTopLevelScopeOwner == currentContent,
And oldTopLevelScopeOwner equals to currentTopLevelScopeOwner at this point, so currentTopLevelScopeOwner also equals to currentContent.
It is not necessary to set currentTopLevelScopeOwner to currentContent again.
3). s/IsHostOrSlot(currentTopLevelScopeOwner)/currentTopLevelScopeOwner/
After above two changes, currentTopLevelScopeOwner is always a scope owner (a host or slot) if it is not null,
so we could remove IsHostOrSlot() checks. And if we don't pass a other non-scope-owner, we will hit the asserion
in HostOrSlotTabIndexValue()
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 5•4 years ago
|
||
See https://bugzilla.mozilla.org/show_bug.cgi?id=1607223#c1 for the details.
|
|
Assignee | |
Comment 6•4 years ago
|
||
|
|
Assignee | |
Comment 7•4 years ago
|
||
(In reply to Edgar Chen [:edgar] from comment #6)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c705a434ce057d9343b58123e605adb50ad549bf
Oh, and this fix could also make following wpt passed,
- https://searchfox.org/mozilla-central/source/testing/web-platform/tests/shadow-dom/focus/focus-tabindex-order-shadow-zero-host-one.html
- https://searchfox.org/mozilla-central/source/testing/web-platform/tests/shadow-dom/focus/focus-tabindex-order-shadow-zero-host-negative.html
(I didn't realized there are wpt for such case)
|
|
Assignee | |
Comment 8•4 years ago
|
||
Pushed by echen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/948884557092 Part 1: Update the comment of `FindOwner` and rename function name to `FindScopeOwner`; r=smaug https://hg.mozilla.org/integration/autoland/rev/66f91734a62a Part 2: Fix GetTopLevelScopeOwner; r=smaug https://hg.mozilla.org/integration/autoland/rev/3220f1acbdf6 Part 3: Simplify the logic of handling *topLevelScopeOwner; r=smaug https://hg.mozilla.org/integration/autoland/rev/6ba2eb15c992 Part 4: Avoid performing check of next tabbable content on the first element of shadow dom duplicatedly; r=smaug
|
||
Comment 10•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/948884557092
https://hg.mozilla.org/mozilla-central/rev/66f91734a62a
https://hg.mozilla.org/mozilla-central/rev/3220f1acbdf6
https://hg.mozilla.org/mozilla-central/rev/6ba2eb15c992
Description
•