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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: intermittent-bug-filer, Assigned: birtles)
References
Details
(Keywords: intermittent-failure)
Attachments
(1 file)
Filed by: wkocher [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=77338871&repo=mozilla-inbound https://queue.taskcluster.net/v1/task/Cg6XgeLGSYiuwyGP1l3Vxw/runs/0/artifacts/public/logs/live_backing.log
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 7•7 years ago
|
||
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
Assignee | ||
Comment 8•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review |
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 11•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2ac13a91244f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 16•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c0bd0885603f
status-firefox56:
--- → fixed
Flags: in-testsuite+
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•