Bug 52917

Summary: REGRESSION (r75543): Styles bleed into new shadow DOM (like slider and video)
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: DOMAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, eric.carlson, hyatt, marc.hoyois, mitz, rolandsteiner, thillaibsetec, tkent
Priority: P1 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 52963    
Bug Blocks: 46595, 48698, 53122, 54179, 57274    
Attachments:
Description Flags
Patch
none
Patch none

Description Dimitri Glazkov (Google) 2011-01-21 13:48:54 PST
REGRESSION(r75543): Styles bleed into new shadow DOM (like slider).
Comment 1 Dimitri Glazkov (Google) 2011-01-21 13:49:26 PST
Created attachment 79785 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2011-01-21 13:54:22 PST
This is not a significant problem for the moment, but it should be solved soon. FWIW, star selector always bled into the shadow DOM.
Comment 3 Dimitri Glazkov (Google) 2011-01-22 10:44:39 PST
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.
Comment 4 Kent Tamura 2011-04-06 07:17:39 PDT
How will we fix this issue?
Applying only pseudo selectors to shadow nodes?
Comment 5 Dimitri Glazkov (Google) 2011-04-06 07:40:43 PDT
(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.
Comment 6 Dimitri Glazkov (Google) 2011-04-17 07:13:34 PDT
*** Bug 58684 has been marked as a duplicate of this bug. ***
Comment 7 Dimitri Glazkov (Google) 2011-04-20 07:12:37 PDT
*** Bug 58971 has been marked as a duplicate of this bug. ***
Comment 8 Eric Carlson 2011-04-20 08:02:23 PDT
<rdar://problem/9310653>
Comment 9 mitz 2011-04-20 12:02:52 PDT
(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/>.
Comment 10 Dimitri Glazkov (Google) 2011-04-20 12:11:05 PDT
Created attachment 90381 [details]
Patch
Comment 11 Dimitri Glazkov (Google) 2011-04-20 15:20:47 PDT
(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! :)
Comment 12 Kent Tamura 2011-04-21 09:35:48 PDT
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...
Comment 13 Dimitri Glazkov (Google) 2011-04-21 10:25:34 PDT
(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 :)
Comment 14 Dimitri Glazkov (Google) 2011-04-21 10:37:57 PDT
Committed r84517: <http://trac.webkit.org/changeset/84517>
Comment 15 Roland Steiner 2011-06-07 00:53:12 PDT
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.
Comment 16 Dimitri Glazkov (Google) 2011-06-07 10:02:35 PDT
(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.
Comment 17 Roland Steiner 2011-06-07 19:45:12 PDT
(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.