@keyframes doesn't work inside shadow dom

RESOLVED FIXED in Firefox 61

Status

()

RESOLVED FIXED
5 years ago
7 months ago

People

(Reporter: cwiiis, Assigned: emilio)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla61
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
I don't know whether this bug is the result of the shadow dom, the @import or the combination of the two, but @keyframes rules go missing in this context.

Discovered while adding CSS animations to Gaia shared components.
Probably shadow dom + @keyframes, since we don't build separate dictionaries of available @keyframes rules for subtrees (and I'm not sure we should).
Yeah, the interaction of shadow dom and stuff like @keyframes is somewhere between "underdefined" and "sadfaces" in the specs...
(Reporter)

Updated

5 years ago
See Also: → bug 1009530
Blocks: 811542
(Reporter)

Comment 3

4 years ago
I don't suppose there's any movement on this? From a developer perspective, it's very obvious what you want to happen...

@keyframes specified in a shadow dom should work, and should be scoped to that shadow dom.
Scoped to that shadow DOM in what way?  Scoped to the animation-name declaration also coming from a stylesheet in the same shadow DOM?  Scoped to the animation-name declaration being specified on an element in that shadow DOM?
(Reporter)

Comment 5

4 years ago
(In reply to Not doing reviews right now from comment #4)
> Scoped to that shadow DOM in what way?  Scoped to the animation-name
> declaration also coming from a stylesheet in the same shadow DOM?  Scoped to
> the animation-name declaration being specified on an element in that shadow
> DOM?

Not sure what you mean by the latter one, but the former - scoped in the same way that a @keyframes in a stylesheet in an iframe would be scoped to the document inside that iframe, @keyframes in a stylesheet included in a shadow DOM ought to apply in that shadow DOM.

The one thing they definitely shouldn't do is just nothing (the current behaviour).
(Reporter)

Comment 6

4 years ago
Or at least, if they're going to do nothing, we need a big fat error output in the console so people don't scratch their heads over it (but I really think the expected behaviour should happen. The amount that this gets worked around in Gaia makes it clear that, at least for Gaia's use of web-components, it's unexpected and undesired behaviour).
> Not sure what you mean by the latter one

Say the shadow host has style="animation-name: foo" and a node in the shadow DOM does style="animation-name: inherit".  Say both the page and the shadow DOM have @keyframes rules with animations named "foo".  Which animation, if any, happens on the node in the shadow DOM and why.

> scoped in the same way that a @keyframes in a stylesheet in an iframe would be scoped to
> the document inside

There is no style inheritance across document boundaries.  There is across shadow DOM boundaries.  So the situations are not at all identical.

> @keyframes in a stylesheet included in a shadow DOM ought to apply in that shadow DOM.

The hard part is deciding what "apply in that shadow DOM" actually means.

> The one thing they definitely shouldn't do is just nothing (the current behaviour).

The reason they do nothing right now is because people haven't figured out what they _should_ do and we're sort of trying to actually standardize as we implement, not implement and then worry about standardizing.

> we need a big fat error output in the console 

_That_ we should definitely have.
(Reporter)

Comment 8

4 years ago
(In reply to Not doing reviews right now from comment #7)
> > Not sure what you mean by the latter one
> 
> Say the shadow host has style="animation-name: foo" and a node in the shadow
> DOM does style="animation-name: inherit".  Say both the page and the shadow
> DOM have @keyframes rules with animations named "foo".  Which animation, if
> any, happens on the node in the shadow DOM and why.

Interesting example, but I'd expect the node in the shadow DOM gets the @keyframes from the shadow DOM. I can see how an argument would work both ways, but it's the style property itself I'd see as being inherited, as opposed to the effect of the style property (but I'm not as acutely aware of the spec as I imagine you are, so perhaps this is a bad assumption?)

> > scoped in the same way that a @keyframes in a stylesheet in an iframe would be scoped to
> > the document inside
> 
> There is no style inheritance across document boundaries.  There is across
> shadow DOM boundaries.  So the situations are not at all identical.

True, I hadn't thought of your above case - but I still have an expectation that isn't in line with what happens - even if either outcome happened in the case you present, I'd be ok with that, but the current outcome is probably the least expected.

> > @keyframes in a stylesheet included in a shadow DOM ought to apply in that shadow DOM.
> 
> The hard part is deciding what "apply in that shadow DOM" actually means.
> 
> > The one thing they definitely shouldn't do is just nothing (the current behaviour).
> 
> The reason they do nothing right now is because people haven't figured out
> what they _should_ do and we're sort of trying to actually standardize as we
> implement, not implement and then worry about standardizing.

Understandable, and given the general state of web components (which I imagine is the primary use of shadow DOM?), I guess this isn't so urgent...

But it'd definitely be a nice-to-have, and assuming the above case is the biggest worry, even if animation-name: inherit just plain didn't work (and resulted in an error message), I think that would be fine. I don't think setting animation-name inherit on a node that has no parent with an animation-name in a shadow DOM is common enough behaviour to hold up doing something sort-of-reasonable in the meantime.

> > we need a big fat error output in the console 
> 
> _That_ we should definitely have.

:)
> but I'd expect the node in the shadow DOM gets the @keyframes from the shadow DOM.

