Last Comment Bug 841896 - CSSKeyframesRule should have a `appendRule` method, not `insertRule`
: CSSKeyframesRule should have a `appendRule` method, not `insertRule`
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: mozilla21
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-15 13:37 PST by James Long (:jlongster)
Modified: 2013-04-01 06:46 PDT (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rename CSSKeyframesRule.insertRule to appendRule to match spec change. (3.51 KB, patch)
2013-02-15 18:59 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review

Description James Long (:jlongster) 2013-02-15 13:37:45 PST
Currently a CSSKeyframesRule objects in javascript has a `insertRule` method, like a normal CSSRule object. This is wrong according to the spec:

http://www.w3.org/TR/css3-animations/#DOM-CSSKeyframesRule

MDN succinctly displays this confusion: https://developer.mozilla.org/en-US/docs/DOM/CSSKeyframesRule

It lists the methods at the top, where `appendRule` is defined, but then when describing the methods is uses `insertRule`.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-15 13:55:57 PST
This was a spec change based on my feedback: https://dvcs.w3.org/hg/csswg/rev/08c0c83446dd .  I need to go back and go through all the spec changes at some point.
Comment 2 James Long (:jlongster) 2013-02-15 14:05:39 PST
So you mean that Firefox is out-of-date with the spec? It looks like the spec change you mention changes "insertRule" to "appendRule".

Opera seems to be the only browser that uses "appendRule", but unfortunately seems to replace the whole keyframe instead of just appending to it.
Comment 3 Maciej Jaros 2013-02-15 16:29:17 PST
I was thinking about appendChild method and insertBefore and I think this would be more consistent:

// insert and merge styles (same as for += cssText, meaning "color:#333;left:10%" + "color:#999" effectively becomes "color:#999;left:10%")
insertIntoRule(in DOMString keyText, in DOMString cssText);
insertIntoRule(in DOMString keyText, in CSSStyleDeclaration style);

// inserting rules at specific key; replacing upon duplicate key
setRule(in DOMString keyText, in DOMString cssText);
setRule(in DOMString keyText, in CSSStyleDeclaration style);

I think neither insertBefore nore appendRule just doesn't fit in as this is a key based list.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-15 18:59:17 PST
Created attachment 714728 [details] [diff] [review]
Rename CSSKeyframesRule.insertRule to appendRule to match spec change.

Note that this does not change the IID of nsIDOMMozCSSKeyframesRule
since neither the method signature nor semantics have changed; only the
name is different.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-02-15 19:52:20 PST
Comment on attachment 714728 [details] [diff] [review]
Rename CSSKeyframesRule.insertRule to appendRule to match spec change.

r=me

But shouldn't there be a way to insert keyframes in the middle too?
Comment 6 Maciej Jaros 2013-02-15 20:59:48 PST
If there are keys "0%" and "100%" then appendRule with key "50%" should actually insert keyframe in the middle (or set/replace style at 50% to match Presto behavior).
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-15 21:04:55 PST
It appends the rule, but it's still a rule for 50%.  The order of the rules inside @keyframes doesn't actually matter.
Comment 8 Maciej Jaros 2013-02-15 21:10:59 PST
Sorry, I continued the conversation from GitHub and haven't noticed a missing reference :-). We were talking about Opera/Presto implementation here:
https://github.com/jlongster/css-animations.js/issues/4

Opera is interpreting specs as there can be only one keyframe in the list with given key. It doesn't do any merge or anything, but simply replaces the style under given key. Seems legit to me given current spec. Maybe any of you could suggest spec. change (clarification at least)? :-)
Comment 9 Maciej Jaros 2013-02-15 21:11:53 PST
^as if there could be...
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-02-15 21:18:08 PST
> The order of the rules inside @keyframes doesn't actually matter.

Ah, that's the key bit.  Makes sense, then.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-15 21:36:19 PST
(In reply to Maciej Jaros from comment #8)
> Opera is interpreting specs as there can be only one keyframe in the list
> with given key. It doesn't do any merge or anything, but simply replaces the
> style under given key. Seems legit to me given current spec. Maybe any of
> you could suggest spec. change (clarification at least)? :-)

Yes, that's what the current spec says, but I've proposed changing it, as described here:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=21018 , since I think it's very non-intuitive given how the rest of CSS works.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-02-15 21:42:39 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9dac8026003
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-02-16 06:57:17 PST
https://hg.mozilla.org/mozilla-central/rev/b9dac8026003

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