Closed Bug 1454162 Opened 6 years ago Closed 6 years ago

Fix cascade order of !important in shadow trees.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

It works backwards to what usually happens, and thus we get it wrong. We need some sort of dynamic cascade level or what not.
Assignee: nobody → emilio
Comment on attachment 8968818 [details]
Bug 1454162: Fix cascade order of !important in Shadow DOM.

https://reviewboard.mozilla.org/r/237552/#review243258

Might be slightly cleaner to have a newtype to wrap the shadow cascade order.  The approach of counting as you go up the shadows looks good to me.  And as you say it might be nice to pack this counter in with the other fields on ADB, but a followup is fine.

::: servo/components/style/applicable_declarations.rs:86
(Diff revision 1)
>      pub source: StyleSource,
>      /// The source order of the block, and the cascade level it belongs to.
>      order_and_level: SourceOrderAndCascadeLevel,
>      /// The specificity of the selector this block is represented by.
>      pub specificity: u32,
> +    /// The order in the tree of trees we carry on.

"tree of trees" is a term I haven't seen before, and I'm not really sure what "carry on" means here.

Would it be accurate to say something like:

The number of shadow hosts between the one that contains this block and the one containing the element we are matching, used to ensure we correctly cascade rules from different shadow hosts that match the element.

?

::: servo/components/style/rule_tree/mod.rs:165
(Diff revision 1)
> +/// A counter that increases in reverse direction as you go up the tree of
> +/// trees.

Maybe s/the tree of trees/through each containing shadow host/?

::: servo/components/style/rule_tree/mod.rs:307
(Diff revision 1)
>  
>          if let Some(source) = important_style_attr {
>              current = current.ensure_child(self.root.downgrade(), source, StyleAttributeImportant);
>          }
>  
> +        for mut list in important_shadow.drain().rev() {

I'm looking at https://drafts.csswg.org/css-scoping/#shadow-cascading and https://drafts.csswg.org/css-cascade/#cascading and trying to work out whether this is the correct order, if if the rules from the shadows should come before the style attribute.  But I can't quite tell...

::: servo/components/style/rule_tree/mod.rs:556
(Diff revision 1)
> +    /// We need this to be able to compute the insertion point of style
> +    /// attribute important rules, to avoid selector-matching on style attribute
> +    /// changes. Otherwise this level and the StyleAttribute* levels could
> +    /// pretty much be just AuthorNormal / AuthorImportant.

I guess I don't really understand this.
Attachment #8968818 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #3)
> I'm looking at https://drafts.csswg.org/css-scoping/#shadow-cascading and
> https://drafts.csswg.org/css-cascade/#cascading and trying to work out
> whether this is the correct order, if if the rules from the shadows should
> come before the style attribute.  But I can't quite tell...

Arg, so this made me realize this approach isn't quite right. This is true (and tested) by that test, when the element is actually in the document (that is, host / slotted, generally).

But it's not true if the element is in the shadow tree itself:

<!doctype html>
<div id="host"></div>
<script>
  host.attachShadow({ mode: "open" }).innerHTML = `
    <style>div { color: blue !important; }</style>
    <div style="color: purple !important">Which color am I</div>
  `;
</script>

I guess I'll really need to make these CascadeOrder::Author, and keep the depth around in that byte or something, and tweak the replacement stuff.

> ::: servo/components/style/rule_tree/mod.rs:556
> (Diff revision 1)
> > +    /// We need this to be able to compute the insertion point of style
> > +    /// attribute important rules, to avoid selector-matching on style attribute
> > +    /// changes. Otherwise this level and the StyleAttribute* levels could
> > +    /// pretty much be just AuthorNormal / AuthorImportant.
> 
> I guess I don't really understand this.

So we have the CascadeWithReplacements stuff, which right now always goes up the tree looking at the level and replaces the style attribute rule or inserts it in the right position in the cascade.

That's fine, because right now we know statically which position in the cascade does that rule go in... But that wouldn't be true if we respected this properly. For example:

<!doctype html>
<div id="host" style="color: purple !important"></div>
<script>
  host.attachShadow({ mode: "open" }).innerHTML = `
    <style>:host { color: blue !important; }</style>
  `;
</script>

Should be blue. I'm not really sure how to keep that optimization for this case yet...
Comment on attachment 8968818 [details]
Bug 1454162: Fix cascade order of !important in Shadow DOM.

This is wrong. :(
Attachment #8968818 - Flags: review+ → review-
Comment on attachment 8968818 [details]
Bug 1454162: Fix cascade order of !important in Shadow DOM.

Oh, actually it's pretty close to be right though... Just need some naming changes.
Attachment #8968818 - Flags: review-
Comment on attachment 8968818 [details]
Bug 1454162: Fix cascade order of !important in Shadow DOM.

https://reviewboard.mozilla.org/r/237552/#review243258

> "tree of trees" is a term I haven't seen before, and I'm not really sure what "carry on" means here.
> 
> Would it be accurate to say something like:
> 
> The number of shadow hosts between the one that contains this block and the one containing the element we are matching, used to ensure we correctly cascade rules from different shadow hosts that match the element.
> 
> ?

Hmm, not quite. Hopefully the rewording I've done above should be enough.

> I'm looking at https://drafts.csswg.org/css-scoping/#shadow-cascading and https://drafts.csswg.org/css-cascade/#cascading and trying to work out whether this is the correct order, if if the rules from the shadows should come before the style attribute.  But I can't quite tell...

The trick to fix the case I posted above (and that, at least, a test already caught), is that we don't really want the "Document" / "Shadow" distinction, but "Same tree" / "Different tree". We rely on the different trees being always "inner" shadow trees, which is something that Shadow DOM v0 had, but was removed because it was way too complex.

> I guess I don't really understand this.

Hopefully the new comment explains better.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62928e8af956
Fix cascade order of !important in Shadow DOM. r=heycam
Blocks: 1455032
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e86a47b84bd9
Fix the sizeof tests since they apparently don't run by default. r=me
https://hg.mozilla.org/mozilla-central/rev/62928e8af956
https://hg.mozilla.org/mozilla-central/rev/e86a47b84bd9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Thanks, the comment above the CascadeLevel definition makes sense to me. :-)  Though I still cannot understand the "The order in the tree of trees we carry on." comment.
(In reply to Cameron McCormack (:heycam) from comment #12)
> Thanks, the comment above the CascadeLevel definition makes sense to me. :-)
> Though I still cannot understand the "The order in the tree of trees we
> carry on." comment.

The tree of trees is a term used by the DOM spec to refer to which Light DOM subtree are you on. But I'll try to reword.
You need to log in before you can comment on or make changes to this bug.