Closed Bug 1339648 Opened 7 years ago Closed 7 years ago

Intermittent layout/style/test/test_transitions_replacement_on_busy_frame.html | From value of transition is updated since the moment when it was generated - didn't expect "none", but got it

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: birtles)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Ok, I think there are two bugs here:

(a) We don't actually trigger the transition when we mean to because at [1] we have:

      getComputedStyle(div);

    which doesn't actually flush style. We want:

      getComputedStyle(div).transform;

(b) Probably don't actually wait for the transition to get past the first frame so it's possible when we trigger the second transition that the computed value is still at `matrix(1, 0, 0, 1, 0, 0)` and hence the second transition won't be created.

I haven't seen (b) happen on automation but I see it often locally (presumably because the automation machines are slower and hence we're less likely to try to trigger the second transition whilst still on the first frame).

[1] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/style/test/file_transitions_replacement_on_busy_frame.html#46
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Component: Layout → CSS Parsing and Computation
I messed something up when rebasing bug 1382841 on top of my fix for this. I'm going to look into it tomorrow instead.
Comment on attachment 8900560 [details]
Bug 1339648 - Fix flaky file_transitions_replacement_on_busy_frame.html test;

https://reviewboard.mozilla.org/r/171960/#review177172

::: commit-message-f0abd:19
(Diff revision 1)
> +    generated (although the first transition will be canceled) since the
> +    there is no change.

nit: superfluous 'the' at the end of the line.

::: layout/style/test/file_transitions_replacement_on_busy_frame.html:45
(Diff revision 1)
>      finish();
>      return;
>    }
>  
>    var div = document.getElementById("target");
> +

This extra line wasn't actually intentional but having added it, I think it probably helps readability.
Comment on attachment 8900560 [details]
Bug 1339648 - Fix flaky file_transitions_replacement_on_busy_frame.html test;

https://reviewboard.mozilla.org/r/171960/#review177174

I am convinced that this flakiness is related what I commented in bug 1366603 comment 18. The MozAfterPaint that waitForAllPaints waits is not what we expected (i.e. transition start).
Anyway, this change looks reasonable to me. Thanks for fixing this!

::: layout/style/test/file_transitions_replacement_on_busy_frame.html:59
(Diff revision 1)
>      var previousKeyframeValue;
>      var anim;
>      requestAnimationFrame(function() {
>        // Start second transition
> -      div.style.transform = "translateX(0px)";
> +      div.style.transform = "translateX(600px)";
>        getComputedStyle(div).transform;

Nit: should we remove this getComputedStyle() for consistency?
Attachment #8900560 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Comment on attachment 8900560 [details]
> Bug 1339648 - Fix flaky file_transitions_replacement_on_busy_frame.html test;
> 
> https://reviewboard.mozilla.org/r/171960/#review177174
> 
> I am convinced that this flakiness is related what I commented in bug
> 1366603 comment 18. The MozAfterPaint that waitForAllPaints waits is not
> what we expected (i.e. transition start).
> Anyway, this change looks reasonable to me. Thanks for fixing this!
> 
> ::: layout/style/test/file_transitions_replacement_on_busy_frame.html:59
> (Diff revision 1)
> >      var previousKeyframeValue;
> >      var anim;
> >      requestAnimationFrame(function() {
> >        // Start second transition
> > -      div.style.transform = "translateX(0px)";
> > +      div.style.transform = "translateX(600px)";
> >        getComputedStyle(div).transform;
> 
> Nit: should we remove this getComputedStyle() for consistency?

Yes, good idea.

Actually, I would also like to rename |anim| to |secondTransition| for consistency, but I was trying to keep the patch small. Maybe I should just do that, however.
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ac13a91244f
Fix flaky file_transitions_replacement_on_busy_frame.html test; r=hiro
https://hg.mozilla.org/mozilla-central/rev/2ac13a91244f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.