Bug 191694 – Click on node assigned to slot in button's shadow cause loss of button focus

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jan Miksovsky]

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.

[Ryosuke Niwa]

*** Bug 111231 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/46107920>

[Ryosuke Niwa]

Created attachment 355308 [details]
Fixes the bug

[Wenson Hsieh]

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]

(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]

Committed r238393: <https://trac.webkit.org/changeset/238393>

[Antti Koivisto]

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]

(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 on attachment 355308 [details]
Fixes the bug

The patch had already been landed.