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•5 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•5 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•5 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•5 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•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
See https://bugzilla.mozilla.org/show_bug.cgi?id=1607223#c1 for the details.
Assignee | ||
Comment 6•5 years ago
|
||
Assignee | ||
Comment 7•5 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•5 years ago
|
||
Comment 10•5 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
•