1308110 - 'tab-size' should be animatable

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Sebastian Zartner [:sebo]]

According to the CSSWG resolution from 2016-10-05[1] 'tab-size' should be animatable.

'-moz-tab-size' should be changed accordingly.

Sebastian

[1] https://lists.w3.org/Archives/Public/www-style/2016Oct/0068.html

Comment 1

[Thomas Wisniewski]

Attachment: 1308110-make-tab-size-animatable.diff

Here's a candidate patch which builds on the patchset in bug 943918. I use eStyleAnimType_Coord as the animation type for tab-size (like line-height). I also ensure that unitless values are converted to their proper coord size in StyleCoordToValue (by using code similar to what's used in nsTextFrame::ComputeTabWidthAppUnits).

Try seems fine with it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a605484a29739d6aef5b651d017d499e1c62537

Comment 2

[Cameron McCormack (:heycam)]

It's not clear to me that the spec allows interpolating between number and length values, especially since (a) the number value is something that is resolved against the font that gets used, which is not something available at computed value time, (b) that animations interpolate between computed values, and (c) the current "Animation type: length" line in the property definition table doesn't really handle this case (although I think that should say "number or length").

line-height similarly doesn't support animating between number and length values.

Comment 3

[Thomas Wisniewski]

Do you suppose we should just not allow that case for now, or should we raise a spec-issue and wait instead?

Comment 4

[Cameron McCormack (:heycam)]

I think we should just not allow it for now.  Once the spec has been updated to allow <number>, we can check that the animation type line is correct.

Not being able to support animation between numbers/integers and lengths when the number/integer can't be converted to a length at computed value time is a broader issue, so I don't think we need to raise an issue specifically about tab-size here.

Comment 5

[Thomas Wisniewski]

Alright, thanks, I'll try to fix up the patch to account for this ASAP.

Comment 6

[Thomas Wisniewski]

Attachment: 1308110-make-tab-size-animatable.diff

Here's an updated patch that just does the obvious and changes the relevant tests, without trying to be clever about number<->length animations.

Note that the try-run has a failure that needed to be addressed in file_discrete-animations.html, but I don't think it's worth running another try since the fix was trivial and it now passes locally.

Otherwise there were just two unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36f4518db94a7e06343ce4b04c8e4053fbee0d0b

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8815787 [details] [diff] [review]
1308110-make-tab-size-animatable.diff

Review of attachment 8815787 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_transitions_per_property.html
@@ +248,5 @@
>      // test_length_percent_calc_transition.
>      "stroke-width": [ test_length_transition_svg, test_percent_transition,
>                        test_length_clamped_svg, test_percent_clamped ],
> +    "-moz-tab-size": [ test_length_transition_pre, test_number_transition_pre,
> +                     test_length_clamped_pre, test_number_clamped_pre ],

Can we just use non-pre versions of these tests?  Since we're not rendering anything, I'm not sure we need to ensure the element is display:pre or that we use specific round numbers for the test to work.  And we can then use test_float_zeroToOne_transition and test_float_aboveOne_transition rather than add a new test_number_transition.

And very small nit: I would indent this line two more spaces.

Comment 8

[Thomas Wisniewski]

Attachment: 1308110-make-tab-size-animatable.diff

>Can we just use non-pre versions of these tests?

I suppose so, it will pass test_float_zeroToOne_transition, test_float_aboveOne_transition, and test_length_clamped without any pre-specific changes.

Here's a new patch with those two comments addressed; there's not much left to review :)

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8818132 [details] [diff] [review]
1308110-make-tab-size-animatable.diff

Review of attachment 8818132 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Thomas. :-)

Comment 10

[Thomas Wisniewski]

No problem. I guess we might as well check it in.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d274079a9664
make tab-size animatable. r=cam

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing test_discrete-animations.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cc33db5d46c99957c1ba117692b54cf943db78b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=127edcc85d6f83149a4dda9dcf144ae65728cb78
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40516991&repo=mozilla-inbound

[task 2016-12-13T10:51:15.582855Z] 10:51:15     INFO - TEST-START | dom/animation/test/mozilla/test_discrete-animations.html
[task 2016-12-13T10:51:16.699515Z] 10:51:16     INFO - TEST-INFO | started process screentopng
[task 2016-12-13T10:51:18.197007Z] 10:51:18     INFO - TEST-INFO | screentopng: exit 0
[task 2016-12-13T10:51:18.199483Z] 10:51:18     INFO - Buffered messages logged at 10:51:16
[task 2016-12-13T10:51:18.203199Z] 10:51:18     INFO - TEST-PASS | dom/animation/test/mozilla/test_discrete-animations.html |  - : Elided 42 passes or known failures.
[task 2016-12-13T10:51:18.204803Z] 10:51:18     INFO - Buffered messages finished
[task 2016-12-13T10:51:18.206788Z] 10:51:18     INFO - TEST-UNEXPECTED-FAIL | dom/animation/test/mozilla/test_discrete-animations.html | -moz-tab-size should animate between '1' and '5' with linear easing - -moz-tab-size should animate between '1' and '5' with linear easing: assert_equals: The value should be 1 at 499ms expected "1" but got "2.996"
[task 2016-12-13T10:51:18.208396Z] 10:51:18     INFO - Not taking screenshot here: see the one that was previously logged
[task 2016-12-13T10:51:18.210338Z] 10:51:18     INFO - TEST-UNEXPECTED-FAIL | dom/animation/test/mozilla/test_discrete-animations.html | -moz-tab-size should animate between '1' and '5' with effect easing - -moz-tab-size should animate between '1' and '5' with effect easing: assert_equals: The value should be 1 at 940ms expected "1" but got "2.71304"
[task 2016-12-13T10:51:18.212273Z] 10:51:18     INFO - Not taking screenshot here: see the one that was previously logged
[task 2016-12-13T10:51:18.215651Z] 10:51:18     INFO - TEST-UNEXPECTED-FAIL | dom/animation/test/mozilla/test_discrete-animations.html | -moz-tab-size should animate between '1' and '5' with keyframe easing - -moz-tab-size should animate between '1' and '5' with keyframe easing: assert_equals: The value should be 1 at 940ms expected "1" but got "2.71304"

Comment 13

[Thomas Wisniewski]

Attachment: 1308110-make-tab-size-animatable.diff

Ah, I forgot about that one. Sorry for the mix-up, here's a new version that removes the no-longer-accurate tests. Try seems fine with the patch now, so I'm carrying over r+ and re-requesting checkin: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64a217978ea5f46f215c0c807566904b93cf304e

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e969c00317
make tab-size animatable. r=cam

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95e969c00317

Comment 16

[Chris Mills [:cmills]]

I've added a note to say it is now animatable on the reference page, and the Fx53 release notes:

https://developer.mozilla.org/en-US/docs/Web/CSS/tab-size
https://developer.mozilla.org/en-US/Firefox/Releases/53#CSS

I've also made sure it is listed on the list of animatable properties"

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties