'tab-size' should be animatable

RESOLVED FIXED in Firefox 53

Status

()

enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: sebo, Assigned: wisniewskit)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

()

Attachments

(1 attachment, 3 obsolete attachments)

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
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
Attachment #8804329 - Flags: review?(cam)
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.
Do you suppose we should just not allow that case for now, or should we raise a spec-issue and wait instead?
Flags: needinfo?(cam)
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.
Flags: needinfo?(cam)
Alright, thanks, I'll try to fix up the patch to account for this ASAP.
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
Assignee: nobody → wisniewskit
Attachment #8804329 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8804329 - Flags: review?(cam)
Attachment #8815787 - Flags: review?(cam)
Keywords: dev-doc-needed
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.
Attachment #8815787 - Flags: review?(cam)
>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 :)
Attachment #8818132 - Flags: review?(cam)
Attachment #8815787 - Attachment is obsolete: true
Comment on attachment 8818132 [details] [diff] [review]
1308110-make-tab-size-animatable.diff

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

Thanks Thomas. :-)
Attachment #8818132 - Flags: review?(cam) → review+
No problem. I guess we might as well check it in.
Keywords: checkin-needed
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"
Flags: needinfo?(wisniewskit)
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
Attachment #8818132 - Attachment is obsolete: true
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/95e969c00317
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
You need to log in before you can comment on or make changes to this bug.