Closed Bug 1367283 Opened 7 years ago Closed 7 years ago

stylo: implement discrete type animation for single keyword properties

Categories

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

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

Tracking

()

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

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(1 file, 8 obsolete files)

Bug 1367283 - Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
Implements single keyword properties such 'background-attachment'.
Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8870702 [details]
Bug 1367283 - Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation.

https://reviewboard.mozilla.org/r/142180/#review145854

::: commit-message-98110:3
(Diff revision 1)
> +To test clone_* method of stylo,
> +we test computed value during animation with keyframe which has 'inherit' value.
> +

Could you please explain why these tests are for test_clone_XX in detail?

Other properties don't use test_clone_XX at all?

::: layout/style/test/file_animations_inherit_keyframe_for_discrete.html:66
(Diff revision 1)
> +    const animation = target.animate({ [property.domProp]: ["inherit"] }, 1000);
> +    animation.currentTime = 500;

I think we don't need to open a new window with dom.animations-api.core.enabled if we don't use currentTime here. Can't we use a negative delay?
Comment on attachment 8870701 [details]
Bug 1367283 - Part 6: Implements xul related properties.

https://reviewboard.mozilla.org/r/142178/#review145860
Attachment #8870701 - Flags: review?(hikezoe) → review+
Comment on attachment 8870699 [details]
Bug 1367283 - Part 4: Implements ui related properties.

https://reviewboard.mozilla.org/r/142174/#review145862

Could you please explain why we can't use mako template for -moz-user-select property? (It should be in commit message)
Even if we can't use it, those functions should be simplified with python.

::: servo/components/style/properties/longhand/ui.mako.rs:20
(Diff revision 1)
>                           products="gecko", gecko_ffi_name="mIMEMode",
>                           animation_value_type="discrete",
>                           spec="https://drafts.csswg.org/css-ui/#input-method-editor")}
>  
>  ${helpers.single_keyword("-moz-user-select", "auto text none all element elements" +
>                              " toggle tri-state -moz-all -moz-none -moz-text",

Though it is not related to this patch, property_database.js does not have '-moz-text', could you please check whether it's correct or not. If it's correct, please file a bug to add '-moz-text' in property_database.js.
Comment on attachment 8870698 [details]
Bug 1367283 - Part 3: Implements pointing related properties.

https://reviewboard.mozilla.org/r/142172/#review145866

I don't quite understand why this one and part 6 were split into.
Attachment #8870698 - Flags: review?(hikezoe) → review+
Comment on attachment 8870696 [details]
Bug 1367283 - Part 1: Implements simple properties.

https://reviewboard.mozilla.org/r/142168/#review145868

::: servo/components/style/properties/longhand/inherited_svg.mako.rs:13
(Diff revision 1)
>  // https://www.w3.org/TR/SVG/
>  <% data.new_style_struct("InheritedSVG",
>                           inherited=True,
>                           gecko_name="SVG") %>
>  
>  // TODO(emilio): Should some of these types be animatable?

Can't we drop this comment? Are there still non-animatable properties?

::: servo/components/style/properties/longhand/svg.mako.rs:9
(Diff revision 1)
>  
>  <%namespace name="helpers" file="/helpers.mako.rs" />
>  
>  <% data.new_style_struct("SVG", inherited=False, gecko_name="SVGReset") %>
>  
>  // TODO: Which of these should be animatable properties?

Likewise.
Attachment #8870696 - Flags: review?(hikezoe) → review+
Comment on attachment 8870697 [details]
Bug 1367283 - Part 2: Implements background related properties.

https://reviewboard.mozilla.org/r/142170/#review145872

It seems to me that we should add a new macro to implement these clone methods.

::: servo/components/style/properties/gecko.mako.rs:2806
(Diff revision 1)
>              geckolayer.${field_name} = {
>                  ${caller.body()}
>              };
>          }
>      }
>  </%def>

Nit: a blank line is needed.
Attachment #8870697 - Flags: review?(hikezoe)
Comment on attachment 8870700 [details]
Bug 1367283 - Part 5: Implements svg related properties.

https://reviewboard.mozilla.org/r/142176/#review145884

I think these properties can be written with the new macro intdocued in part 2.
Attachment #8870700 - Flags: review?(hikezoe)
Thank you for your review, Hiro!
I'll check and fix your points.

Also, I'm so sorry, please let me append two more patches before test patch.
* box related properties as patch 7
* position related properties as patch 8
* test patch as patch 9

Thanks.
Comment on attachment 8870699 [details]
Bug 1367283 - Part 4: Implements ui related properties.

https://reviewboard.mozilla.org/r/142174/#review145862

If the property value has "-moz" prefix, it is removed by following macro in gecko_constant.
https://dxr.mozilla.org/mozilla-central/source/servo/components/style/properties/data.py#114

For example, the -moz-user-select property has -moz-all value.
In this case, we want to make:

Keyword::_moz_all => structs::StyleUserSelect::MozAll ,

but got,

Keyword::_moz_all => structs::StyleUserSelect::All ,

But -moz-none case is special.
In this case, we want to remove the prefix:

T::_moz_none => structs::StyleUserSelect::None

Because the struct does not have MozNone value.

> Though it is not related to this patch, property_database.js does not have '-moz-text', could you please check whether it's correct or not. If it's correct, please file a bug to add '-moz-text' in property_database.js.

Oh, okay!
I'll file a bug.
(In reply to Daisuke Akatsuka (:daisuke) from comment #16)
> Comment on attachment 8870699 [details]
> Bug 1367283 - Part 4: Implements ui related properties.
> 
> https://reviewboard.mozilla.org/r/142174/#review145862
> 
> If the property value has "-moz" prefix, it is removed by following macro in
> gecko_constant.
> https://dxr.mozilla.org/mozilla-central/source/servo/components/style/
> properties/data.py#114
> 
> For example, the -moz-user-select property has -moz-all value.
> In this case, we want to make:
> 
> Keyword::_moz_all => structs::StyleUserSelect::MozAll ,
> 
> but got,
> 
> Keyword::_moz_all => structs::StyleUserSelect::All ,
> 
> But -moz-none case is special.
> In this case, we want to remove the prefix:
> 
> T::_moz_none => structs::StyleUserSelect::None
> 
> Because the struct does not have MozNone value.

Can't we specify custom_consts for -moz-none?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #16)
> > Comment on attachment 8870699 [details]
> > Bug 1367283 - Part 4: Implements ui related properties.
> > 
> > https://reviewboard.mozilla.org/r/142174/#review145862
> > 
> > If the property value has "-moz" prefix, it is removed by following macro in
> > gecko_constant.
> > https://dxr.mozilla.org/mozilla-central/source/servo/components/style/
> > properties/data.py#114
> > 
> > For example, the -moz-user-select property has -moz-all value.
> > In this case, we want to make:
> > 
> > Keyword::_moz_all => structs::StyleUserSelect::MozAll ,
> > 
> > but got,
> > 
> > Keyword::_moz_all => structs::StyleUserSelect::All ,
> > 
> > But -moz-none case is special.
> > In this case, we want to remove the prefix:
> > 
> > T::_moz_none => structs::StyleUserSelect::None
> > 
> > Because the struct does not have MozNone value.
> 
> Can't we specify custom_consts for -moz-none?

I see! Will try to use it.
Comment on attachment 8870697 [details]
Bug 1367283 - Part 2: Implements background related properties.

https://reviewboard.mozilla.org/r/142170/#review145872

Do you mean, we should make a macro without any conversion logic manually for each value?
Now, I made a macro "clone_simple_image_array_property" in this patch.
This macro uses the caller.body() which has conversion logic from Gecko value to Servo value.

Your suggestion is, we should not use caller.body() in a new macro?
If it is feasible, 'simple_image_array_property' macro would cover set, copy and clone method.
(In reply to Daisuke Akatsuka (:daisuke) from comment #19)
> Comment on attachment 8870697 [details]
> Bug 1367283 - Part 2: Implements background related properties.
> 
> https://reviewboard.mozilla.org/r/142170/#review145872
> 
> Do you mean, we should make a macro without any conversion logic manually
> for each value?
> Now, I made a macro "clone_simple_image_array_property" in this patch.
> This macro uses the caller.body() which has conversion logic from Gecko
> value to Servo value.

I mean the conversion logic can also be a macro, it's very similar to what we do in impl_keyword_clone(), no?
It's mostly just a string conversion, I think it's a python job.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> It's mostly just a string conversion, I think it's a python job.

Okay thanks.
Comment on attachment 8870702 [details]
Bug 1367283 - Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation.

https://reviewboard.mozilla.org/r/142180/#review145854

> Could you please explain why these tests are for test_clone_XX in detail?
> 
> Other properties don't use test_clone_XX at all?

clone_XX methods are used for two cases.
1. For CSS Transitions[1, 2]
2. To get valid 'inherit' or 'initial'values during animation.

However, we are not supporting discrete animation for CSS Transitions now.
So I'd like to test by second path.
For other non discretre animatable properties, we can test in CSS Transition tests.

[1] https://dxr.mozilla.org/mozilla-central/source/servo/components/style/properties/helpers/animated_properties.mako.rs#378
[2] https://dxr.mozilla.org/mozilla-central/source/servo/components/style/properties/helpers/animated_properties.mako.rs#571
[3] https://dxr.mozilla.org/mozilla-central/source/servo/components/style/properties/helpers/animated_properties.mako.rs#502

> I think we don't need to open a new window with dom.animations-api.core.enabled if we don't use currentTime here. Can't we use a negative delay?

I see! Thanks!
(In reply to Daisuke Akatsuka (:daisuke) from comment #23)
> Comment on attachment 8870702 [details]
> Bug 1367283 - Part 7: Add tests inherited computed value of keyframe's value
> ( to test clone_* method of stylo ).
> 
> https://reviewboard.mozilla.org/r/142180/#review145854
> 
> > Could you please explain why these tests are for test_clone_XX in detail?
> > 
> > Other properties don't use test_clone_XX at all?
> 
> clone_XX methods are used for two cases.
> 1. For CSS Transitions[1, 2]
> 2. To get valid 'inherit' or 'initial'values during animation.
> 
> However, we are not supporting discrete animation for CSS Transitions now.
> So I'd like to test by second path.
> For other non discretre animatable properties, we can test in CSS Transition
> tests.

So if we introduce this in our tree, we have two different test files for discrete type properties. This one and the one in web-platform-test. (actually there is another one for discrete type properties in @keyframes).
Can we keep managing both of them individually in the future?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #23)
> > Comment on attachment 8870702 [details]
> > Bug 1367283 - Part 7: Add tests inherited computed value of keyframe's value
> > ( to test clone_* method of stylo ).
> > 
> > https://reviewboard.mozilla.org/r/142180/#review145854
> > 
> > > Could you please explain why these tests are for test_clone_XX in detail?
> > > 
> > > Other properties don't use test_clone_XX at all?
> > 
> > clone_XX methods are used for two cases.
> > 1. For CSS Transitions[1, 2]
> > 2. To get valid 'inherit' or 'initial'values during animation.
> > 
> > However, we are not supporting discrete animation for CSS Transitions now.
> > So I'd like to test by second path.
> > For other non discretre animatable properties, we can test in CSS Transition
> > tests.
> 
> So if we introduce this in our tree, we have two different test files for
> discrete type properties. This one and the one in web-platform-test.
> (actually there is another one for discrete type properties in @keyframes).
> Can we keep managing both of them individually in the future?

Ah, I see.
But, we should test properties which need -moz prefix (e.g. -moz-user-select).
We could not append those properties into web-platform, right?
Comment on attachment 8871547 [details]
Bug 1367283 - Part 7: Implements position related properties.

https://reviewboard.mozilla.org/r/143028/#review146730

Nice simplified!

::: servo/components/style/properties/gecko.mako.rs
(Diff revision 1)
>      }
>  
> -    pub fn set_box_sizing(&mut self, v: longhands::box_sizing::computed_value::T) {
> -        use computed_values::box_sizing::T;
> -        use gecko_bindings::structs::StyleBoxSizing;
> -        // TODO: guess what to do with box-sizing: padding-box

Please mention as commit message about that box-sizing: padding-box has been removed in bug 1166728.
Attachment #8871547 - Flags: review?(hikezoe) → review+
Comment on attachment 8870699 [details]
Bug 1367283 - Part 4: Implements ui related properties.

https://reviewboard.mozilla.org/r/142174/#review145862

> Oh, okay!
> I'll file a bug.

I filed bug 1367952
Comment on attachment 8871548 [details]
Bug 1367283 - Part 8: Implements box related properties.

https://reviewboard.mozilla.org/r/143030/#review146734

::: servo/components/style/properties/gecko.mako.rs:2141
(Diff revision 1)
> -    ${impl_simple_copy('page_break_after', 'mBreakAfter')}
> +        match self.gecko.mBreak${kind.title()} {
> +            true => T::always,
> +            false => T::auto,
> +        }

I wonder we should leave a comment about bug 24000 as well?
Attachment #8871548 - Flags: review?(hikezoe) → review+
Comment on attachment 8871548 [details]
Bug 1367283 - Part 8: Implements box related properties.

https://reviewboard.mozilla.org/r/143030/#review146734

> I wonder we should leave a comment about bug 24000 as well?

Thanks, leave this one.
Comment on attachment 8870699 [details]
Bug 1367283 - Part 4: Implements ui related properties.

https://reviewboard.mozilla.org/r/142174/#review146738

I was wondering why gecko_ignore_clone_values is necessary there.
I just realized that we should use extra_gecko_aliases instead of custom_consts?
Attachment #8870699 - Flags: review?(hikezoe)
Comment on attachment 8870697 [details]
Bug 1367283 - Part 2: Implements background related properties.

https://reviewboard.mozilla.org/r/142170/#review146744

This looks pretty good to me!
Attachment #8870697 - Flags: review?(hikezoe) → review+
Comment on attachment 8870700 [details]
Bug 1367283 - Part 5: Implements svg related properties.

https://reviewboard.mozilla.org/r/142176/#review146746

Simplified! yay!
Attachment #8870700 - Flags: review?(hikezoe) → review+
Comment on attachment 8870702 [details]
Bug 1367283 - Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation.

https://reviewboard.mozilla.org/r/142180/#review146748

I am reluctant to add this test file only for "-moz-user-select".
How many moz-prefixed properties that are not tested for animations (i.e. including non-discrete properties)?

I think we should a test file that tests for all such properties (not only for discrete type).
test_transition_per_property.html does not test additive or accumulative animations, so I think the new test file should test such animations as well.
(And I guess if we are going to it, the test file should be in dom/animation/test/mozilla.)

What do you think?
Attachment #8870702 - Flags: review?(hikezoe)
Comment on attachment 8871547 [details]
Bug 1367283 - Part 7: Implements position related properties.

https://reviewboard.mozilla.org/r/143028/#review146730

> Please mention as commit message about that box-sizing: padding-box has been removed in bug 1166728.

Yes, thanks!
Comment on attachment 8870702 [details]
Bug 1367283 - Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation.

https://reviewboard.mozilla.org/r/142180/#review146748

Totally 28 animatable properties exist that have "-moz" prefix.
And 21 properties are discrete type which we need to test. Remaining 7 properties are in test_transition_per_property.html.

I get worried a bit we have to re-define and maintain those test values, if we move to under dom/animation/test/mozilla.
OK. If you are going to implement additive or accumulative test cases in the file, either places fine to me.
Comment on attachment 8870702 [details]
Bug 1367283 - Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation.

https://reviewboard.mozilla.org/r/142180/#review146768

I still don't quite understand what the purpose of this test file is.

I thought it's only for moz-prefixed properties, no?  non-prefixed properties should be tested in web-platform tests?
Attachment #8870702 - Flags: review?(hikezoe)
Comment on attachment 8870699 [details]
Bug 1367283 - Part 4: Implements ui related properties.

https://reviewboard.mozilla.org/r/142174/#review146772

::: servo/components/style/properties/helpers.mako.rs:674
(Diff revision 3)
>          }
>      }
>  </%def>
>  
>  <%def name="single_keyword_computed(name, values, vector=False,
> -            extra_specified=None, needs_conversion=False, **kwargs)">
> +            extra_specified=None, needs_conversion=False, gecko_strip_moz_prefix=True, **kwargs)">

Is this default argument really needed?

::: servo/components/style/properties/helpers.mako.rs:682
(Diff revision 3)
>              'gecko_constant_prefix', 'gecko_enum_prefix',
>              'extra_gecko_values', 'extra_servo_values',
>              'aliases', 'extra_gecko_aliases', 'extra_servo_aliases',
>              'custom_consts', 'gecko_inexhaustive',
>          ]}
> +        keyword_kwargs.update({ "gecko_strip_moz_prefix": gecko_strip_moz_prefix })

It seems to me that we just need to add 'gecko_strip_moz_prefix' into the above list?
If we really need this, please explain the reason in commit message!
Attachment #8870699 - Flags: review?(hikezoe)
Comment on attachment 8870702 [details]
Bug 1367283 - Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation.

https://reviewboard.mozilla.org/r/142180/#review146768

Yeah, I wanted to add web-platform tests if possible.

All I want to test clone_XX methods I made.
The clone_XX methods are called for CSS Transitions.
Also called for getting computed value of 'inherit' during animation in Web Animations and CSS Animation.
However, we are not supporting discrete animation for CSS Transitions.
So, I made such the test.

Of course, we can write the same tests for non-prefixed properties in web-platform. Should we do?
(In reply to Daisuke Akatsuka (:daisuke) from comment #54)
> Comment on attachment 8870702 [details]
> Bug 1367283 - Part 9: Add tests computed value of keyframe's 'inherit' value
> ( to test clone_* method of stylo ).
> 
> https://reviewboard.mozilla.org/r/142180/#review146768
> 
> Yeah, I wanted to add web-platform tests if possible.
> 
> All I want to test clone_XX methods I made.
> The clone_XX methods are called for CSS Transitions.
> Also called for getting computed value of 'inherit' during animation in Web
> Animations and CSS Animation.
> However, we are not supporting discrete animation for CSS Transitions.
> So, I made such the test.
> 
> Of course, we can write the same tests for non-prefixed properties in
> web-platform. Should we do?

I think so.  I am fine with that in a later bug though. Anyway, I think we should keep this test file only for moz-prefixed properties, and move test cases into the web-platform tests when the property is unprefixed.
Comment on attachment 8870699 [details]
Bug 1367283 - Part 4: Implements ui related properties.

https://reviewboard.mozilla.org/r/142174/#review146772

> Is this default argument really needed?

If no default value, gecko_strip_moz_prefix is sent to Keyword constructor as None. So the constructor ignores the default value (True)...

> It seems to me that we just need to add 'gecko_strip_moz_prefix' into the above list?
> If we really need this, please explain the reason in commit message!

It is same reason, None is sent.

So, if we get None in this helper, ignore to update the keyword. Otherwise, we update?
Comment on attachment 8870699 [details]
Bug 1367283 - Part 4: Implements ui related properties.

https://reviewboard.mozilla.org/r/142174/#review146802

::: servo/components/style/properties/helpers.mako.rs:674
(Diff revision 3)
>          }
>      }
>  </%def>
>  
>  <%def name="single_keyword_computed(name, values, vector=False,
> -            extra_specified=None, needs_conversion=False, **kwargs)">
> +            extra_specified=None, needs_conversion=False, gecko_strip_moz_prefix=True, **kwargs)">

Thanks for the explanation.
Then unfortunately, I have to stamp r- to this patch.
The way you introduced is somewhat error-prone and will be messey. If someone try to another boolean arguments, we end up adding those arguments there? 
We should somehow integrate those kind of boolean arguments into the conversion (i.e. convertions kwargs to keyword_args).  I haven't look into the conversion closely, but it seems to be broken (or fragile) in the first place?
Attachment #8870699 - Flags: review-
Comment on attachment 8870699 [details]
Bug 1367283 - Part 4: Implements ui related properties.

https://reviewboard.mozilla.org/r/142174/#review146902

::: servo/components/style/properties/helpers.mako.rs:676
(Diff revision 4)
> -        keyword_kwargs = {a: kwargs.pop(a, None) for a in [
> +       keyword_kwargs = dict(filter(
> +           lambda x: not (x[0] is 'gecko_strip_moz_prefix' and x[1] is None),
> +           {a: kwargs.pop(a, None) for a in [
>              'gecko_constant_prefix', 'gecko_enum_prefix',
>              'extra_gecko_values', 'extra_servo_values',
>              'aliases', 'extra_gecko_aliases', 'extra_servo_aliases',
> -            'custom_consts', 'gecko_inexhaustive',
> -        ]}
> +            'custom_consts', 'gecko_inexhaustive', 'gecko_strip_moz_prefix'
> +           ]}.items()))

