1355732 - stylo: Make column-* animatable

Status: RESOLVED FIXED

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

Description

[Mantaroh Yoshinaga[:mantaroh]]

Next, we will make column-* properties animatable (i.e. column-width / column-count / column-gap).

I think that...
The column-width is LengthOrAuto type, so we can make animatable changing to animation type.
The column-count is IntegerOrAuto type, we will need to implement predefined_types[1].
The column-gap is LengthOrNormal type, we will need to implement Normal interpolate function.

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

Try :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9e82cd830c40a90c468ceae379475fc6092af2


The column-count is different from other properties. We will need to specified this type as predefined_types, however this type is not coordinate_type or length type simple type, so we can't use exist macro(like impl_sytle_coord or imple_simple). [1]

[1] https://dxr.mozilla.org/mozilla-central/rev/abf145ebd05fe105efbc78b761858c34f7690154/servo/components/style/properties/gecko.mako.rs#641

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1355732 part 1 - Make column-* animatable on stylo.

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

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1355732 Enable web platform tests of column-* properties animation.

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8857832 [details]
Bug 1355732 part 1 - Make column-* animatable on stylo.

https://reviewboard.mozilla.org/r/129854/#review132426

There are no clone_column_gap() and clone_column_width(). Does it mean we have no glue codes for thoese properties?

