Bug 62218 - Shadow Pseudo ID should be able to nest to point nested shadow DOM
Summary: Shadow Pseudo ID should be able to nest to point nested shadow DOM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Hajime Morrita
URL:
Keywords:
: 62261 (view as bug list)
Depends on:
Blocks: 72352 59827
  Show dependency treegraph
 
Reported: 2011-06-07 10:05 PDT by Dimitri Glazkov (Google)
Modified: 2012-06-12 18:28 PDT (History)
10 users (show)

See Also:


Attachments
Patch that exposes the bug (1.03 KB, text/plain)
2012-05-09 16:53 PDT, Silvia Pfeiffer
no flags Details
Patch (8.19 KB, patch)
2012-06-11 02:02 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (13.04 KB, patch)
2012-06-11 22:50 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (13.41 KB, patch)
2012-06-11 22:59 PDT, Hajime Morrita
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-06-07 10:05:09 PDT
Per https://bugs.webkit.org/show_bug.cgi?id=52917#c15, we need to be more precise in determining whether a rule applies a given instance of the shadow DOM.
Comment 1 Roland Steiner 2011-06-07 21:13:12 PDT
*** Bug 62261 has been marked as a duplicate of this bug. ***
Comment 2 Hajime Morrita 2012-02-27 23:52:15 PST
Roland, do you think this is live?
Comment 3 Roland Steiner 2012-02-28 00:12:42 PST
I think this is still valid, but more of a theoretical concern at the moment. Per spec, a pseudo-element selector cannot be followed by anything, except a few pseudo-classes. It's only when we relax this requirement that it may become a real issue.

(setting this to P3 for now).
Comment 4 Dimitri Glazkov (Google) 2012-02-28 08:58:27 PST
(In reply to comment #3)
> I think this is still valid, but more of a theoretical concern at the moment. Per spec, a pseudo-element selector cannot be followed by anything, except a few pseudo-classes. It's only when we relax this requirement that it may become a real issue.
> 
> (setting this to P3 for now).

We relaxed it a while back: http://trac.webkit.org/changeset/85077.
Comment 5 Silvia Pfeiffer 2012-03-14 22:16:39 PDT
I have an issue with nested shadow DOMs and selectors.

I'm working on the video controls. They are built as a shadow dom. The transport bar and the volume slider are implemented as input[type"range"] elements.

Now I want to style the input element's shadow dom.

In theory, I'd like to do the following:

video::-webkit-media-controls  input[type="range"]::-webkit-slider-container {
    border: 1px solid rgb(130, 130, 130);
    border-radius: 5px;
}
Comment 6 Silvia Pfeiffer 2012-03-14 22:33:18 PDT
Hmm, that was incomplete.

Instead of the above, I have to do:

input[type="range"]::-webkit-slider-container {
    border: 1px solid rgb(130, 130, 130);
    border-radius: 5px;
}

otherwise I get no changes to the sliders.
Comment 7 Roland Steiner 2012-03-15 00:05:01 PDT
(In reply to comment #5)
> video::-webkit-media-controls  input[type="range"]::-webkit-slider-container {

A descendant combinator after a pseudo-element - I understand what you're attempting to do, but this is quite hairy from a spec point of view. Have you tried

video::-webkit-media-controls::-webkit-slider-container

Or even

video::-webkit-slider-container

? If those don't work either, then finding a solution for something like this (even with code changes) would go over much smoother IMHO.
Comment 8 Silvia Pfeiffer 2012-03-15 19:23:10 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > video::-webkit-media-controls  input[type="range"]::-webkit-slider-container {
> 
> A descendant combinator after a pseudo-element - I understand what you're attempting to do, but this is quite hairy from a spec point of view.

OK, I am just trying to find something that works.

> Have you tried
> 
> video::-webkit-media-controls::-webkit-slider-container

This is what I tried first and it doesn't work.


> Or even
> 
> video::-webkit-slider-container

Again, this doesn't work.

Also, I cannot address individual sliders differently in this way. For example, I have a slider for the timeline and a slider for the volume. If I wanted to style them differently, it wouldn't be possible. (Thank god that's not currently required, but I can see it coming...)


> ? If those don't work either, then finding a solution for something like this (even with code changes) would go over much smoother IMHO.

I'm open for any suggestions. Other than hard coding media controls knowledge into the shadow dom of the input element, I couldn't see a solution. I don't think hard coding is the right approach.
Comment 9 Roland Steiner 2012-03-15 22:28:45 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Have you tried
> > 
> > video::-webkit-media-controls::-webkit-slider-container
> 
> This is what I tried first and it doesn't work.

Hm, I have a hunch why, but could you please post a minimal test case so that we can look at the issue in detail?

> Also, I cannot address individual sliders differently in this way. For example, I have a slider for the timeline and a slider for the volume. If I wanted to style them differently, it wouldn't be possible. (Thank god that's not currently required, but I can see it coming...)

Well, just use 3 levels of pseudo-elements: volume::-webkit-media-controls::-webkit-volume-control::-webkit-slider-container ... ;) (just kidding!)

The problem in any case is that all of this requires more or less knowledge of the <volume>'s internal structure. The way I think we really want to take this is to use CSS Variables. You'd write

video {
    data-webkit-volume-control-border-width: 1px;
    data-webkit-volume-control-border-color: rgb(130, 130, 130);
    data-webkit-volume-control-border-radius: 5px;
}

and inside the shadow DOM you'd use 

<style scoped>
    #volume::-webkit-slider-container {
        border: data(webkit-volume-control-border-width, 0px) solid data(webkit-volume-control-border-color, white);
        border-radius: data(webkit-volume-control-border-radius, 0px);
    }
