1348519 - [css-grid] Implement animation for grid-template-columns/rows

Status: RESOLVED FIXED

(Core :: CSS Transitions and Animations, enhancement, P3)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase

https://drafts.csswg.org/css-grid/#track-sizing
"
Animatable: 	as a simple list of length, percentage, or calc, provided the only differences are the values of the length, percentage, or calc components in the list 
"

It's worth noting in this context that these properties are kind of
special in that the computed values are the resolved values:
https://drafts.csswg.org/css-grid/#resolved-track-list

Comment 1

[Mats Palmgren (inactive)]

Here's some advice from Brian Birtles (:birtles) (bug 1348500 comment 5):

For the second question about how to implement animation of new types, it basically just involves adding the necessary handling to StyleAnimationValue. I think you might find the implementation of animation of the background-position and background-size properties a helpful reference: https://hg.mozilla.org/mozilla-central/rev/eb947f317c42ec7168e44ace57714af65472cfe6

The trouble with the last point is that we're currently working on replacing StyleAnimationValue as part of the stylo project. As a result, any code you add to StyleAnimationValue will also need to be added to Servo's interpolation functions.

Comment 2

[Mats Palmgren (inactive)]

Still kind of blocked on moving animation to Stylo (bug 1288269).
(b/c it seems like a waste of time to implement it in the old style system
only to have to port/re-implement it in stylo later)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #2)
> Still kind of blocked on moving animation to Stylo (bug 1288269).
> (b/c it seems like a waste of time to implement it in the old style system
> only to have to port/re-implement it in stylo later)

StyleAnimationValue is mostly gone, there's some tasks to clean it up completely, but there would be no need to implement anything there afaict.

Comment 4

[Mats Palmgren (inactive)]

Awesome!  I guess we should try to implement this then. :-)

Comment 5

[Boris Chiou [:boris]]

I think it's time to do this bug. Take this.

Comment 6

[Mats Palmgren (inactive)]

FYI, the "Animation type" link in the Grid spec is wrong,
it should link here:
https://www.w3.org/TR/web-animations-1/#animating-properties
which describes the "by computed value" type.

I think we should probably relax the conditions for when to
fall back to "discrete" animation for these properties.
For example, I think we should try to handle <line-names>
as "discrete" while animating track sizing functions normally,
e.g. "1px [a] 2px" -> "3px [b c] 5px" could just switch
the 2nd component from "[a]" to "[b c]" halfway through.

I also don't see a problem with handling some <track-size>
components as "discrete" and others not.  E.g.
"minmax(1px, 500px)" -> "minmax(100px, max-content)"
could animate the min track-sizing function normally while
animating "500px" -> "max-content" discretely.  IIRC, our
internal computed value always has both a min and a max value
which we can treat as "components" of the overall value.

A mismatch in list length is probably harder to do something
sane for.  Likewise for any "repeat(auto-fit/auto-fill, ...)"
values.  We should probably fall back to discrete animation
of the whole list when that happens, at least in the initial
implementation.

FTR, there's a spec issue where we can give feedback here:
https://github.com/w3c/csswg-drafts/issues/3201

Comment 7

[Boris Chiou [:boris]]

Move this into CSS Transitions and Animations Component.

Comment 8

[Boris Chiou [:boris]]

(In reply to Mats Palmgren (:mats) from comment #6)
> FTR, there's a spec issue where we can give feedback here:
> https://github.com/w3c/csswg-drafts/issues/3201

Good news, the spec issue has been resolved. :)

Comment 9

[Boris Chiou [:boris]]

Attachment: Bug 1348519 - Part 2: Implement Animate for track lists on grid-template-{columns|rows}.

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1348519#c6 and
https://github.com/w3c/csswg-drafts/issues/3201:

Currently grid-template-rows/columns interpolate “per computed value”, which
means that if the number of tracks differs, or any track changes to/from a
particular keyword value to any other value, or if a line name is added/removed
at any position, the entire track listing is interpolated as “discrete”.
But we "agree" with two more granular options:

1. Check interpolation type per track, rather than for the entire list, before
   falling back to discrete. I.e. a length-percentage track can animate between
   two values while an adjacent auto track flips discretely to min-content.
2. Allow discrete interpolation of line name changes independently of track
   sizes.

Besides, for the repeat() function, it's complicated to support interpolation
between different repeat types (i.e. auto-fill, auto-fit) and different repeat
counts, so we always fall-back to discrete if the first parameter of repeat()
is different.

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1348519 - Part 3: Add web platform tests for grid-template-{columns|rows}.

Add wpt for testing interpolation result on grid-template-{columns|rows}.

Depends on D16129

Comment 11

[Brian Birtles (:birtles)]

I will be away from tomorrow so please don't consider my review blocking. The patches look good and the comments I've raised so far could be addressed in follow up bugs if need be.

Comment 12

[Boris Chiou [:boris]]

Thanks, Brian. I will address your comments, and I believe emilio will check my update. :)

Comment 13

[Boris Chiou [:boris]]

Attachment: Bug 1348519 - Part 1: Support field_bound on Animate.

SO we can derive Animate on more generic types.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37838b3650fd
Part 1: Support field_bound on Animate. r=emilio
https://hg.mozilla.org/integration/autoland/rev/bd4efc854419
Part 2: Implement Animate for track lists on grid-template-{columns|rows}. r=emilio
https://hg.mozilla.org/integration/autoland/rev/80dc7df104e6
Part 3: Add web platform tests for grid-template-{columns|rows}. r=mats

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/37838b3650fd
https://hg.mozilla.org/mozilla-central/rev/bd4efc854419
https://hg.mozilla.org/mozilla-central/rev/80dc7df104e6

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14849 for changes under testing/web-platform/tests

Comment 17

[Florin Strugariu [:Bebe]]

Performance improvement:

== Change summary for alert #18716 (as of Sat, 12 Jan 2019 04:02:11 GMT) ==

Improvements:

2% Heap Unclassified windows10-64 pgo stylo 43,202,793.53 -> 42,267,482.74

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=18716

Comment 18

[Chris Mills [:cmills]]

Note to MDN writers — I've added a note to the Fx66 rel notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/66#CSS

To complete this, I think we probably just need to change the animation type data for them in the MDN data repo to something more suitable (they are currently set to discrete), and update the BCD to include a subpoint saying when they are animatable from?