1312918 - Firefox honors @-webkit-keyframes over earlier @keyframes (but Chrome does not), and this causes regressions

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Justin]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0 SeaMonkey/2.40
Build ID: 20160120202951

Steps to reproduce:

This has been in my webpage for years and worked fine until build 49.  My transform translate works fine still.  Static rotate works.  I use Windows machines.

    <style>
    .anim
    {
    animation-name:fieldsetrotate;
    animation-duration: 4s;
    animation-delay: .75s;
    }
    @keyframes fieldsetrotate
    {
    0%   {transform:rotateX(0deg);}
    100% {transform:rotateX(360deg);}
    }
    </style>
    <div class="anim">
        my rotating text div
    </div>


Actual results:

No animation - nothing happens.


Expected results:

The div should animate rotation.

Comment 1

[Loic]

Attachment: 1312918.html

Comment 2

[Loic]

I see the rotation in FF49. What's exactly the issue?

Comment 3

[Justin]

Sorry about that.  It looks like I over-simplified the code.  I took your code and it does work.  Compare to below that does NOT work.  I pinpointed it down to the denoted closing curly bracket.  The below works pre-49.  Did I have malformed code?  Why does a missing bracket make it work?

<style>
.anim
{
animation-name:fieldsetrotate;
animation-duration: 5s;
animation-delay: 1s;  /* was 7 when on builds field */
-webkit-animation:fieldsetrotate 5s .5s; /* Safari and Chrome */
}

@keyframes fieldsetrotate
{
0%   {transform:rotateX(0deg);}
100% {transform:rotateX(360deg);}
}  /* DELETE THIS BRACKET and it works */

@-webkit-keyframes fieldsetrotate /* Safari and Chrome */
{
0%   {-webkit-transform:rotateX(0deg);}
50%   {-webkit-transform:rotateX(0deg);}
51% {-webkit-transform:rotateY(360deg);}
100% {-webkit-transform:rotateY(360deg);}
}
</style>

Comment 4

[Mike Taylor [:miketaylr]]

Attachment: test-busted.html

Here's a TC with the described issue.

Comment 5

[Mike Taylor [:miketaylr]]

So this is weird. If I flip the order of the prefixed animations, it works as expected:

@-webkit-keyframes fieldsetrotate /* Safari and Chrome */
{
0%   {-webkit-transform:rotateX(0deg);}
50%   {-webkit-transform:rotateX(0deg);}
51% {-webkit-transform:rotateY(360deg);}
100% {-webkit-transform:rotateY(360deg);}
}

@keyframes fieldsetrotate
{
0%   {transform:rotateX(0deg);}
100% {transform:rotateX(360deg);}
}

There's something about the @-webkit-animation that we don't like, I'm not sure what.

Justin, if this is affecting a site you work on, I'd recommend putting unprefixed after prefixed (which is the right way to do things generally).

Comment 6

[Mike Taylor [:miketaylr]]

As for:

@keyframes fieldsetrotate
{
0%   {transform:rotateX(0deg);}
100% {transform:rotateX(360deg);}
}  /* DELETE THIS BRACKET and it works */

I'd assume we do some kind of error recovery w/ a missing end-bracket and it puts us in a different state somehow.

Daniel, do you know who the best person to look at CSS animations is?

Comment 7

[Mike Taylor [:miketaylr]]

Attachment: tc-working-with-missing-end-bracket

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Mike Taylor [:miketaylr] from comment #6)
> As for:
> @keyframes fieldsetrotate
> {
> [...]
> }  /* DELETE THIS BRACKET and it works */
> 
> I'd assume we do some kind of error recovery w/ a missing end-bracket and it
> puts us in a different state somehow.

Yeah, that DELETE THIS BRACKET thing is kind of a red herring.  If we run into an unexpected character inside of a braced/parenthesized expression (the "@" on "@-webkit") in this case, when you've deleted that bracket),  we'll effectively skip everything until we hit the next (balanced) close-bracket, or the end of the stream.  And all brackets after that point are balanced {...} so we never find a close-bracket, so we end up skipping all the remaining CSS (including the broken @-webkit-keyframes expression).  So we honor the @keyframes because it's the only thing we saw.

The "key" (ha!) difference here seems to be that Chrome gives priority to @keyframes over @-webkit-keyframes, regardless of the order. Firefox honors whichever (valid) one comes last, following standard CSS behavior.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (pulsing colors)

This testcase demonstrates what I said above.
* Chrome honors the unprefixed @keyframes expression, if it's present, regardless of ordering, and it paints the text green as a result.
* Firefox honors the latest @keyframes/@-webkit-keyframes expression (i.e. it honors the ordering), and it paints the text red as a result.

Comment 10

[Mike Taylor [:miketaylr]]

Edge agrees with us (red text in testcase 3), which we might take as indirect evidence that top sites don't rely on this Chrome quirk. I don't think we want to ignore the cascade here. I'm inclined to think this bug is INVALID -- in case the OP isn't reading bugmail I'm going to send him an email and explain what he can do to fix the bug.

Comment 11

[Daniel Holbert [:dholbert]]

If we wanted to fix this, we'd need to adjust this code...
https://dxr.mozilla.org/mozilla-central/rev/944cb0fd05526894fcd90fbe7d1e625ee53cd73d/layout/style/nsCSSParser.cpp#3326-3332
...to use a different parsing function depending on whether mToken.mIdent is exactly "keyframes".  And when we parse a (maybe-prefixed) keyframes rule, we'd need to store some state about whether it was prefixed or not. And then we'd only honor new prefixed rules if there wasn't already an earlier unprefixed rule of the same name.

I think this wouldn't actually be too hard to fix, so I'm inclined to leave this open.  Maybe hiro (or someone else on birtles' team) would be interested in taking this?

Comment 12

[Mike Taylor [:miketaylr]]

(I won't close the bug, I'll let Layout folks work that out -- I did email the original reporter and let him know how he can fix his specific web page -- thanks!)

Comment 13

[Mike Taylor [:miketaylr]]

Although if we do decide to fix this, can someone please file a bug @ https://github.com/whatwg/compat/issues/new so we can make sure to document it?

Comment 14

[Daniel Holbert [:dholbert]]

Sure, I'll do that now.

I think this is the sort of thing we should fix.  It's a case where our webkit CSS support is making us start honoring some [potentially-broken] legacy CSS that Chrome/Safari simply ignore.  And that's bad.

We do want to make a best-effort to honor the same legacy CSS that Chrome/Safari honor (and if we don't do a perfect job with that CSS, that's OK in some cases).  But we *really* don't want to be honoring *extra* legacy CSS above what they're honoring.

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

There's agreed-upon but as-yet unspecced behavior for cascading @keyframes rules. i.e. you could do:

@keyframes abc {
  50% { transform: ... }
}
@keyframes abc {
  50% { opacity: ... }
}

And the two would get spliced together (with later keyframes overriding earlier of properties overlapped).

Or at least, I think that was the intent. I'm on very flaky hotel wifi right now so I'm having trouble looking it up but the issue is [1], initial proposal [2].

Presumably @-webkit-keyframes would cascade the same way as @keyframes?

My intention was to implement and spec at the same time (i.e. once I understand exactly how it should work) which might happen as part of the Stylo work. As far as I can see we don't have a bug for that yet.

[1] https://github.com/w3c/csswg-drafts/issues/71
[2] https://lists.w3.org/Archives/Public/www-style/2015Jul/0391.html

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Overrides vendor prefix keyframes rules with the same name

I am not sure we should wait for works that Brian commented in comment 15. FWIW, I am posting a patch that I wrote yesterday.  The test case in this patch is incomplete because of bug 1313716.
In this patch -moz-keyframes overrides -webkit-keyframes too.

Comment 17

[Justin]

I confirmed that reordering the CSS as Mike suggested works.  I appreciate the help with this one!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Is this patch ready for review?

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Is this patch ready for review?

Not yet.  It needs more test cases at least.
Also it's not clear to me whether -moz-keyframes should override -webkit-keyframes or not.  From the point of view of interoperability, it should not?  Daniel?

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> Also it's not clear to me whether -moz-keyframes should override
> -webkit-keyframes or not.  From the point of view of interoperability, it
> should not?  Daniel?

I'm not sure it matters -- perhaps it'd be simplest to just treat them the same, and (when both are specified) just give priority to whichever of them comes last? (but honor unrpefixed @keyframes over both prefixed versions, regardless of its ordering)

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> > Also it's not clear to me whether -moz-keyframes should override
> > -webkit-keyframes or not.  From the point of view of interoperability, it
> > should not?  Daniel?
> 
> I'm not sure it matters -- perhaps it'd be simplest to just treat them the
> same, and (when both are specified) just give priority to whichever of them
> comes last? (but honor unrpefixed @keyframes over both prefixed versions,
> regardless of its ordering)

Thanks for the suggestion.  I will revise attachment 8805875 [details] [diff] [review] so.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Hiro, any luck here? Looks like this got lost somehow.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f85d69899c5e1fd390ecc93d9c86f7691bc1f2b9

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1312918 - Do not overrider previous keyframes rule if the previous rule is non-prefixed rule and the new one is prefixed.

Review commit: https://reviewboard.mozilla.org/r/130474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130474/

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1312918 - Skip test_vendor_prefix_keyframes.html for stylo.

We don't yet support testharness.js in stylo-failrues.md (bug 1353218).

Review commit: https://reviewboard.mozilla.org/r/130476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130476/

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8858504 [details]
Bug 1312918 - Do not overrider previous keyframes rule if the previous rule is non-prefixed rule and the new one is prefixed.

https://reviewboard.mozilla.org/r/130474/#review133848

::: commit-message-93798:1
(Diff revision 1)
> +Bug 1312918 - Do not overrider previous keyframes rule if the previous rule is non-prefixed rule and the new one is prefixed. r?dholbert

Three commit message nits:
s/overrider/override/
s/keyframes/@keyframes/   (for clarity)
s/rule is non-prefixed rule/rule is non-prefixed/   (drop second "rule" word)

::: layout/style/nsCSSParser.cpp:4362
(Diff revision 1)
> +template <VendorPrefixType VendorPrefix>
>  bool
>  CSSParserImpl::ParseKeyframesRule(RuleAppendFunc aAppendFunc, void* aData)

Is there a reason you're doing this using a template rather than just a new parameter to this function?  

It looks like we only use this variable once, as an argument to a constructor a little ways down here.

::: layout/style/nsCSSRuleProcessor.cpp:3841
(Diff revision 1)
> +            // Do not override the previous rule if the previous rule is
> +            // non-vendor and the new rule is prefixed rules.

s/non-vendor/non-prefixed/
s/rule is prefixed rules/rule is prefixed/

::: layout/style/nsCSSRules.h:329
(Diff revision 1)
> +  // This enum is ordered by priority. None < Mozilla < WebKit.
> +  enum class VendorPrefixType : uint8_t {
> +    None = 0,
> +    Mozilla = 1,
> +    WebKit = 1,
> +  };

A few things:
 - Please add a bit more detail to the comment here (e.g. another sentence) to explain what "ordered by priority" actually means here, in terms of actual CSS rules.  (And do we care about the Mozilla/WebKit ordering or not?)

 - Mozilla and WebKit have the same numeric value here! That looks like a bug (and disagrees with "Mozilla < WebKit" in the documentation). I suspect it kind of works out, though, since it looks like the relative values of these enums don't matter.  So maybe really we really only want two possible enum-values here, maybe named "NoPrefix" and "WithPrefix"?  (And we don't need to distinguish Mozilla/WebKit?)

 - Please include a comment alongside each of the enum values here to indicate what CSS they actually correspond to. (In particular, the CSS that "Mozilla" corresponds to *does not include the string "Mozilla", so it's good to make that mapping clear.)

So e.g.
  None = 0,    // @keyframes
  Mozilla = 1, // @-moz-keyframes
  WebKit = 2,  // @-webkit-keyframes
...or:
  NoPrefix = 0,   // @keyframes
  WithPrefix = 1, // @-moz-keyframes or @-webkit-keyframes

::: layout/style/test/test_vendor_prefix_keyframes.html:9
(Diff revision 1)
> +<script src='/resources/testharness.js'></script>
> +<script src='/resources/testharnessreport.js'></script>
> +<div id='log'></div>
> +<script>
> +/**
> + * Appends a style div to the document head.

s/style div/style element/

::: layout/style/test/test_vendor_prefix_keyframes.html:56
(Diff revision 1)
> +      if (div.parentNode) {
> +        div.parentNode.removeChild(div);
> +      }

You can replace these 3 lines with just:
  div.remove()

That's what you use in the other helper-function (addStyle) actually -- so, a win for both brevity & consistency! :)

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Thank you Daniel for reviewing.
I decided to fix this kind of issue for stylo first in bug 1356779, and then revisit to fix for gecko later.
I'd like to ask you to review test codes in that bug.
Thanks!

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Hiro, so, would this be fixed in 57 when we release stylo? Or is there any point in also fixing it in Gecko, for 56 or 57?

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Yes, we've already fixed this issue for stylo, but not yet for gecko.  I am not sure when I can take time for gecko, it depends on how much work remains for stylo.  The remaining work will appear clearly when stylo is enabled by default on nightly.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Closing since this bug is specific for the old style system.