Bug 191694

Summary: Click on node assigned to slot in button's shadow cause loss of button focus
Product: WebKit Reporter: Jan Miksovsky <jan>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, jan, koivisto, rniwa, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=191851
Bug Depends on:    
Bug Blocks: 148695    
Attachments:
Description Flags
Fixes the bug wenson_hsieh: review+

Jan Miksovsky
Reported 2018-11-15 09:06:13 PST
See discussion at: https://github.com/w3c/webcomponents/issues/773 Summary: Consider a custom element whose shadow contains a focusable element that contains a slot: <test-element> #shadow-root <div tabindex="0"> <slot></slot> </div> </test-element> And suppose this custom element receives some light DOM children: <test-element> <span>Click me</span> </test-element> Expect: a click on the light DOM span should give focus to the focusable div inside test-element. Actual: the focus goes to the document body. Repro: https://jsbin.com/qivoheg/edit?html,output. Firefox exhibits the desired behavior here. Chrome currently has a bug to fix this, rniwa indicated in the linked discussion that this should be fixed in WebKit too.
Attachments
Fixes the bug (4.83 KB, patch)
2018-11-19 21:16 PST, Ryosuke Niwa 		wenson_hsieh: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2018-11-15 14:19:01 PST
*** Bug 111231 has been marked as a duplicate of this bug. ***
Radar WebKit Bug Importer
Comment 2 2018-11-15 14:19:45 PST
<rdar://problem/46107920>
Ryosuke Niwa
Comment 3 2018-11-19 21:16:31 PST
Created attachment 355308 [details] Fixes the bug
Wenson Hsieh
Comment 4 2018-11-19 21:44:26 PST
Comment on attachment 355308 [details] Fixes the bug r=mews As an aside, it looks like there's similar logic in FrameSelection::setFocusedElementIfNeeded() that attempts to walk up the DOM in search of an element to focus.
Ryosuke Niwa
Comment 5 2018-11-19 21:47:28 PST
(In reply to Wenson Hsieh from comment #4) > Comment on attachment 355308 [details] > Fixes the bug > > r=mews > > As an aside, it looks like there's similar logic in > FrameSelection::setFocusedElementIfNeeded() that attempts to walk up the DOM > in search of an element to focus. Since we don't really allow the edibility to propagate across shadow boundaries, that one might be okay. It's probably also never going t one useful in real life.
Ryosuke Niwa
Comment 6 2018-11-20 00:53:59 PST
Committed r238393: <https://trac.webkit.org/changeset/238393>
Antti Koivisto
Comment 7 2018-11-20 03:55:39 PST
Comment on attachment 355308 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=355308&action=review > Source/WebCore/page/EventHandler.cpp:2602 > // Walk up the DOM tree to search for an element to focus. > - Element* element; > - for (element = m_elementUnderMouse.get(); element; element = element->parentOrShadowHostElement()) { > + RefPtr<Element> element; > + for (element = m_elementUnderMouse.get(); element; element = element->parentElementInComposedTree()) { Most (all?) clients of parentOrShadowHostElement() are buggy or work correctly by accident (the context can't have slots). It would be good to get rid of it entirely.
Ryosuke Niwa
Comment 8 2018-11-20 16:17:08 PST
(In reply to Antti Koivisto from comment #7) > Comment on attachment 355308 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=355308&action=review > > > Source/WebCore/page/EventHandler.cpp:2602 > > // Walk up the DOM tree to search for an element to focus. > > - Element* element; > > - for (element = m_elementUnderMouse.get(); element; element = element->parentOrShadowHostElement()) { > > + RefPtr<Element> element; > > + for (element = m_elementUnderMouse.get(); element; element = element->parentElementInComposedTree()) { > > Most (all?) clients of parentOrShadowHostElement() are buggy or work > correctly by accident (the context can't have slots). It would be good to > get rid of it entirely. Yeah, but I think it requires quite a bit of figuring out what makes sense in each case. e.g. Element::isSpellCheckingEnabled uses parentOrShadowHostElement but it's non-sensical to call parentElementInComposedTree there since the content editable doesn't propagate across shadow boundaries. parentNode() probably makes more sense there but perhaps we'd have to special case input/textarea because when spellcheck=false is set on input/textarea, we're supposed to disable spellchecking.
Ryosuke Niwa
Comment 9 2018-12-03 12:57:24 PST
Comment on attachment 355308 [details] Fixes the bug The patch had already been landed.
Note You need to log in before you can comment on or make changes to this bug.