1387939 - stylo: Interpolation of "Integer or Auto" is incorrect in test_transitions_per_property.html

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Boris Chiou [:boris]]

While running test_transitions_per_property.html, I got these failures:

1. column-count:
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | integer-valued property column-count: auto not interpolable - got "6", expected "auto"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | integer-valued property column-count: computed value before transition - got "7", expected "8"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | integer-valued property column-count: interpolation of integers - got "6", expected "7"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | integer-valued property column-count: clamping of negatives - got "auto", expected "1"

2. z-index:
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | integer-valued property z-index: interpolation of integers - got "-1", expected "0"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | integer-valued property z-index: auto not interpolable - got "6", expected "auto"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | integer-valued property z-index: computed value before transition - got "7", expected "8"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | integer-valued property z-index: interpolation of integers - got "6", expected "7"
    
Both column-count and z-index are integer values, and it seems their interpolated results are not correct.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Oh right.
I think those are the test cases that need to check the values are interpolatable.

Comment 2

[Boris Chiou [:boris]]

And property "order"

Comment 3

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #2)
> And property "order"

TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | integer-valued property order: interpolation of integers - got "-1", expected "0"

Comment 4

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Oh right.
> I think those are the test cases that need to check the values are
> interpolatable.

I thought we checked if values were interpolable on the Gecko side as well so that bug 1355761 was not necessary? (i.e. bug 1355761 is only about checking for interpolability on the Servo side to avoid having to do the Gecko-side check)

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #4)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > Oh right.
> > I think those are the test cases that need to check the values are
> > interpolatable.
> 
> I thought we checked if values were interpolable on the Gecko side as well
> so that bug 1355761 was not necessary? (i.e. bug 1355761 is only about
> checking for interpolability on the Servo side to avoid having to do the
> Gecko-side check)

It seemed that we did not check interpolability on the Gecko side.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I did fix this too when I investigate bug 1387951.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1387939 - Check interpolatable to tell whether transition is created or not.

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1387939 - Make assertion for colum-count more accurate.

column-count should be greater than or equal to 1. Also it should be less than
or equal to nsStyleColumn_kMaxColumnCount since we limit it in
set_column_count().

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

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1387939 - ComputedPositiveInteger is an integer greater than or equal to 1.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1387939 - Don't pretend as discrete type animation in add_weighted() for Either<>.

We should pretend it in Servo_AnimationCompose() for animations
(i.e not for transitions).

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1387939 - Round harlway values toward positive infinity for integer type of animation.

From the spec[1];

 `with values halfway between a pair of integers rounded towards positive infinity.`

[1] https://drafts.csswg.org/css-transitions/#animtype-integer

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

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1387939 - Enable test cases for interpolation between 'auto' and other values on stylo.

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

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896959 [details]
Bug 1387939 - Enable test cases for interpolation between 'auto' and other values on stylo.

https://reviewboard.mozilla.org/r/168266/#review173422

::: layout/style/test/test_transitions_per_property.html
(Diff revision 1)
>      "mask-size": true,              // Bug 1387946
>      "box-shadow": true,             // Bug 1387973
> -    "caret-color": true,            // Bug 1387951
> -    "clip": true,                   // Bug 1387951
> -    "column-count": true,           // Bug 1387939
> -    "-moz-image-region": true,      // Bug 1387951

Note that all -moz-image-region test cases can not be passed without the fix for bug 1387951.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Though I did not notice but the forth patch "Don't pretend..." makes accumulation/addition for stroke-dasharray tests in wpt pass. That's because servo still uses Either type for stroke-dasharray, thus with the patch accumulation/addition fails between different types (e.g. Length and Number).

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1387939 - Update wpt expectations for stroke-dasharray accumulation/addition.

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

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896954 [details]
Bug 1387939 - Check interpolatable to tell whether transition is created or not.

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

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896955 [details]
Bug 1387939 - Make assertion for colum-count more accurate.

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

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896956 [details]
Bug 1387939 - ComputedPositiveInteger is an integer greater than or equal to 1.

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

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896957 [details]
Bug 1387939 - Don't pretend as discrete type animation in add_weighted() for Either<>.

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

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896958 [details]
Bug 1387939 - Round harlway values toward positive infinity for integer type of animation.

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

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896959 [details]
Bug 1387939 - Enable test cases for interpolation between 'auto' and other values on stylo.

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

Comment 24

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> Though I did not notice but the forth patch "Don't pretend..." makes
> accumulation/addition for stroke-dasharray tests in wpt pass. That's because
> servo still uses Either type for stroke-dasharray, thus with the patch
> accumulation/addition fails between different types (e.g. Length and Number).

Bug 1369614 has the same patch to enable these tests. If I understand your comment above, you're saying that with these patches those tests also happen to pass but only because we got lucky?

Comment 25

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #24)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> > Though I did not notice but the forth patch "Don't pretend..." makes
> > accumulation/addition for stroke-dasharray tests in wpt pass. That's because
> > servo still uses Either type for stroke-dasharray, thus with the patch
> > accumulation/addition fails between different types (e.g. Length and Number).
> 
> Bug 1369614 has the same patch to enable these tests. If I understand your
> comment above, you're saying that with these patches those tests also happen
> to pass but only because we got lucky?

Yeah, it's some sort of lucky. In bug 1369614, we change the type for stroke-dasharray to non Either type. So, the patches for bug 1369614 has explicit add() and accumulate() functions that just fail.

