Closed Bug 1356779 Opened 7 years ago Closed 7 years ago

stylo: support prefixed @keyframes rules

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
It seems that we have no automation tests for this, but I will add some test cases in bug 1312918.
I am not sure it's worth supporting @-moz-keyframes as well.
Summary: stylo: support @-webkit-keyframes → stylo: support prefixed @keyframes rules
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review134698

::: layout/style/test/test_vendor_prefix_keyframes.html:121
(Diff revision 1)
> +  if (SpecialPowers.getBoolPref('layout.css.servo.enabled')) {
> +    // FIXME: Bug 1336863: insertRule is something broken in this test case.
> +    return;

Unfortunately this test case fails on stylo because   of bug 1336863, so I added a reftest to check this test case in the third patch.
Comment on attachment 8859854 [details]
Bug 1356779 - Test that there are multiple prefixed/non-prefixed @keyframes for servo.

https://reviewboard.mozilla.org/r/131838/#review134700

::: servo/tests/unit/style/keyframes.rs:288
(Diff revision 1)
> +        );
> +
> +    // @-moz-keyframes does not override ealier non-prefixed one.
> +    let rules = r"
> +        @keyframes      foo { from, to { width: 100px } }
> +        @-moz-keyframes foo { from, to { width: 0px } }

Servo's test-tidy check considers '-moz-keyframes'as  subtraction, so it requires additional spaces around the '-'.  How can we avoid it?
Comment on attachment 8859854 [details]
Bug 1356779 - Test that there are multiple prefixed/non-prefixed @keyframes for servo.

https://reviewboard.mozilla.org/r/131838/#review134700

> Servo's test-tidy check considers '-moz-keyframes'as  subtraction, so it requires additional spaces around the '-'.  How can we avoid it?

I did escape by backslash. I have no other idea.
Comment on attachment 8859853 [details]
Bug 1356779 - A reftest that checks later @keyframes rule overrides ealier @keyframes.

https://reviewboard.mozilla.org/r/131836/#review134718

Do we really need to add this testcase, or is it redundant?  This seems like a simple enough thing that I'd expect it's covered by existing testcases already.

In particular: from a quick glance at mochitests, I think the mochitest "test_animations.html" already covers this -- specifically these redundant "@keyframes kf_cascade2" rules:
https://dxr.mozilla.org/mozilla-central/rev/c0ea5ed7f91a6be996a4a3c5ab25e2cdf6b4377e/layout/style/test/test_animations.html#63-65
...which we test here to be sure that only the last one applies:
https://dxr.mozilla.org/mozilla-central/rev/c0ea5ed7f91a6be996a4a3c5ab25e2cdf6b4377e/layout/style/test/test_animations.html#654-657

::: commit-message-231f3:1
(Diff revision 1)
> +Bug 1356779 - A reftest that checks later @keyframes rule overrides ealier @keyframes. r?dholbert

Typo: s/ealier/earlier/
Attachment #8859853 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 8859853 [details]
> Bug 1356779 - A reftest that checks later @keyframes rule overrides ealier
> @keyframes.
> 
> https://reviewboard.mozilla.org/r/131836/#review134718
> 
> Do we really need to add this testcase, or is it redundant?  This seems like
> a simple enough thing that I'd expect it's covered by existing testcases
> already.

Oh, thanks. I did not realize that. I will drop the reftest.
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review134724

A few editorial nits for now -- I'll look at the substance of the test tomorrow.

::: commit-message-9d59e:1
(Diff revision 1)
> +Bug 1356779 - Test that there are multiple prefixed/non-prefixed @keyframes with the same name. r?dholbert

"Test that there are..." is the wrong phrasing here. That sounds like it's saying "Ensure that there are...", but that's not what you mean.

Please rephrase as:
 "Add test for interaction between..."

::: layout/style/test/mochitest.ini:311
(Diff revision 1)
> +[test_vendor_prefix_keyframes.html]
> +run-if = stylo # bug 1312918
>  [test_video_object_fit.html]