</style>

or even do another stage of handing variables off:

<style scoped>
    #volume {
        data-webkit-slider-border-color: data(webkit-volume-control-border-color, white);
        data-webkit-slider-border-width: data(webkit-volume-control-border-width, 0px);
        data-webkit-slider-border-radius: data(webkit-volume-control-border-radius, 0px);
    }
</style>

where the shadow DOM within <input type="range"> uses the variables similarly to set the border color and radius. But unfortunately all of this isn't ready yet.

> I'm open for any suggestions. Other than hard coding media controls knowledge into the shadow dom of the input element, I couldn't see a solution. I don't think hard coding is the right approach.

Actually, what is the purpose? Is this to allow the user to style the <video> element, or is it for internal styling only? In the latter case a <style scoped> inside the <video> shadow DOM might work, at least for the time being (?).
Comment 10 Silvia Pfeiffer 2012-03-16 00:05:29 PDT
(In reply to comment #9)
> > > video::-webkit-media-controls::-webkit-slider-container
> > 
> > This is what I tried first and it doesn't work.
> 
> Hm, I have a hunch why, but could you please post a minimal test case so that we can look at the issue in detail?


The patch for Chrome that I'm working on is not landed yet, so it's kinda difficult. I'll post a link to it when I'm uploading it to the chrome bug tracker.


> The problem in any case is that all of this requires more or less knowledge of the <volume>'s internal structure. The way I think we really want to take this is to use CSS Variables. You'd write
> 
> video {
>     data-webkit-volume-control-border-width: 1px;
>     data-webkit-volume-control-border-color: rgb(130, 130, 130);
>     data-webkit-volume-control-border-radius: 5px;
> }

> where the shadow DOM within <input type="range"> uses the variables similarly to set the border color and radius. But unfortunately all of this isn't ready yet.


Hmm, these are very long variable names. Also, it's not actually the control's border that I'm setting, but the first shadow child (the container) in the input control.

I thought the shadowPseudoIds were a clever way of addressing the correct shadow element from CSS, but it's just unfortunate that there is only one level of hierarchical depth that we're allowed to use.


> > I'm open for any suggestions. Other than hard coding media controls knowledge into the shadow dom of the input element, I couldn't see a solution. I don't think hard coding is the right approach.
> 
> Actually, what is the purpose? Is this to allow the user to style the <video> element, or is it for internal styling only?


It's for internal styling only right now. User styling is a bonus, but not strictly necessary for my Chrome video controls styling update.

> In the latter case a <style scoped> inside the <video> shadow DOM might work, at least for the time being (?).

The shadow dom is coded in C++. I have an external stylesheet that is applied to the shadow dom, so scoping is out of the question IIUC.

If everything else fails, I think I will be able to grab the graphics context and set the CSS properties by hand. It's just that CSS is so much nicer to write than coding CSS rules in C++!
Comment 11 Silvia Pfeiffer 2012-05-09 16:48:53 PDT
The full patch for the updated video controls is now at https://bugs.webkit.org/attachment.cgi?id=140240&action=prettypatch . I'm attaching here just the subpart that will show this bug.
Comment 12 Silvia Pfeiffer 2012-05-09 16:53:47 PDT
Created attachment 141052 [details]
Patch that exposes the bug

Patch that exposes the bug
Comment 13 Silvia Pfeiffer 2012-05-15 19:25:01 PDT
Also see https://code.google.com/p/chromium/issues/detail?id=112508
Comment 14 Hajime Morrita 2012-06-11 02:02:26 PDT
Created attachment 146812 [details]
Patch
Comment 15 Gustavo Noronha (kov) 2012-06-11 02:13:22 PDT
Comment on attachment 146812 [details]
Patch

Attachment 146812 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12946116
Comment 16 Build Bot 2012-06-11 02:27:26 PDT
Comment on attachment 146812 [details]
Patch

Attachment 146812 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12943198
Comment 17 Build Bot 2012-06-11 02:31:17 PDT
Comment on attachment 146812 [details]
Patch

Attachment 146812 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12925909
Comment 18 Dimitri Glazkov (Google) 2012-06-11 09:03:04 PDT
Comment on attachment 146812 [details]
Patch

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

I think you're missing expected file, and the bots are angry.

> Source/WebCore/ChangeLog:10
> +        - updateSpecifiersWithElementName() didn't take nesting into account.
> +          tag history can contain selector entries which isn't marked as ShadowDescendant yet.
> +          such entry can be found by investigating isUnknownPseudoElement().

I am curious when this happens.
Comment 19 Hajime Morrita 2012-06-11 22:50:01 PDT
Created attachment 147010 [details]
Patch
Comment 20 Hajime Morrita 2012-06-11 22:56:16 PDT
Hi Dimitri, thanks for taking a look.

(In reply to comment #18)
> (From update of attachment 146812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146812&action=review
> 
> I think you're missing expected file, and the bots are angry.
> 
> > Source/WebCore/ChangeLog:10
> > +        - updateSpecifiersWithElementName() didn't take nesting into account.
> > +          tag history can contain selector entries which isn't marked as ShadowDescendant yet.
> > +          such entry can be found by investigating isUnknownPseudoElement().
> 
> I am curious when this happens.

Exactly in this case:

 tag::-shadow-id::-nested-shadow-id.

When the parser calls updateSpecifiersWithElementName(), there is a selector chain which looks like:

 [-shadow-id] <-(ShadowDescenDant) - [-nested-shadow-id]

Since [-shadow-id] is the head of the chain, it has no tag history, thus no relationship to it is set.
It set tag name to the selector, instead of creating new one, which creates

 [both tag name and -shadow-id is set] <-(ShadowDescenDant) - [-nested-shadow-id]

instead of 

 [tag] <-(ShadowDescenDant) - [-shadow-id is set] <-(ShadowDescenDant) - [-nested-shadow-id]
Comment 21 Hajime Morrita 2012-06-11 22:59:59 PDT
Created attachment 147012 [details]
Patch
Comment 22 WebKit Review Bot 2012-06-12 18:28:49 PDT
Comment on attachment 147012 [details]
Patch

Clearing flags on attachment: 147012

Committed r120147: <http://trac.webkit.org/changeset/120147>
Comment 23 WebKit Review Bot 2012-06-12 18:28:56 PDT
All reviewed patches have been landed.  Closing bug.