978648 - Change to keyframes' name does not affect current elements

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

Attachment: a simple testcase

Changing the name of keyframes rules has no effect on the existing elements immediately.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

This seems not to be very important for this case as few people will change the name of keyframes. But I hope some people could fix this bug and show me the way to process the dynamic change of this kind of at-rules, so that I could have an idea about how to implement the same function for bug 966166.

Comment 2

[Boris Zbarsky [:bzbarsky]]

David, what does the spec say here?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://dev.w3.org/csswg/css-animations/#animations says:

Once an animation has started it continues until it ends or the ‘animation-name’ is removed. The values used for the keyframes and animation properties are snapshotted at the time the animation starts. Changing them during the execution of the animation has no effect. Note also that changing the value of ‘animation-name’ does not necessarily restart an animation (e.g., if a list of animations are applied and one is removed from the list, only that animation will stop; The other animations will continue). In order to restart an animation, it must be removed then reapplied.

But I didn't like that, and pushed for more dynamic behavior.

We agreed to make changes in http://lists.w3.org/Archives/Public/www-style/2012Nov/0261.html but didn't yet agree on all the details, and I don't think any of what we agreed there has yet made it into the spec.

I need to catch up on spec editing at some point, which includes trying to figure out what the state of that message is.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan from comment #1)
> This seems not to be very important for this case as few people will change
> the name of keyframes. But I hope some people could fix this bug and show me
> the way to process the dynamic change of this kind of at-rules, so that I
> could have an idea about how to implement the same function for bug 966166.

If that's your motivation, you'd probably be better off looking at @font-face.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, given what we currently implement, I believe this is a bug, since it takes effect when I change the zoom.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think what's missing here is probably:
 (1) BeginUpdate/EndUpdate pairs
 (2) calls to doc->StyleRuleChanged()