::: servo/components/style/properties/gecko.mako.rs:3507
(Diff revision 1)
>  
>      ${impl_simple_copy('column_count', 'mColumnCount')}
>  
> +    pub fn clone_column_count(&self) -> longhands::column_count::computed_value::T {
> +        use gecko_bindings::structs::NS_STYLE_COLUMN_COUNT_AUTO;
> +        if self.gecko.mColumnCount != NS_STYLE_COLUMN_COUNT_AUTO {

gecko.mColumnCount is uint32_t, whereas Integer is i32. So it's worth adding debug_assert!() to check mColumnCount is less than <i32>::max_value()?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8857833 [details]
Bug 1355732 Enable web platform tests of column-* properties animation.

https://reviewboard.mozilla.org/r/129856/#review132454

Brian told me that getComputedStyle() for 'normal' of column-gap should return 'normal'. So, you don't need to add getNormalValue() function here.  Please file a bug for this issue and let's mark FAIL for now.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:226
(Diff revision 1)
> +  testAddition: function(property, setup) {
> +    test(function(t) {
> +      var idlName = propertyToIDL(property);
> +      var target = createTestElement(t, setup);
> +      target.style[idlName] = 1;
> +      var animation = target.animate({ [idlName]: [0, 2] },

Does this test work as expected?  It seems to me that '0' is not a positive integer.

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857832 [details]
Bug 1355732 part 1 - Make column-* animatable on stylo.

https://reviewboard.mozilla.org/r/129854/#review132426

Thanks.

I believe that the properties which using the predefined_type helper have clone function. This clone function will generate by mako template.[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/servo/components/style/properties/data.py#154

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857833 [details]
Bug 1355732 Enable web platform tests of column-* properties animation.

https://reviewboard.mozilla.org/r/129856/#review132454

Thanks for information.
I filed as bug 1356241.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> Comment on attachment 8857832 [details]
> Bug 1355732 part 1 - Make column-* animatable on stylo.
> 
> https://reviewboard.mozilla.org/r/129854/#review132426
> 
> Thanks.
> 
> I believe that the properties which using the predefined_type helper have
> clone function. This clone function will generate by mako template.[1]

That's really interesting.  We implemented clone_column_count() and column-count is also using predefined_type().  What's the difference?

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #6)
> > Comment on attachment 8857832 [details]
> > Bug 1355732 part 1 - Make column-* animatable on stylo.
> > 
> > https://reviewboard.mozilla.org/r/129854/#review132426
> > 
> > Thanks.
> > 
> > I believe that the properties which using the predefined_type helper have
> > clone function. This clone function will generate by mako template.[1]
> 
> That's really interesting.  We implemented clone_column_count() and
> column-count is also using predefined_type().  What's the difference?

I think that we specified the column-count as skipped longhand[1].

[1] https://dxr.mozilla.org/mozilla-central/rev/ce69b6e1773e9e0d0a190ce899f34b1658e66ca4/servo/components/style/properties/gecko.mako.rs#3586

So I think that this property couldn't generate clone_* function.
If we remove this skip code, we will need to implement IntegerOrAuto types[2].

[2] https://dxr.mozilla.org/mozilla-central/rev/ce69b6e1773e9e0d0a190ce899f34b1658e66ca4/servo/components/style/properties/gecko.mako.rs#673,689

We use this type on z-index and column-count only.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Oh, that's good to know. I did not notice that CoordDataValue implements Clone trait.  We could use it for some other gecko's struct to simplify glues.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8857832 [details]
Bug 1355732 part 1 - Make column-* animatable on stylo.

https://reviewboard.mozilla.org/r/129854/#review133230

r=me with debug_assert().

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857832 [details]
Bug 1355732 part 1 - Make column-* animatable on stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129854/diff/1-2/

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857833 [details]
Bug 1355732 Enable web platform tests of column-* properties animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129856/diff/1-2/

Comment 14

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857832 [details]
Bug 1355732 part 1 - Make column-* animatable on stylo.

https://reviewboard.mozilla.org/r/129854/#review133230

Thank, hiro,
I added the debug_assert.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8857833 [details]
Bug 1355732 Enable web platform tests of column-* properties animation.

https://reviewboard.mozilla.org/r/129856/#review133262

Thanks for doing this!

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js:405
(Diff revision 2)
>      }
>    },
>    'column-width': {
>      // https://drafts.csswg.org/css-multicol/#propdef-column-width
>      types: [ 'length',
> -      { type: 'discrete', options: [ [ 'auto', '1px' ] ] }
> +      { type: 'discrete', options: [ [ 'auto', '100px' ] ] }

I wonder why we can't use '1px' here.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:5
(Diff revision 2)
>  const discreteType = {
>    testInterpolation: function(property, setup, options) {
>      options.forEach(function(keyframes) {
>        var [ from, to ] = keyframes;
> -      test(function(t) {
> -        var idlName = propertyToIDL(property);
> +      var idlName = propertyToIDL(property);

I guess we can move this propertyToIDL() outside of options.forEach()?
That's being said, I'd prefer to do such refactoring in a separate bugs for other types altogether.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:55
(Diff revision 2)
>  
>    testAddition: function(property, setup, options) {
>      options.forEach(function(keyframes) {
>        var [ from, to ] = keyframes;
> -      test(function(t) {
> -        var idlName = propertyToIDL(property);
> +      var idlName = propertyToIDL(property);

Likewise here.

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857832 [details]
Bug 1355732 part 1 - Make column-* animatable on stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129854/diff/2-3/

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857833 [details]
Bug 1355732 Enable web platform tests of column-* properties animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129856/diff/2-3/

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857833 [details]
Bug 1355732 Enable web platform tests of column-* properties animation.

https://reviewboard.mozilla.org/r/129856/#review133262

Thanks, hiro.

Sorry, I forgot adding meta ini file for the additional-per-property.

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

I sent the PR: https://github.com/servo/servo/pull/16494.

Comment 20

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857833 [details]
Bug 1355732 Enable web platform tests of column-* properties animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129856/diff/3-4/

Comment 21

[Mantaroh Yoshinaga[:mantaroh]]

This rebased patch contain an unnecessary file description in MANIFEST.json.

Comment 22

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857833 [details]
Bug 1355732 Enable web platform tests of column-* properties animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129856/diff/4-5/

Comment 23

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s bbefc8b5f404 -d 92f94fc0993b: rebasing 390344:bbefc8b5f404 "Bug 1355732 part 2 - Enable w-p-t of column-* properties. r=hiro" (tip)
merging testing/web-platform/meta/MANIFEST.json
merging testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js
merging testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 24

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8857833 [details]
Bug 1355732 Enable web platform tests of column-* properties animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129856/diff/5-6/

Comment 25

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #24)
> Comment on attachment 8857833 [details]
> Bug 1355732 Enable web platform tests of column-* properties animation.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/129856/diff/5-6/

I rebase this patch and changed the commit message.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/140d33b3fbe2
Enable web platform tests of column-* properties animation. r=hiro

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/140d33b3fbe2

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/932eb823a7d4
Backed out changeset 140d33b3fbe2

Comment 29

[Mantaroh Yoshinaga[:mantaroh]]

Hi Tomcat,

I really appreciate what you've checked this tree result.

I have a question about this back out. This was back out with bug 1290951. Is this back out related with bug 1357437?
In the tree result, it looks passing the web-platform-test (wpt10). [1]

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=2458112439037abae9e72fc0612b75fe7a82344d

Thanks.

Comment 30

[Boris Chiou [:boris]]

Attachment: Servo PR, #16536

I think this is an accident, so create a PR to fix this (column-count).

Comment 31

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #30)
> Created attachment 8859836 [details] [review]
> Servo PR, #16536
> 
> I think this is an accident, so create a PR to fix this (column-count).

Yeah, revision 2 did change the animation_type for coumn-count, but revision 3 didn't.

Comment 32

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Boris Chiou [:boris] from comment #30)
> Created attachment 8859836 [details] [review]
> Servo PR, #16536
> 
> I think this is an accident, so create a PR to fix this (column-count).

Thanks boris,
Sorry, I removed column-count's animation type by mistake when addressing patch.

Comment 33

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1355732 Enable web platform tests of column-* properties animation.

Sorry, I've forgot land this test patch.
I rebased this patch, and try it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f15f8989832a943eef8eb61ec98da439f94f1bc3

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Please drop the checks at 0ms and at 1000ms for interpolation tests. Jeremy already drop them in bug 1359786.
Thanks!

Comment 35

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1355732 Enable web platform tests of column-* properties animation.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #34)
> Please drop the checks at 0ms and at 1000ms for interpolation tests. Jeremy
> already drop them in bug 1359786.
> Thanks!

Thank you for poiting it. I addressed remove the test of 0ms and 100ms.

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

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Would you mind landing the test case?

Comment 37

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1355732 Enable web platform tests of column-* properties animation.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> Would you mind landing the test case?

Oh, sorry, I'll land it.
(This patch is conflicted to m-c, so I rebased it.)

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

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d515d9bc0fd
Enable web platform tests of column-* properties animation. r=hiro

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d515d9bc0fd