Last Comment Bug 978648 - Change to keyframes' name does not affect current elements
: Change to keyframes' name does not affect current elements
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla30
Assigned To: David Baron :dbaron: ⌚️UTC-8
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-02 19:13 PST by Xidorn Quan [:xidorn] (UTC+10)
Modified: 2014-03-05 04:31 PST (History)
4 users (show)
dbaron: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
a simple testcase (1.21 KB, text/html)
2014-03-02 19:13 PST, Xidorn Quan [:xidorn] (UTC+10)
no flags Details
patch 1: Mark the Declaration inside a keyframe rule as immutable when it has been matched (1.95 KB, patch)
2014-03-03 00:48 PST, David Baron :dbaron: ⌚️UTC-8
no flags Details | Diff | Splinter 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 (6.18 KB, patch)
2014-03-03 00:48 PST, David Baron :dbaron: ⌚️UTC-8
no flags Details | Diff | Splinter Review
patch 3: Handle dynamic changes to @keyframes rules and keyframe rules better (7.66 KB, patch)
2014-03-03 00:48 PST, David Baron :dbaron: ⌚️UTC-8
no flags Details | Diff | Splinter Review
Handle dynamic changes to @keyframes rules and keyframe rules better (7.54 KB, patch)
2014-03-03 09:58 PST, David Baron :dbaron: ⌚️UTC-8
cam: review+
Details | Diff | Splinter Review

Description User image Xidorn Quan [:xidorn] (UTC+10) 2014-03-02 19:13:40 PST
Created attachment 8384402 [details]
a simple testcase

Changing the name of keyframes rules has no effect on the existing elements immediately.
Comment 1 User image Xidorn Quan [:xidorn] (UTC+10) 2014-03-02 19:17:36 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2014-03-02 20:17:45 PST
David, what does the spec say here?
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 2014-03-02 20:34:59 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-02 20:35:44 PST
(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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-02 20:36:57 PST
That said, given what we currently implement, I believe this is a bug, since it takes effect when I change the zoom.
Comment 6 User image David Baron :dbaron: ⌚️UTC-8 2014-03-02 20:49:58 PST
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 User image Xidorn Quan [:xidorn] (UTC+10) 2014-03-02 20:52:51 PST
(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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-02 20:59:37 PST
Please discuss that in the bug on @counter-style.
Comment 9 User image David Baron :dbaron: ⌚️UTC-8 2014-03-02 21:34:28 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 00:48:11 PST
Created attachment 8384473 [details] [diff] [review]
patch 1:  Mark the Declaration inside a keyframe rule as immutable when it has been matched
Comment 11 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 00:48:21 PST
Created 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
Comment 12 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 00:48:29 PST
Created attachment 8384475 [details] [diff] [review]
patch 3:  Handle dynamic changes to @keyframes rules and keyframe rules better
Comment 13 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 00:51:05 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 01:18:33 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 01:49:19 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 01:52:28 PST
(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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 09:29:52 PST
I've split off that one issue into bug 978833, and I'll fix the others here.
Comment 18 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 09:58:57 PST
Created attachment 8384704 [details] [diff] [review]
Handle dynamic changes to @keyframes rules and keyframe rules better
Comment 19 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 10:01:08 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 10:02:22 PST
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 User image Boris Zbarsky [:bz] (still a bit busy) 2014-03-03 14:26:06 PST
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 User image Cameron McCormack (:heycam) (away 25 Feb–5 Mar) 2014-03-03 14:27:33 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2014-03-03 19:25:22 PST
(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 User image Cameron McCormack (:heycam) (away 25 Feb–5 Mar) 2014-03-03 22:26:38 PST
(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 26 User image Carsten Book [:Tomcat] 2014-03-05 04:31:14 PST
https://hg.mozilla.org/mozilla-central/rev/1a34a6a07d71

Note You need to log in before you can comment on or make changes to this bug.