as we do in the BEGIN_MEDIA_CHANGE and END_MEDIA_CHANGE macros.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #4)
> (In reply to Xidorn Quan from comment #1)
> > This seems not to be very important for this case as few people will change
> > the name of keyframes. But I hope some people could fix this bug and show me
> > the way to process the dynamic change of this kind of at-rules, so that I
> > could have an idea about how to implement the same function for bug 966166.
> 
> If that's your motivation, you'd probably be better off looking at
> @font-face.

Unfortunately, although the spec has such API, there is no implementation in the current code. CSSFontFaceRule in Mozilla lacks all the attributes defined in the new spec. See bug 443978.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Please discuss that in the bug on @counter-style.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In some sense, we don't really want to call StyleRuleChanged here, though, since there's nothing in the style data that actually needs rebuilding other than the animation rules (we don't, e.g., need to rerun selector matching).  On the other hand, it's probably a rare case, so it's not worth adding too much code to optimize it.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: Mark the Declaration inside a keyframe rule as immutable when it has been matched

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: Give a keyframe rule a new identity when it changes, so that it gets a new path in the rule tree when the animation manager builds a path in the rule tree with it

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: Handle dynamic changes to @keyframes rules and keyframe rules better

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bz, if you want to look at these patches, you're welcome to, but I didn't want to add more to your review queue.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And, to be clear, what this is fixing isn't really a spec-relevant issue -- the previous behavior was inconsistent in the presence of other style changes or the timing of rule tree GCs.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8384474 [details] [diff] [review]
patch 2:  Give a keyframe rule a new identity when it changes, so that it gets a new path in the rule tree when the animation manager builds a path in the rule tree with it

Actually, these aren't right.  In particular, patch 2 swaps out the identity of an object that's actually exposed to the DOM, not an object that gets wrapped when it's exposed to the DOM.

I'm thinking maybe I should todo() the one test that patches 1 and 2 were needed to fix, for now at least.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(The easiest fix for that problem is to add yet another additional object, but I'd sort of rather not.  The next easiest, which perhaps is due at this point, is to move the thing that implements nsIStyleRule (the source of style data) from the rule object to either the declaration or the compressed data block.)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've split off that one issue into bug 978833, and I'll fix the others here.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Handle dynamic changes to @keyframes rules and keyframe rules better

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8384474 [details] [diff] [review]
patch 2:  Give a keyframe rule a new identity when it changes, so that it gets a new path in the rule tree when the animation manager builds a path in the rule tree with it

These two patches (originally patch 1 and 2) are now associated with bug 978833 rather than this bug.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8384704 [details] [diff] [review]
Handle dynamic changes to @keyframes rules and keyframe rules better

bz, same comment applies as before -- feel free to look if you're interested, but I didn't want to fill up your review queue

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8384704 [details] [diff] [review]
Handle dynamic changes to @keyframes rules and keyframe rules better

I'm quite happy to let Cameron review this.

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8384704 [details] [diff] [review]
Handle dynamic changes to @keyframes rules and keyframe rules better

Review of attachment 8384704 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSRules.cpp
@@ +2398,3 @@
>    nsCSSStyleSheet* sheet = GetStyleSheet();
>    if (sheet) {
>      sheet->SetModifiedByChildRule();

Can you explain why we don't need to call WillDirty, like BEGIN_MEDIA_CHANGE does, given that SetModifiedByChildRule can call DidDirty?

@@ +2429,3 @@
>    // Be careful to not assign to an nsAutoPtr if we would be assigning
>    // the thing it already holds.
>    if (aDeclaration != mDeclaration) {

Can we just return early if aDeclaration == mDeclaration, before the MOZ_AUTO_DOC_UPDATE?

@@ +2647,5 @@
>  NS_IMETHODIMP
>  nsCSSKeyframesRule::DeleteRule(const nsAString& aKey)
>  {
> +  nsIDocument* doc = GetDocument();
> +  MOZ_AUTO_DOC_UPDATE(doc, UPDATE_STYLE, true);

Move this into the |if (index != RULE_NOT_FOUND)|?

::: layout/style/test/test_animations.html
@@ +1497,5 @@
>  is(cs.marginTop, "25px", "animation-name list length is the length that matters");
>  done_div();
>  
> +var dyn_sheet_elt = document.createElement("style");
> +document.documentElement.firstElementChild.appendChild(dyn_sheet_elt);

Did you know you can write |document.head|? :)

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #22)
> Can you explain why we don't need to call WillDirty, like BEGIN_MEDIA_CHANGE
> does, given that SetModifiedByChildRule can call DidDirty?

WillDirty is only needed if we haven't already called EnsureUniqueInner, which we must have if we're getting DOM accesses on rules.  (We might want to change the setup there.)

> Can we just return early if aDeclaration == mDeclaration, before the
> MOZ_AUTO_DOC_UPDATE?

Nope.  There was still a change we need to handle, it was just done in-place on the declaration.

> >  nsCSSKeyframesRule::DeleteRule(const nsAString& aKey)
> >  {
> > +  nsIDocument* doc = GetDocument();
> > +  MOZ_AUTO_DOC_UPDATE(doc, UPDATE_STYLE, true);
> 
> Move this into the |if (index != RULE_NOT_FOUND)|?

I guess so.  Hopefully BeginUpdate won't go and mutate style sheets.

> Did you know you can write |document.head|? :)

Ooh.  Fancy.  I'll do that.

You couldn't back in the day:  http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-26809268

Comment 24

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #23)
> (In reply to Cameron McCormack (:heycam) from comment #22)
> > Can you explain why we don't need to call WillDirty, like BEGIN_MEDIA_CHANGE
> > does, given that SetModifiedByChildRule can call DidDirty?
> 
> WillDirty is only needed if we haven't already called EnsureUniqueInner,
> which we must have if we're getting DOM accesses on rules.  (We might want
> to change the setup there.)

I see.

> > Can we just return early if aDeclaration == mDeclaration, before the
> > MOZ_AUTO_DOC_UPDATE?
> 
> Nope.  There was still a change we need to handle, it was just done in-place
> on the declaration.

Ah, for a SetPropertyValue call for example, right.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a34a6a07d71

(see also try run at https://tbpl.mozilla.org/?tree=Try&rev=d7e48cf00e0e)

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a34a6a07d71