Comment 26

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > > Oh right.
> > > I think those are the test cases that need to check the values are
> > > interpolatable.
> > 
> > I thought we checked if values were interpolable on the Gecko side as well
> > so that bug 1355761 was not necessary? (i.e. bug 1355761 is only about
> > checking for interpolability on the Servo side to avoid having to do the
> > Gecko-side check)
> 
> It seemed that we did not check interpolability on the Gecko side.

We seem to check it:

  http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/layout/style/nsTransitionManager.cpp#895

Is it simply that we are returning Ok() when we should return Err() for some types?

Comment 27

[Brian Birtles (:birtles)]

Comment on attachment 8896957 [details]
Bug 1387939 - Don't pretend as discrete type animation in add_weighted() for Either<>.

https://reviewboard.mozilla.org/r/168262/#review173720

::: commit-message-d4b0c:1
(Diff revision 2)
> +Bug 1387939 - Don't pretend as discrete type animation in add_weighted() for Either<>. r?birtles

Don't fallback to discrete animation within add_weighted for Either<>

::: commit-message-d4b0c:3
(Diff revision 2)
> +We should pretend it in Servo_AnimationCompose() for animations
> +(i.e not for transitions).

For CSS Transitions we want this case to return Err() so we know that the two values are not interpolable.

For CSS Animations/Web Animations we implement discrete animation as the fallback behavior when Err() is returned.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #26)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > (In reply to Brian Birtles (:birtles) from comment #4)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > > > Oh right.
> > > > I think those are the test cases that need to check the values are
> > > > interpolatable.
> > > 
> > > I thought we checked if values were interpolable on the Gecko side as well
> > > so that bug 1355761 was not necessary? (i.e. bug 1355761 is only about
> > > checking for interpolability on the Servo side to avoid having to do the
> > > Gecko-side check)
> > 
> > It seemed that we did not check interpolability on the Gecko side.
> 
> We seem to check it:
> 
>  
> http://searchfox.org/mozilla-central/rev/
> e5b13e6224dbe3182050cf442608c4cb6a8c5c55/layout/style/nsTransitionManager.
> cpp#895
> 
> Is it simply that we are returning Ok() when we should return Err() for some
> types?

oh-oh, I missed that, I though the function is only gecko! So, the first patch might not be needed here. I will drop it if you think it shouldn't be.

Comment 29

[Brian Birtles (:birtles)]

Comment on attachment 8896958 [details]
Bug 1387939 - Round harlway values toward positive infinity for integer type of animation.

https://reviewboard.mozilla.org/r/168264/#review173722

::: commit-message-48fef:1
(Diff revision 2)
> +Bug 1387939 - Round harlway values toward positive infinity for integer type of animation. r?birtles

s/harlway/halfway/

Comment 30

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> (In reply to Brian Birtles (:birtles) from comment #26)
> > We seem to check it:
> > 
> >  
> > http://searchfox.org/mozilla-central/rev/
> > e5b13e6224dbe3182050cf442608c4cb6a8c5c55/layout/style/nsTransitionManager.
> > cpp#895
> > 
> > Is it simply that we are returning Ok() when we should return Err() for some
> > types?
> 
> oh-oh, I missed that, I though the function is only gecko! So, the first
> patch might not be needed here. I will drop it if you think it shouldn't be.

Yeah, I think the first patch is fine, but I'm afraid it might cover up other bugs like this. Perhaps we can land it later?

Comment 31

[Brian Birtles (:birtles)]

Comment on attachment 8896954 [details]
Bug 1387939 - Check interpolatable to tell whether transition is created or not.

https://reviewboard.mozilla.org/r/168256/#review173726

Comment 32

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #30)
> Yeah, I think the first patch is fine, but I'm afraid it might cover up
> other bugs like this. Perhaps we can land it later?

Actually, never mind. Let's just land it.

Comment 33

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8896955 [details]
Bug 1387939 - Make assertion for colum-count more accurate.

https://reviewboard.mozilla.org/r/168258/#review173728

Thanks!

Comment 34

[Boris Chiou [:boris]]

Comment on attachment 8896956 [details]
Bug 1387939 - ComputedPositiveInteger is an integer greater than or equal to 1.

https://reviewboard.mozilla.org/r/168260/#review173748

Comment 35

[Boris Chiou [:boris]]

Comment on attachment 8896959 [details]
Bug 1387939 - Enable test cases for interpolation between 'auto' and other values on stylo.

https://reviewboard.mozilla.org/r/168266/#review173750

Comment 36

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=825e3f81329e3e3341a74ad6cd41aeec6686ef31

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Attachment: Servo PR

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8896959 [details]
Bug 1387939 - Enable test cases for interpolation between 'auto' and other values on stylo.

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

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8897177 [details]
Bug 1387939 - Update wpt expectations for stroke-dasharray accumulation/addition.

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

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8897177 [details]
Bug 1387939 - Update wpt expectations for stroke-dasharray accumulation/addition.

https://reviewboard.mozilla.org/r/168462/#review173798

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bf71d6ebb33
Enable test cases for interpolation between 'auto' and other values on stylo. r=boris
https://hg.mozilla.org/integration/autoland/rev/831879813489
Update wpt expectations for stroke-dasharray accumulation/addition. r=hiro

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7bf71d6ebb33
https://hg.mozilla.org/mozilla-central/rev/831879813489