Bug 52917 - REGRESSION (r75543): Styles bleed into new shadow DOM (like slider and video)
Summary: REGRESSION (r75543): Styles bleed into new shadow DOM (like slider and video)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords: InRadar, Regression
: 58684 58971 (view as bug list)
Depends on: 52963
Blocks: 46595 48698 53122 54179 57274
  Show dependency treegraph
 
Reported: 2011-01-21 13:48 PST by Dimitri Glazkov (Google)
Modified: 2011-06-07 19:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.44 KB, patch)
2011-01-21 13:49 PST, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (10.95 KB, patch)
2011-04-20 12:11 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.