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.

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

And property "order"

(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"

(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)

(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.

I did fix this too when I investigate bug 1387951.

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.

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).

(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?

(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 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.

(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 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/

(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 on attachment 8896954 [details] Bug 1387939 - Check interpolatable to tell whether transition is created or not. https://reviewboard.mozilla.org/r/168256/#review173726

(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 on attachment 8896955 [details] Bug 1387939 - Make assertion for colum-count more accurate. https://reviewboard.mozilla.org/r/168258/#review173728 Thanks!

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 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 on attachment 8897177 [details] Bug 1387939 - Update wpt expectations for stroke-dasharray accumulation/addition. https://reviewboard.mozilla.org/r/168462/#review173798

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

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