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

RESOLVED FIXED in Firefox 57

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: boris, Assigned: hiro)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Oh right.
I think those are the test cases that need to check the values are interpolatable.
Depends on: 1355761
(Reporter)

Comment 2

2 years ago
And property "order"
(Reporter)

Comment 3

2 years ago
(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"
Priority: -- → P2
(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)
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
I did fix this too when I investigate bug 1387951.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1355761
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
mozreview-review
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.
(Assignee)

Updated

2 years ago
Depends on: 1387951
(Assignee)

Comment 16

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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?
(Assignee)

Comment 25

2 years ago
(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.
(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

2 years ago
mozreview-review
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.
Attachment #8896957 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 28

2 years ago
(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

2 years ago
mozreview-review
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/
Attachment #8896958 - Flags: review?(bbirtles) → review+
(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

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

https://reviewboard.mozilla.org/r/168256/#review173726
Attachment #8896954 - Flags: review?(bbirtles) → review+
(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

2 years ago
mozreview-review
Comment on attachment 8896955 [details]
Bug 1387939 - Make assertion for colum-count more accurate.

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

Thanks!
Attachment #8896955 - Flags: review?(mantaroh) → review+
(Reporter)

Comment 34

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

https://reviewboard.mozilla.org/r/168260/#review173748
Attachment #8896956 - Flags: review?(boris.chiou) → review+
(Reporter)

Comment 35

2 years ago
mozreview-review
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
Attachment #8896959 - Flags: review?(boris.chiou) → review+
(Assignee)

Comment 37

2 years ago
Posted file Servo PR
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8896954 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8896955 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8896956 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8896957 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8896958 - Attachment is obsolete: true
(Assignee)

Comment 40

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

https://reviewboard.mozilla.org/r/168462/#review173798
Attachment #8897177 - Flags: review?(hikezoe) → review+

Comment 41

2 years ago
Pushed by hikezoe@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7bf71d6ebb33
https://hg.mozilla.org/mozilla-central/rev/831879813489
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.