Closed Bug 1354437 Opened 6 years ago Closed 6 years ago

stylo: Make border-spacing animatable

Categories

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

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

Tracking

()

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

People

(Reporter: hiro, Assigned: chenpighead)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1354437 - enable border-spacing interpolation test.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
There is no FIXME comment for this property, but it should be animatable too.

Oh interesting. There is an implementation of Interpolate trait for border-spacing, but animatable flag is set to False.
Jeremy, would you like to fix this?
Flags: needinfo?(jeremychen)
Sure, take it from here.
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Flags: needinfo?(jeremychen)
Attachment #8861334 - Flags: review?(boris.chiou)
Thank you for taking this.  Please add some web-platform test case like other making property animatable bugs.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Thank you for taking this.  Please add some web-platform test case like
> other making property animatable bugs.

Oops, I thought we had tests already... :(

I'll write some in a separate patch then. Thank you for the pointers.
Comment on attachment 8861334 [details]
Bug 1354437 - stylo: Make border-spacing animatable.

https://reviewboard.mozilla.org/r/133316/#review136138

LGTM.
Attachment #8861334 - Flags: review?(boris.chiou) → review+
try with this wip: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fb624e3991e, looks like we could reduce couple mochitest failures here (from 145 to 129). :)
Comment on attachment 8861795 [details]
Bug 1354437 - enable border-spacing interpolation test.

Hi Hiro, I did referred to other animation bugs and wrote this WIP. However, I've no idea how to verify locally if the test is indeed enabled. Could you enlighten me a bit about how to get this done? Thanks.
Attachment #8861795 - Flags: feedback?(hikezoe)
You can run testing/web-platform/tests/web-animations/animation-model/animation-types/interpolation-per-property.html locally.
Comment on attachment 8861795 [details]
Bug 1354437 - enable border-spacing interpolation test.

Clear request for now, since I know how to verify the patch and it doesn't pass the test... :(
Attachment #8861795 - Flags: feedback?(hikezoe)
Yeah, actually it's not 'discrete' type animation. We should add lengthPairType or something.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Yeah, actually it's not 'discrete' type animation. We should add
> lengthPairType or something.

Yeah, that's the reason why I need a way to do the local verification, to check if I'm not going to the wrong direction.
I do add a type with exactly same name as you said (high five). Will ask for review very soon!!!! Patch on the way~
Attachment #8861795 - Flags: review?(hikezoe)
Comment on attachment 8861795 [details]
Bug 1354437 - enable border-spacing interpolation test.

https://reviewboard.mozilla.org/r/133798/#review136754

Thanks for quick fix.
Note that positionType is similar to this lengthPair. We can reuse this lengthPair for positionType. (See lengthPercentageOrCals for example).  Please feel free open a bug or fix it in later patch.
Thank you!

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:135
(Diff revision 2)
> +                           [{ time: 0,    expected: '10px 10px' },
> +                            { time: 500,  expected: '30px 30px' },
> +                            { time: 1000, expected: '50px 50px' }]);

In case of interpolation we just need to check at 500ms.

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:146
(Diff revision 2)
> +                           [{ time: 0,    expected: '10px 10px' },
> +                            { time: 500,  expected: '30px 30px' },
> +                            { time: 1000, expected: '50px 50px' }]);

Likewise here.

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

This should be '10px 10px'.

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

Likewise here.
Attachment #8861795 - Flags: review?(hikezoe) → review+
These new test should be passed on normal gecko, please run it locally (or try) before landing.
Blocks: 1359786
Comment on attachment 8861795 [details]
Bug 1354437 - enable border-spacing interpolation test.

https://reviewboard.mozilla.org/r/133798/#review136754

Ok, filed Bug 1359786 for this, so we won't block the stylo work here.

> In case of interpolation we just need to check at 500ms.

Ok, good to know.

> This should be '10px 10px'.

Hmmm, should've got this. Will fix this in the next version.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> These new test should be passed on normal gecko, please run it locally (or
> try) before landing.

Okay, I'll run a more thorough try before landing.
Comment on attachment 8861928 [details]
Bug 1354437 - update mochitest expectations for border-spacing animation support.

https://reviewboard.mozilla.org/r/133940/#review136818

This patch is mainly based on a previous try https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fb624e3991e&selectedJob=93987427.
Note that MANIFEST.json will be conflicted on autoland since bug 1356162 landed.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> Note that MANIFEST.json will be conflicted on autoland since bug 1356162
> landed.

Oops, then I might need to rebase on top of that change... thanks for the notice.
Comment on attachment 8861928 [details]
Bug 1354437 - update mochitest expectations for border-spacing animation support.

https://reviewboard.mozilla.org/r/133940/#review137122

::: layout/style/test/stylo-failures.md:78
(Diff revision 1)
>    * test_bug413958.html `monitorConsole` [3]
>    * test_parser_diagnostics_unprintables.html [550]
>  * Transition support:
>    * test_compute_data_with_start_struct.html `transition` [2]
>    * test_transitions.html: pseudo elements [10]
> -  * test_transitions_computed_value_combinations.html [145]
> +  * test_transitions_computed_value_combinations.html [129]

Thanks for updating this. I just landed a patch to remove this, so I think you can drop this commit after rebasing.
Attachment #8861928 - Flags: review?(boris.chiou) → review+
(In reply to Boris Chiou [:boris] from comment #28)
> Comment on attachment 8861928 [details]
> Bug 1354437 - update mochitest expectations for border-spacing animation
> support.
> 
> https://reviewboard.mozilla.org/r/133940/#review137122
> 
> ::: layout/style/test/stylo-failures.md:78
> (Diff revision 1)
> >    * test_bug413958.html `monitorConsole` [3]
> >    * test_parser_diagnostics_unprintables.html [550]
> >  * Transition support:
> >    * test_compute_data_with_start_struct.html `transition` [2]
> >    * test_transitions.html: pseudo elements [10]
> > -  * test_transitions_computed_value_combinations.html [145]
> > +  * test_transitions_computed_value_combinations.html [129]
> 
> Thanks for updating this. I just landed a patch to remove this, so I think
> you can drop this commit after rebasing.

Nice!!! \o/
Will drop this then.
Attachment #8861334 - Attachment is obsolete: true
Attachment #8861928 - Attachment is obsolete: true
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c1d8fb6bbb76
enable border-spacing interpolation test. r=hiro
https://hg.mozilla.org/mozilla-central/rev/c1d8fb6bbb76
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.