REGRESSION (r75543): Styles bleed into new shadow DOM (like slider and video)

• Component: DOM
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

REGRESSION(r75543): Styles bleed into new shadow DOM (like slider).

[Dimitri Glazkov (Google)]

Created attachment 79785 [details]
Patch

[Dimitri Glazkov (Google)]

This is not a significant problem for the moment, but it should be solved soon. FWIW, star selector always bled into the shadow DOM.

[Dimitri Glazkov (Google)]

The trick here is that we need to know quickly whether a node is inside of a shadow DOM. Node::isInShadowDOM() traverses up, so it's not sufficient. The solution is fixing bug 52963.

[Kent Tamura]

How will we fix this issue?
Applying only pseudo selectors to shadow nodes?

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> How will we fix this issue?
> Applying only pseudo selectors to shadow nodes?

Yes. That's why we need to have O(1) access to shadow root -- to not have to traverse the tree to find it out.

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

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

[Eric Carlson]

<rdar://problem/9310653>

(In reply to comment #2)
> This is not a significant problem for the moment, but it should be solved soon. FWIW, star selector always bled into the shadow DOM.

It is breaking the video controls at <https://squareup.com/>.

[Dimitri Glazkov (Google)]

Created attachment 90381 [details]
Patch

[Dimitri Glazkov (Google)]

(In reply to comment #9)
> (In reply to comment #2)
> > This is not a significant problem for the moment, but it should be solved soon. FWIW, star selector always bled into the shadow DOM.
> 
> It is breaking the video controls at <https://squareup.com/>.

I have a fix! :)

[Kent Tamura]

Comment on attachment 90381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90381&action=review

> LayoutTests/fast/css/shadow-dom-scope.html:45
> +    runSelectorTest(NO_MATCH, 'div');
> +    runSelectorTest(NO_MATCH, '*');
> +    runSelectorTest(NO_MATCH, 'body *');

There are no MATCH tests.  Are MATCH cases covered by existing tests?

> Source/WebCore/ChangeLog:35
> +        * dom/ShadowRoot.h: Added.

This line looks the file is added.

> Source/WebCore/ChangeLog:38
> +        * dom/TreeScope.h: Added.

ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:755
> +private:

nit: We usually put a blank line before it.

> Source/WebCore/css/CSSStyleSelector.cpp:756
> +    static bool m_matchingUARules;

I wondered if such global variable was ok, and found there were some other instances of global variables...

[Dimitri Glazkov (Google)]

(In reply to comment #12)
> (From update of attachment 90381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90381&action=review
> 
> > LayoutTests/fast/css/shadow-dom-scope.html:45
> > +    runSelectorTest(NO_MATCH, 'div');
> > +    runSelectorTest(NO_MATCH, '*');
> > +    runSelectorTest(NO_MATCH, 'body *');
> 
> There are no MATCH tests.  Are MATCH cases covered by existing tests?

Yes! LayoutTests/fast/css/unknown-pseudo-element-matching.html 

> 
> > Source/WebCore/ChangeLog:35
> > +        * dom/ShadowRoot.h: Added.
> 
> This line looks the file is added.

> 
> > Source/WebCore/ChangeLog:38
> > +        * dom/TreeScope.h: Added.
> 
> ditto.
> 

Will fix.

> > Source/WebCore/css/CSSStyleSelector.cpp:755
> > +private:
> 
> nit: We usually put a blank line before it.

Will fix.

> 
> > Source/WebCore/css/CSSStyleSelector.cpp:756
> > +    static bool m_matchingUARules;
> 
> I wondered if such global variable was ok, and found there were some other instances of global variables...

Yeah, me too :)

[Dimitri Glazkov (Google)]

Committed r84517: <http://trac.webkit.org/changeset/84517>

[Roland Steiner]

Unless I'm mistaken (a common enough occurrence), it seems there may be cases where a bleed still can happen: E.g., you have a media control panel component that defines a container for button controls. This container is given a pseudo-ID "panel", e.g., to allow styling its background. The container also has one or more children of its own for the individual buttons, which are defined as separate components.

In this case, a selector such as "::panel *" would bleed into the buttons, even if they are set to not allow selectors through. That check would be circumvented by the pseudo-ID used in the parent panel component.

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> Unless I'm mistaken (a common enough occurrence), it seems there may be cases where a bleed still can happen: E.g., you have a media control panel component that defines a container for button controls. This container is given a pseudo-ID "panel", e.g., to allow styling its background. The container also has one or more children of its own for the individual buttons, which are defined as separate components.
> 
> In this case, a selector such as "::panel *" would bleed into the buttons, even if they are set to not allow selectors through. That check would be circumvented by the pseudo-ID used in the parent panel component.

Yes, you're right. We should probably file a bug on this.

[Roland Steiner]

(In reply to comment #16)
> Yes, you're right. We should probably file a bug on this.

Filed new issue https://bugs.webkit.org/show_bug.cgi?id=62261.