Hmm, my review comment was somewhat misleading.
I just wanted to categorize those arguments with default values.  Something like this:

  arguments_with_default_value  = {
      None: [
          'gecko_constant_prefix', 'gecko_enum_prefix',
          'extra_gecko_values', 'extra_servo_values',
          'aliases', 'extra_gecko_aliases', 'extra_servo_aliases',
          'custom_consts', 'gecko_inexhaustive',
      ],
      True: [
          'gecko_strip_moz_prefix',
      ],
  }
  keyword_kwargs = {name: kwargs.pop(name, default) for default,names in arguments_with_default_value.iteritems() for name in names}

With this way, if someone try to add another boolean argument that the default value if 'False', he/she could know he will need to add an additional dict member in arguments_with_default_value.

Note that the final line should be formatted.  I am not good at python, I don't know better formatt for the line. Sorry about that.
Attachment #8870699 - Flags: review?(hikezoe) → review+
Comment on attachment 8870702 [details]
Bug 1367283 - Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation.

https://reviewboard.mozilla.org/r/142180/#review146908

If you are going to use this test file for additive or accumulative for moz-prefixed properties, please drop 'discrete' from file name and all coments. The name will be 'test_animations_for_moz_prefixed_properties or something.

Also please double-check this test can be run with dom.animations-api.core.enabled off.