OK.  That's what I was asking in comment 4.  So you want to have the scoping follow the element the style is for, not the source of the name.  This gets a bit weird if the page explicitly styles the shadow element with an animation-name instead of it just inheriting it.  This is a bigger worry than the inheritance case, fwiw, since in practice no one is going to do `animation-name: inherit`.
(Reporter)

Comment 10

4 years ago
(In reply to Not doing reviews right now from comment #9)
> > but I'd expect the node in the shadow DOM gets the @keyframes from the shadow DOM.
> 
> OK.  That's what I was asking in comment 4.  So you want to have the scoping
> follow the element the style is for, not the source of the name.  This gets
> a bit weird if the page explicitly styles the shadow element with an
> animation-name instead of it just inheriting it.  This is a bigger worry
> than the inheritance case, fwiw, since in practice no one is going to do
> `animation-name: inherit`.

I think it'd be good to get someone who's done more webdev and components work than me to comment on this - Wilson?

I guess the question is kind of; if a shadow DOM has a conflicting @keyframes name with the host document and the host document sets the animation-name of something in the shadow DOM, which @keyframes should get used? I think it should be the shadow DOM's - you shouldn't really be styling the inside of shadow DOMs anyway, but this feels logical to me (imo it shouldn't matter where the rule came from, or where it was set - the same scope should always apply)
Flags: needinfo?(wilsonpage)
I've run into this gotcha before. From my experience `@keyframe` animation definitions are some of the least reusable things in CSS.

I would like `@keyframe my-animation { ... }` scoping to behave in much the same way `.my-class { ... }` scoping works in Shadow DOM. If component authors want their animations to be overridden, then this is similar territory to Shadow Piercing Combinators, and should probably be discussed in V2.

I'd be interested to here cases whereby inheritance of `@keyframe` is actually useful.
Flags: needinfo?(wilsonpage)
(In reply to Chris Lord [:cwiiis] from comment #10)
> (In reply to Not doing reviews right now from comment #9)
> > > but I'd expect the node in the shadow DOM gets the @keyframes from the shadow DOM.
> > 
> > OK.  That's what I was asking in comment 4.  So you want to have the scoping
> > follow the element the style is for, not the source of the name.  This gets
> > a bit weird if the page explicitly styles the shadow element with an
> > animation-name instead of it just inheriting it.  This is a bigger worry
> > than the inheritance case, fwiw, since in practice no one is going to do
> > `animation-name: inherit`.
> 
> I think it'd be good to get someone who's done more webdev and components
> work than me to comment on this - Wilson?
> 
> I guess the question is kind of; if a shadow DOM has a conflicting
> @keyframes name with the host document and the host document sets the
> animation-name of something in the shadow DOM, which @keyframes should get
> used? I think it should be the shadow DOM's - you shouldn't really be
> styling the inside of shadow DOMs anyway, but this feels logical to me (imo
> it shouldn't matter where the rule came from, or where it was set - the same
> scope should always apply)

Agreed.
Flags: needinfo?(emilio)
(Assignee)

Comment 13

7 months ago
That is https://searchfox.org/mozilla-central/rev/8837610b6c999451435695e800f38d4acbc0a644/layout/style/ServoStyleSet.cpp#1188.

That being said the spec is super handwavy about how this should behave. I guess doing the obvious here should be ok for now.
Blocks: 1405937
(Assignee)

Comment 14

7 months ago
This doesn't have anything to do about @import.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Summary: @keyframes doesn't work inside an @import inside shadow dom → @keyframes doesn't work inside shadow dom
Comment hidden (mozreview-request)
(Assignee)

Comment 16

7 months ago
Hmm, so now interop fun... The two basic tests pass in WebKit and Blink.

Now, however, the tricky parts are ::slotted(..) and :host.

WebKit fixed it properly in https://bugs.webkit.org/show_bug.cgi?id=164608, however that's a lot of complexity. Blink just does something totally broken. Yay.

So, Blink does look at the wrong scope when the animation name is specified and the element is a Shadow Host. That's kinda broken.

That is, the following test:

<!doctype html>
<style>
#host {
  width: 100px;
  height: 100px;
  background: green;
  animation: myanim 10s infinite;
}
</style>
<div id="host"></div>
<script>
  host.attachShadow({ mode: "open" }).innerHTML = `
    <style>
      @keyframes myanim {
        from { background: red; }
        to { background: red; }
      }
    </style>
  `;
</script>


Shows red in Blink. That means that Blink "leaks" the animation name to the outside world just so you can use :host { animation: foo; }.

The right thing to do is stashing the shadow cascade order in the animation name IMO. That's non-trivial though.
(Assignee)

Comment 17

7 months ago
Implementing Blink's hack is trivial, but I'd rather don't, I'll look a bit into the proper fix. Meanwhile this patch should be ok though.
Per IRC discussion, I'm fine with taking the approach in the patch for now, where looking up @keyframes for animations started on element in shadow trees can find @keyframes in that scope, and ignoring issues with :host / ::slotted, until the spec situation gets sorted out.

Comment 19

7 months ago
mozreview-review
Comment on attachment 8972232 [details]
Bug 1018269: Handle Shadow DOM in keyframes lookup.

https://reviewboard.mozilla.org/r/240888/#review246664

::: layout/style/ServoStyleSet.cpp:1185
(Diff revision 1)
>    // TODO(emilio): This may need to look at the element itself for handling
>    // @keyframes properly in Shadow DOM.

Can this comment be dropped now?

::: servo/components/style/stylist.rs:1433
(Diff revision 1)
> +        if let Some(shadow) = element.containing_shadow() {
> +            return shadow.style_data().animations.get(name)
> +        }

In theory we should be also looking at other non-document origins (e.g. from user style sheets, or UA sheets, although of course we shouldn't have any @keyframes rules in UA sheets).

Can you please also add a comment outlining the issue with :host / ::slotted() that we're ignoring here.

::: testing/web-platform/tests/css/css-scoping/keyframes-001.html:21
(Diff revision 1)
> +<script>
> +test(function() {
> +  host.attachShadow({ mode: "open" }).innerHTML = `
> +    <style>
> +      @keyframes myanim {
> +        from { background: red; }

Just make this green?

::: testing/web-platform/tests/css/css-scoping/keyframes-002.html:10
(Diff revision 1)
> +<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
> +<link rel="help" href="https://drafts.csswg.org/css-scoping/#host-selector">
> +<style>
> +@keyframes myanim {
> +  from { background: red; }
> +  to { background: green; }

Just make this red?
Attachment #8972232 - Flags: review?(cam) → review+
(Assignee)

Updated

7 months ago
Blocks: 1458189

Comment 20

7 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69109fe28dc0
Handle Shadow DOM in keyframes lookup. r=heycam
> Blink just does something totally broken.

Blink issue filed?
Flags: needinfo?(emilio)
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10728 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/w3c/web-platform-tests/pull/10728
* continuous-integration/travis-ci/pr (https://travis-ci.org/w3c/web-platform-tests/builds/373480443?utm_source=github_status&utm_medium=notification)
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 26

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/69109fe28dc0
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.