stylo: The calc() computed values of {backgournd|mask}-{position|size} are not correct in test_transitions_per_property.html

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: boris, Assigned: hiro)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
I got many failures while using calc() as the value of background-{position|size} and mask-{position|size} in test_transitions_per_property.html

e.g.
1. backgroung-position 
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property background-position: interpolation of lists of lengths - got "calc(31.25px + 8.4375%) calc(20.3833px + 8.4375%), calc(43.75px + 8.4375%) calc(29.75px + 8.4375%), calc(30px + 8.4375%) calc(21.6333px + 8.4375%)", expected "20px 35px, 55px 50px, 30px 25px"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property background-position-x: interpolation of lists of lengths - got "calc(11.5667px + 45%), calc(24.0667px + 45%), calc(10.3167px + 45%)", expected "20px, 55px, 30px"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property background-position-y: interpolation of lists of lengths - got "calc(11.5667px + 45%), calc(24.0667px + 45%), calc(10.3167px + 45%)", expected "20px, 55px, 30px"

2. background-size
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property background-size: interpolation of lists of lengths - got "calc(31.25px + 8.4375%) calc(20.3833px + 8.4375%), calc(43.75px + 8.4375%) calc(29.75px + 8.4375%), calc(30px + 8.4375%) calc(21.6333px + 8.4375%)", expected "20px 35px, 55px 50px, 30px 25px"

3. mask-position
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property mask-position: interpolation of lists of lengths - got "calc(31.25px + 8.4375%) calc(20.3833px + 8.4375%), calc(43.75px + 8.4375%) calc(29.75px + 8.4375%), calc(30px + 8.4375%) calc(21.6333px + 8.4375%)", expected "20px 35px, 55px 50px, 30px 25px"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property mask-position-x: interpolation of lists of lengths - got "calc(11.5667px + 45%), calc(24.0667px + 45%), calc(10.3167px + 45%)", expected "20px, 55px, 30px"
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property mask-position-y: interpolation of lists of lengths - got "calc(11.5667px + 45%), calc(24.0667px + 45%), calc(10.3167px + 45%)", expected "20px, 55px, 30px"

4. mask-size
TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | property mask-size: interpolation of lists of lengths - got "calc(31.25px + 8.4375%) calc(20.3833px + 8.4375%), calc(43.75px + 8.4375%) calc(29.75px + 8.4375%), calc(30px + 8.4375%) calc(21.6333px + 8.4375%)", expected "20px 35px, 55px 50px, 30px 25px"
Bug 1385140 (and revision 1 of that patch: https://reviewboard.mozilla.org/r/162348/diff/1/) might be relevant here.
    div.style.setProperty(prop, "10px 40px, 50px 50px, 30px 20px", "");
    is(cs.getPropertyValue(prop), "10px 40px, 50px 50px, 30px 20px",
       "property " + prop + ": computed value before transition");
    div.style.setProperty(prop, "50px 20px, 70px 50px, 30px 40px", "");
    is(cs.getPropertyValue(prop), "20px 35px, 55px 50px, 30px 25px",
       "property " + prop + ": interpolation of lists of lengths");

This part of the test makes little sense to me, could you explain it to me, Brian?
Flags: needinfo?(bbirtles)
For what it's worth, I really don't like these mammoth tests -- they waste a lot of time isolating failures or, in this case, just understanding them.

I'm not sure what part you're asking about but if it's about how the test works in general, then basically we have the following setup:

  div.style.setProperty("transition-duration", "4s", "");
  div.style.setProperty("transition-delay", "-1s", "");
  div.style.setProperty("transition-timing-function", "linear", "");

Then for each |property| we typically:

1. Set transition-property to 'none'
2. Set the initial value of |property|
3. Flush style (and check the result of getComputedStyle)
4. Set transition-property to |property|
5. Set the target value of |property|
6. Flush style and check that getComputedStyle returns the value 1/4 of the way through.

For the above test, however, it seems like we're not doing 1 or 4 which looks like a bug to me.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #3)
> For what it's worth, I really don't like these mammoth tests -- they waste a
> lot of time isolating failures or, in this case, just understanding them.

Preaching to the choir here. :) For the little story, I asked about line >2400 on IRC, and emilio replied by a link to line 1080 for a possible explanation, hah.

> I'm not sure what part you're asking about but if it's about how the test
> works in general, then basically we have the following setup:
> 
>   div.style.setProperty("transition-duration", "4s", "");
>   div.style.setProperty("transition-delay", "-1s", "");
>   div.style.setProperty("transition-timing-function", "linear", "");
> 
> Then for each |property| we typically:
> 
> 1. Set transition-property to 'none'
> 2. Set the initial value of |property|
> 3. Flush style (and check the result of getComputedStyle)
> 4. Set transition-property to |property|
> 5. Set the target value of |property|
> 6. Flush style and check that getComputedStyle returns the value 1/4 of the
> way through.

Exactly what I wanted to know, thanks.

> For the above test, however, it seems like we're not doing 1 or 4 which
> looks like a bug to me.

This is what I was concluding from the code too, but I wasn't sure.

Should we just fix the tests, then?
(In reply to Anthony Ramine [:nox] from comment #4)
> Should we just fix the tests, then?

Yes, I think so!
(Assignee)

Comment 6

2 years ago
(In reply to Brian Birtles (:birtles) from comment #3)
> 1. Set transition-property to 'none'
> 2. Set the initial value of |property|
> 3. Flush style (and check the result of getComputedStyle)
> 4. Set transition-property to |property|
> 5. Set the target value of |property|
> 6. Flush style and check that getComputedStyle returns the value 1/4 of the
> way through.
> 
> For the above test, however, it seems like we're not doing 1 or 4 which
> looks like a bug to me.

I did look this. The reason why gecko does not fail, does cancel an old transition and creates a new transition is because of bug 1390446.
On gecko different length background-position list can not be interpolated [1], so if the element has already a transition, the transition is canceled [2], then we can create a new transition without resetting transition property value in the test cases.
I've filed bug 1390446 for this behavior.

Anyway test cases in test_transition_per_property.html should not rely on the behavior. We should explicitly set/reset transition property value.

[1] https://hg.mozilla.org/mozilla-central/file/b95b1638db48/layout/style/StyleAnimationValue.cpp#l3304
[2] https://hg.mozilla.org/mozilla-central/file/b95b1638db48/layout/style/nsTransitionManager.cpp#l934
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8897593 [details]
Bug 1387946 - Set/reset transition-property to cancel/create transition properly.

https://reviewboard.mozilla.org/r/168860/#review174254

::: commit-message-91e40:5
(Diff revision 1)
> +Bug 1387946 - Set/reset transition-property to cancel/create transition properly. r?birtles
> +
> +These test cases rely on the behavior that different lengths repeated list can
> +not be transitioned on Gecko. But it's not clear the behavior is reasonable or
> +not.  The tess cases here should not depend on the behavior.

Nit: test cases
Attachment #8897593 - Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request)

Comment 11

2 years ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d638c206ad77
Set/reset transition-property to cancel/create transition properly. r=birtles

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d638c206ad77
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.