Closed Bug 1355732 Opened 7 years ago Closed 7 years ago

stylo: Make column-* animatable

Categories

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

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

Tracking

()

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

People

(Reporter: mantaroh, Assigned: mantaroh)

References

Details

Attachments

(2 files, 4 obsolete files)

Servo PR, #16536
41 bytes, text/x-github-pull-request
Details | Review
Bug 1355732 Enable web platform tests of column-* properties animation.
9.38 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review 
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.
Priority: -- → P2
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
Assignee: nobody → mantaroh
Blocks: 1353918
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 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 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 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.
(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?
(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.
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 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().
Attachment #8857832 - Flags: review?(hikezoe) → review+
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 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.
Attachment #8857833 - Flags: review?(hikezoe) → review+
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.
Attachment #8857832 - Attachment is obsolete: true
This rebased patch contain an unnecessary file description in MANIFEST.json.
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)
(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.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/140d33b3fbe2
Enable web platform tests of column-* properties animation. r=hiro
https://hg.mozilla.org/mozilla-central/rev/140d33b3fbe2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/932eb823a7d4
Backed out changeset 140d33b3fbe2
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.
Flags: needinfo?(cbook)
Attached file Servo PR, #16536 — 
I think this is an accident, so create a PR to fix this (column-count).
(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.
(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.
Flags: needinfo?(cbook)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, I've forgot land this test patch.
I rebased this patch, and try it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f15f8989832a943eef8eb61ec98da439f94f1bc3
Attachment #8857833 - Attachment is obsolete: true
Attachment #8863289 - Flags: review+
Please drop the checks at 0ms and at 1000ms for interpolation tests. Jeremy already drop them in bug 1359786.
Thanks!
(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
Attachment #8863300 - Flags: review+
Attachment #8863289 - Attachment is obsolete: true
Would you mind landing the test case?
Flags: needinfo?(mantaroh)
(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
Flags: needinfo?(mantaroh)
Attachment #8863635 - Flags: review+
Attachment #8863300 - Attachment is obsolete: true
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d515d9bc0fd
Enable web platform tests of column-* properties animation. r=hiro
https://hg.mozilla.org/mozilla-central/rev/8d515d9bc0fd
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: