1387951 - stylo: Make interpolation behavior of -moz-image-region with 'auto' value match with gecko

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Chiou [:boris]]

-moz-image-region and clip use ClipRectOrAuto, and I got these failures:

TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | rect-valued property -moz-image-region: interpolation of rects - got "rect(0px, 4px, 4px, 2px)", expected "rect(3px, 13px, 10px, 5px)"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | rect-valued property -moz-image-region: can't interpolate auto components - got "rect(0px, 4px, 4px, 2px)", expected "auto"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | rect-valued property clip: can't interpolate auto components - got "rect(0px, 6px, 4px, auto)", expected "auto"


It seems the interpolation of ClipRectOrAuto is not correct. Besides, ColorOrAuto is also not correct while using "auto":

TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | color-valued property caret-color: not interpolatable between auto and rgb color - got "rgb(0, 0, 0)", expected "rgb(51, 102, 153)"


I remember Either<T, Auto> is a known issue, but still need this bug to track it.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

The test cases that use 'auto' should also be resolved by bug 1355761.

(In reply to Boris Chiou [:boris] from comment #0)
> -moz-image-region and clip use ClipRectOrAuto, and I got these failures:
> 
> TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html
> | rect-valued property -moz-image-region: interpolation of rects - got
> "rect(0px, 4px, 4px, 2px)", expected "rect(3px, 13px, 10px, 5px)"

I have no idea about this test case at first glance.

Comment 2

[Boris Chiou [:boris]]

Thanks for making this dependency. :)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> The test cases that use 'auto' should also be resolved by bug 1355761.
> 
> (In reply to Boris Chiou [:boris] from comment #0)
> > -moz-image-region and clip use ClipRectOrAuto, and I got these failures:
> > 
> > TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html
> > | rect-valued property -moz-image-region: interpolation of rects - got
> > "rect(0px, 4px, 4px, 2px)", expected "rect(3px, 13px, 10px, 5px)"
> 
> I have no idea about this test case at first glance.

File bug 1390067 for this.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> The test cases that use 'auto' should also be resolved by bug 1355761.
> 
> (In reply to Boris Chiou [:boris] from comment #0)
> > -moz-image-region and clip use ClipRectOrAuto, and I got these failures:
> > 
> > TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html
> > | rect-valued property -moz-image-region: interpolation of rects - got
> > "rect(0px, 4px, 4px, 2px)", expected "rect(3px, 13px, 10px, 5px)"
> 
> I have no idea about this test case at first glance.

I am going to reuse this bug for this particular issue.
Other 'auto' problem should be handled in bug 1387939.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=972e7a45c33d81ca79298f8e26142cb32b9f759b

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1387951 - Enable test_discrete-animations.html on stylo.

Review commit: https://reviewboard.mozilla.org/r/168246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168246/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1387951 - Don't convert zero component value to 'auto' in clone__moz_image_region.

Currently Gecko treats 'auto' component value as zero for animation.

Review commit: https://reviewboard.mozilla.org/r/168248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168248/

Comment 8

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8896950 [details]
Bug 1387951 - Enable test_discrete-animations.html on stylo.

https://reviewboard.mozilla.org/r/168246/#review173418

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896951 [details]
Bug 1387951 - Don't convert zero component value to 'auto' in clone__moz_image_region.

https://reviewboard.mozilla.org/r/168248/#review173420

::: layout/style/test/test_transitions_per_property.html:2143
(Diff revision 1)
>      is(cs.getPropertyValue(prop), "rect(0px, 6px, 4px, auto)",
>         "rect-valued property " + prop + ": can't interpolate auto components");
> -    div.style.setProperty(prop, "rect(0px, 6px, 4px, 2px)", "");
> +  } else {
> +    is(prop, "-moz-image-region");
> +    // Bug 1390067: -moz-image-region should behave just like clip does.
> +    is(cs.getPropertyValue(prop), "rect(2.25px, 11.25px, 8.5px, 3.75px)",

We should make this todo_is() and use the same expected value as the above case before landing, but I just wanted to make this change clear what happens currently.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896950 [details]
Bug 1387951 - Enable test_discrete-animations.html on stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168246/diff/1-2/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896951 [details]
Bug 1387951 - Don't convert zero component value to 'auto' in clone__moz_image_region.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168248/diff/1-2/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=972e7a45c33d81ca79298f8e26142cb32b9f759b

Oops, I did push the try based on an old revision.  Also the test case which was added in test_discrete-animations.html relies on a patch for bug 1387939. I am going to drop the case here.

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8896951 [details]
Bug 1387951 - Don't convert zero component value to 'auto' in clone__moz_image_region.

https://reviewboard.mozilla.org/r/168248/#review173752

::: layout/style/test/test_transitions_per_property.html:2146
(Diff revision 2)
> +  if (prop == "clip") {
>      is(cs.getPropertyValue(prop), "rect(0px, 6px, 4px, auto)",
>         "rect-valued property " + prop + ": can't interpolate auto components");
> -    div.style.setProperty(prop, "rect(0px, 6px, 4px, 2px)", "");
> +  } else {
> +    is(prop, "-moz-image-region");

Only clip and -moz-image-region use this function, so it's fine for now. However, it would be better to make it more general.

::: layout/style/test/test_transitions_per_property.html:2152
(Diff revision 2)
> +    is(cs.getPropertyValue(prop), "rect(2.25px, 11.25px, 8.5px, 3.75px)",
> +       "rect-valued property " + prop + ": can't interpolate auto components");

I think we should also update the description because this is not an expected result, right?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #13)
> Comment on attachment 8896951 [details]
> Bug 1387951 - Don't convert zero component value to 'auto' in
> clone__moz_image_region.
> 
> https://reviewboard.mozilla.org/r/168248/#review173752
> 
> ::: layout/style/test/test_transitions_per_property.html:2146
> (Diff revision 2)
> > +  if (prop == "clip") {
> >      is(cs.getPropertyValue(prop), "rect(0px, 6px, 4px, auto)",
> >         "rect-valued property " + prop + ": can't interpolate auto components");
> > -    div.style.setProperty(prop, "rect(0px, 6px, 4px, 2px)", "");
> > +  } else {
> > +    is(prop, "-moz-image-region");
> 
> Only clip and -moz-image-region use this function, so it's fine for now.
> However, it would be better to make it more general.

Indeed.  The situation is annoying. I'd really want to enable stylo on Android and drop the Gecko code prior to using this test for a new property. :)

> ::: layout/style/test/test_transitions_per_property.html:2152
> (Diff revision 2)
> > +    is(cs.getPropertyValue(prop), "rect(2.25px, 11.25px, 8.5px, 3.75px)",
> > +       "rect-valued property " + prop + ": can't interpolate auto components");
> 
> I think we should also update the description because this is not an
> expected result, right?

Right. I am going to flip 'is' to 'todo_is'.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Comment 16

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=146f42a0e991592a17f456d0e2f038e5ecea2761

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896951 [details]
Bug 1387951 - Don't convert zero component value to 'auto' in clone__moz_image_region.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168248/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a960422510cc
Enable test_discrete-animations.html on stylo. r=daisuke
https://hg.mozilla.org/integration/autoland/rev/b4b58cf12108
Don't convert zero component value to 'auto' in clone__moz_image_region. r=boris

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a960422510cc
https://hg.mozilla.org/mozilla-central/rev/b4b58cf12108