Could we rename this to "test_keyframes_vendor_prefix.html"?  (mentioning "keyframes" first)

That way, it'll sort next to the existing test "test_keyframes_rules.html", which seems nice grouping-wise.

::: layout/style/test/test_vendor_prefix_keyframes.html:122
(Diff revision 1)
> +  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
> +}, 'last prefixed keyframes overrides previous prefixed keyframes');
> +
> +test(function(t) {
> +  if (SpecialPowers.getBoolPref('layout.css.servo.enabled')) {
> +    // FIXME: Bug 1336863: insertRule is something broken in this test case.

"insertRule is something broken" doesn't quite make sense. I think you really want to say
 "insertRule call is broken [...]"
or:
 "something is broken with inserRule [...]"
Attachment #8859853 - Attachment is obsolete: true
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review134724

> "insertRule is something broken" doesn't quite make sense. I think you really want to say
>  "insertRule call is broken [...]"
> or:
>  "something is broken with inserRule [...]"

As you pointed out, we have already this test case in test_animations.html so I could drop this problematic test case in this file. Thanks!
Comment on attachment 8859851 [details]
Bug 1356779 - Support vendor prefix keyframes rule.

https://reviewboard.mozilla.org/r/131832/#review134764

r=me with those, thanks!

::: servo/components/style/stylesheets.rs:1126
(Diff revision 1)
>                  } else {
>                      Err(())
>                  }
>              },
> -            "keyframes" => {
> +            "keyframes" | "-webkit-keyframes" | "-moz-keyframes" => {
> +                let prefix = if name.starts_with("-webkit-") {

This needs to be ascii case insensitive, otherwise you can skip `-WEBKIT-keyframes`, for example.

You can probably move and reuse http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/servo/components/style/gecko/media_queries.rs#343.

::: servo/components/style/stylist.rs:359
(Diff revision 1)
> +                    // * If there is no exisiting rule.
> +                    // * If the existing rule is prefixed rule.
> +                    let needs_insertion =
> +                        keyframes_rule.vendor_prefix.as_ref().map_or(true,
> +                            |_| self.animations.get(&keyframes_rule.name)
> +                                               .map_or(true, |a| a.vendor_prefix.is_some()));

I think this can be slightly clearer. I think the rule is "don't let a prefixed keyframes animation override a non-prefixed one", right?

In that case, perhaps something like:

```
// Don't let a prefixed keyframes animation override a non-prefixed one.
let needs_insertion = keyframes_rule.vendor_prefix.is_none() || self.animations.get(&keyframes_rule.name).map_or(true, |rule| rule.vendor_prefix.is_some());
```

What do you think?
Attachment #8859851 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8859854 [details]
Bug 1356779 - Test that there are multiple prefixed/non-prefixed @keyframes for servo.

https://reviewboard.mozilla.org/r/131838/#review134766

This test looks really complex (using too many apis that can change), so I think we should try to make it a testharness.js test or similar instead of landing a unit test like this. Otherwise it slowly becomes a pain to maintain. What do you think?

::: servo/tests/unit/style/keyframes.rs:232
(Diff revision 3)
>      };
>  
>      assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
>  }
> +
> +fn same_name_keyframes_test(rules: &str,

This is probably ok, though it'd be better to just add a Servo test, don't you think?

Otherwise we need to change this all the time when we change the API of the stylist.

I'd say the tests you add in part two should be enough. Servo also supports testharness.js tests, so perhaps you can just land it on servo too?
Attachment #8859854 - Flags: review?(emilio+bugs)
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review134770

Oh, and please make sure to test the ascii case-insensitivity bug of part one here. Thanks! :)
Comment on attachment 8859851 [details]
Bug 1356779 - Support vendor prefix keyframes rule.

https://reviewboard.mozilla.org/r/131832/#review134764

> This needs to be ascii case insensitive, otherwise you can skip `-WEBKIT-keyframes`, for example.
> 
> You can probably move and reuse http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/servo/components/style/gecko/media_queries.rs#343.

Thanks, fixed locally.

> I think this can be slightly clearer. I think the rule is "don't let a prefixed keyframes animation override a non-prefixed one", right?
> 
> In that case, perhaps something like:
> 
> ```
> // Don't let a prefixed keyframes animation override a non-prefixed one.
> let needs_insertion = keyframes_rule.vendor_prefix.is_none() || self.animations.get(&keyframes_rule.name).map_or(true, |rule| rule.vendor_prefix.is_some());
> ```
> 
> What do you think?

Yes, exactly. That is what I want to do.  Thanks!
Comment on attachment 8859854 [details]
Bug 1356779 - Test that there are multiple prefixed/non-prefixed @keyframes for servo.

https://reviewboard.mozilla.org/r/131838/#review134766

> This is probably ok, though it'd be better to just add a Servo test, don't you think?
> 
> Otherwise we need to change this all the time when we change the API of the stylist.
> 
> I'd say the tests you add in part two should be enough. Servo also supports testharness.js tests, so perhaps you can just land it on servo too?

Yeah, agree. I will drop this. Thanks.
Attachment #8859854 - Attachment is obsolete: true
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review134808

::: layout/style/test/test_keyframes_vendor_prefix.html:64
(Diff revision 3)
> +test(function(t) {
> +  if (isStylo) {
> +    // FIXME: Bug 1336863: insertRule call is broken on stylo.
> +    return;
> +  }
> +
> +  addStyle(t,
> +    { '@-WEBKIT-keyframes anim': 'from,to { color: rgb(0, 255, 0); }' });
> +
> +  var div = addDiv(t, { style: 'animation: anim 100s' });
> +
> +  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
> +}, 'uppercase -webkit- prefix keyframes');

Unfortunately this uppercase test case fails on stylo.  I have no idea what the trigger of bug 1336863.
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review134962

::: commit-message-9d59e:1
(Diff revision 3)
> +Bug 1356779 - Add tests for interaction prefixed and non-prefixed @keyframes rules with the same name. r?dholbert

s/interaction/interaction of/
(or "interaction between")

::: layout/style/test/test_keyframes_vendor_prefix.html:1
(Diff revision 3)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Test for ...</title>

This <title> needs completing. :)

::: layout/style/test/test_keyframes_vendor_prefix.html:70
(Diff revision 3)
> +  addStyle(t,
> +    { '@-WEBKIT-keyframes anim': 'from,to { color: rgb(0, 255, 0); }' });
> +
> +  var div = addDiv(t, { style: 'animation: anim 100s' });
> +
> +  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');

Could you add a testcase like this but with "-webkit-KEYFRAMES", for good measure?
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review134968

::: layout/style/test/test_keyframes_vendor_prefix.html:70
(Diff revision 3)
> +  addStyle(t,
> +    { '@-WEBKIT-keyframes anim': 'from,to { color: rgb(0, 255, 0); }' });
> +

Please add a testcase for plain, not-at-all-capitalized "-webkit-keyframes" and "-moz-keyframes" by themselves, too.

Otherwise, for all we know, those styles could just be rejected by the engine, and we could be trivially "passing" the later prefixed-vs-not-prefixed override-tests -- trivially because the prefixed styles might simply be rejected entirely.

::: layout/style/test/test_keyframes_vendor_prefix.html:93
(Diff revision 3)
> +  if (!isStylo) {
> +    // FIXME: Bug 1312918: later prefixed rule overrides earlier non-prefixed on
> +    // gecko.
> +    return;

Just for extra clarity here:
 s/overrides/incorrectly overrides/

::: layout/style/test/test_keyframes_vendor_prefix.html:106
(Diff revision 3)
> +      '@-webkit-keyframes anim': 'from,to { color: rgb(255, 0, 0); }' });
> +
> +  var div = addDiv(t, { style: 'animation: anim 100s' });
> +
> +  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
> +}, '-webkit-keyframes does not override non-prefix keyframes');