::: layout/style/test/test_animations_inherit_keyframe_for_discrete.html:35
(Diff revision 4)
> +    container.appendChild(target);
> +    document.body.appendChild(container);
> +
> +    container.style[property.domProp] = value;
> +
> +    const animation = target.animate({ [property.domProp]: ["inherit"] },

Should we specify a value at offset 0?  IIRC, missing keyframe handling is still preffed off by default on beta and release channels.
Attachment #8870702 - Flags: review?(hikezoe) → review+
Comment on attachment 8870699 [details]
Bug 1367283 - Part 4: Implements ui related properties.

https://reviewboard.mozilla.org/r/142174/#review146902

> Hmm, my review comment was somewhat misleading.
> I just wanted to categorize those arguments with default values.  Something like this:
> 
>   arguments_with_default_value  = {
>       None: [
>           'gecko_constant_prefix', 'gecko_enum_prefix',
>           'extra_gecko_values', 'extra_servo_values',
>           'aliases', 'extra_gecko_aliases', 'extra_servo_aliases',
>           'custom_consts', 'gecko_inexhaustive',
>       ],
>       True: [
>           'gecko_strip_moz_prefix',
>       ],
>   }
>   keyword_kwargs = {name: kwargs.pop(name, default) for default,names in arguments_with_default_value.iteritems() for name in names}
> 
> With this way, if someone try to add another boolean argument that the default value if 'False', he/she could know he will need to add an additional dict member in arguments_with_default_value.
> 
> Note that the final line should be formatted.  I am not good at python, I don't know better formatt for the line. Sorry about that.

Thank you very much, Hiro!

How about this?
Drop the all keywords that have 'None' value in this helper.( by filter? )
If we do so, we can use primary default values of Keyword constructor in data.py without re-define the default values in here.
Comment on attachment 8870702 [details]
Bug 1367283 - Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation.

https://reviewboard.mozilla.org/r/142180/#review146908

Okay!

> Should we specify a value at offset 0?  IIRC, missing keyframe handling is still preffed off by default on beta and release channels.

Oh, I see. Will specify.
I discussed with Hiro on IRC.

About default value of single_keyword_computed:
I will use the code Hiro suggested since we should define default value at helper.

About tests:
We will append not only CSS Transitions, but CSS Animations and Web Animations tests.
So, dom/animation/test/mozilla is better to fit to those tests.
I will move the test to that location.
Even move, we use the layout/style/property_database.js using support-files in dom/animation/test/mochitest.ini
(In reply to Daisuke Akatsuka (:daisuke) from comment #68)

> About tests:
> We will append not only CSS Transitions, but CSS Animations and Web
> Animations tests.
> So, dom/animation/test/mozilla is better to fit to those tests.
> I will move the test to that location.
> Even move, we use the layout/style/property_database.js using support-files
> in dom/animation/test/mochitest.ini

I sent to try-server without support-files in mochitest.ini.
Also, I loaded the database by 
`<script src="/tests/layout/style/test/property_database.js"></script>`

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acba530f1e3cb564ffec115ca50f1e4fb08dbada&selectedJob=102740890

It looks our new test passed without support-files.
It seems to me that using support-files is a proper way to share a file in different directory.
See the document;
http://gecko.readthedocs.io/en/latest/build/buildsystem/test_manifests.html
I see, thanks!
I discussed with Hiro again.
We drop -moz-appearance and -moz-user-select from this bug.
Because, the structure of -moz-appearance was changed.
Also, for -moz-user-select, we should consider for extra_gecko_aliases tests in servo.
Attachment #8870696 - Attachment is obsolete: true
Attachment #8870697 - Attachment is obsolete: true
Attachment #8870698 - Attachment is obsolete: true
Attachment #8870699 - Attachment is obsolete: true
Attachment #8870700 - Attachment is obsolete: true
Attachment #8870701 - Attachment is obsolete: true
Attachment #8871547 - Attachment is obsolete: true
Attachment #8871548 - Attachment is obsolete: true
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c848143bb85d
Part 9: Add tests to confirm valid 'inherit' value of -moz prefixed properties during animation. r=hiro
https://hg.mozilla.org/mozilla-central/rev/6feb3ac7548f
https://hg.mozilla.org/mozilla-central/rev/c848143bb85d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.