s/does not/should not/, throughout this test.

(Test failure messages should make it clear that they're expressing an *intent*/*expectation*.  Otherwise, if they just state a fact ("does not" / "did not"), it's hard to tell from a test failure log whether the message is describing *something that's supposed to happen* vs. *something that did happen & was unexpected*.)


Also:
s/override/override earlier/, throughout these test-failure-messages in this test.
(The "earlier" is a key part here because that's what would normally make it override if we didn't have this special-case behavior.)

::: layout/style/test/test_keyframes_vendor_prefix.html:109
(Diff revision 3)
> +  if (!isStylo) {
> +    // FIXME: Bug 1312918: later prefixed rule overrides earlier non-prefixed on
> +    // gecko.

As above: s/overrides/incorrectly overrides/ here.

::: layout/style/test/test_keyframes_vendor_prefix.html:132
(Diff revision 3)
> +      '@keyframes anim':      'from,to { color: rgb(0, 255, 0); }' });
> +
> +  var div = addDiv(t, { style: 'animation: anim 100s' });
> +
> +  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
> +}, 'non-prefix keyframes overrides -moz-keyframes');

s/overrides/should override earlier/
...in this message & the following one.

(for similar reasons that I expressed above, RE "should" & "earlier")
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review134968

> Please add a testcase for plain, not-at-all-capitalized "-webkit-keyframes" and "-moz-keyframes" by themselves, too.
> 
> Otherwise, for all we know, those styles could just be rejected by the engine, and we could be trivially "passing" the later prefixed-vs-not-prefixed override-tests -- trivially because the prefixed styles might simply be rejected entirely.

Ah, indeed.  Adding this simple test case noticed me that a single insertRule call is broken on stylo.  It would be helpful to fix bug 1336863. Thanks!

> s/does not/should not/, throughout this test.
> 
> (Test failure messages should make it clear that they're expressing an *intent*/*expectation*.  Otherwise, if they just state a fact ("does not" / "did not"), it's hard to tell from a test failure log whether the message is describing *something that's supposed to happen* vs. *something that did happen & was unexpected*.)
> 
> 
> Also:
> s/override/override earlier/, throughout these test-failure-messages in this test.
> (The "earlier" is a key part here because that's what would normally make it override if we didn't have this special-case behavior.)

Thanks you always for reviewing with detailed explanation!
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review135066

::: layout/style/test/mochitest.ini:227
(Diff revision 4)
> +[test_keyframes_vendor_prefix.html]
> +run-if = stylo # bug 1312918

Do we need this "run-if = stylo" exclusion?

The test itself has a bunch of "if stylo" / "if not-stylo" skipping inside of it, so it seems redundant to also have a stylo exclusion at this level as well.

::: layout/style/test/test_keyframes_vendor_prefix.html:67
(Diff revision 4)
> +test(function(t) {
> +  if (isStylo) {
> +    // FIXME: Bug 1336863: a single insertRule call is broken on stylo.
> +    return;
> +  }
> +
> +  addStyle(t,
> +    { '@-webkit-keyframes anim': 'from,to { color: rgb(0, 255, 0); }' });

I'm still not clear on what this explanation  really means -- and particularly, why we Stylo can run the later "should not override" tests, but can't run this one.

(Also, if the "run-if = stylo" chunk in mochitest.ini sticks around, then this early-return here means this code will never be executed, for now at least. :-/  )

Does the comment mean to say that <style> elements are broken if they only receive a single call to insertRule() -- but that they work correctly if they receive multiple calls?

If that's the case, I'd rather we just hack around the brokenness by adding a "dummy rule" here -- e.g. by starting with a dummy selector like so:
===
addStyle(t,
 { 'dummySelector' : '', // XXX bug 1336863 hackaround
   '@-webkit-keyframes anim': 'from,to { color: rgb(0, 255, 0); }'
===
...so that we can actually run the test instead of skipping it.
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review135066

> Do we need this "run-if = stylo" exclusion?
> 
> The test itself has a bunch of "if stylo" / "if not-stylo" skipping inside of it, so it seems redundant to also have a stylo exclusion at this level as well.

Sorry for confusion.  I did drop this line in updated patch.  I did run this test on gecko, getting 'layout.css.servo.enabled' pref raised an assertion, so I added this 'run-if'. But soon after, I noticed we can avoid the assertion with try block.

> I'm still not clear on what this explanation  really means -- and particularly, why we Stylo can run the later "should not override" tests, but can't run this one.
> 
> (Also, if the "run-if = stylo" chunk in mochitest.ini sticks around, then this early-return here means this code will never be executed, for now at least. :-/  )
> 
> Does the comment mean to say that <style> elements are broken if they only receive a single call to insertRule() -- but that they work correctly if they receive multiple calls?
> 
> If that's the case, I'd rather we just hack around the brokenness by adding a "dummy rule" here -- e.g. by starting with a dummy selector like so:
> ===
> addStyle(t,
>  { 'dummySelector' : '', // XXX bug 1336863 hackaround
>    '@-webkit-keyframes anim': 'from,to { color: rgb(0, 255, 0); }'
> ===
> ...so that we can actually run the test instead of skipping it.

Yes, indeed.  Actually I did try it to confirm that a single insertRule is problematic or not, and I did think to use it in this test case (but actually quit using it).  Yeah, I'd be happy to do this. Thanks!
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8859852 [details]
Bug 1356779 - Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name.

https://reviewboard.mozilla.org/r/131834/#review135072

This looks good. r=me with nits:

::: layout/style/test/test_keyframes_vendor_prefix.html:86
(Diff revision 6)
> +  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
> +}, '-webkit- prefix keyframes');
> +
> +test(function(t) {
> +  addStyle(t,
> +    { 'dummy': '', // XXX same hackaround as above test.

(optional) It might be marginally better to mention bug 1336863 in each of these XXX comments.

XXX "follow up later" comments are much more discoverable & hence more-likely-to-be-addressed if they each mention the corresponding bug number. (Otherwise it's easy to miss some of them when the corresponding bug is resolved.

So I'd suggest that you globally replace the string "same hackaround as above test" with "bug 1336863 hackaround, as above" or something like that.

::: layout/style/test/test_keyframes_vendor_prefix.html:131
(Diff revision 6)
> +  addStyle(t,
> +    { '@keyframes anim':         'from,to { color: rgb(0, 255, 0); }',
> +      '@-webkit-keyframes anim': 'from,to { color: rgb(255, 0, 0); }' });
> +
> +  var div = addDiv(t, { style: 'animation: anim 100s' });
> +
> +  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
> +}, '-webkit-keyframes should not override later non-prefix keyframes');

s/later/earlier/

::: layout/style/test/test_keyframes_vendor_prefix.html:147
(Diff revision 6)
> +  addStyle(t,
> +    { '@keyframes anim':      'from,to { color: rgb(0, 255, 0); }',
> +      '@-moz-keyframes anim': 'from,to { color: rgb(255, 0, 0); }' });
> +
> +  var div = addDiv(t, { style: 'animation: anim 100s' });
> +
> +  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
> +}, '-moz-keyframes should not override later non-prefix keyframes');

s/later/earlier/
Attachment #8859852 - Flags: review?(dholbert) → review+
Attachment #8859851 - Attachment is obsolete: true
Thanks for the review!

https://github.com/servo/servo/pull/16553
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/049d6f1921bd
Add tests for interaction between prefixed and non-prefixed @keyframes rules with the same name. r=dholbert
I understand that Stylo takes priority when everyone allocates their time, but as far as I can tell it was not even considered to add a couple lines to make non-Stylo Servo not parse vendor-prefixed rules. It is potentially as simple as: if ! cfg!(feature = "gecko") && prefix.is_some() { return Err(()) }

Maybe supporting those turns out to be required for web compat and so Servo should support them too. But maybe that only applies to -webkit- and not -moz-? In any case this would merit some discussion/consideration.

Hiro, would you agree to take this on in a follow-up?
Flags: needinfo?(hikezoe)
Yes, I will send a PR for it.
Flags: needinfo?(hikezoe)
https://hg.mozilla.org/mozilla-central/rev/049d6f1921bd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: