Closed Bug 1304700 Opened 8 years ago Closed 8 years ago

No animation when collapsing HTML splitter

Categories

(DevTools :: Shared Components, defect, P1)

defect

Tracking

(firefox49 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52 verified)

VERIFIED FIXED
Firefox 52
Iteration:
52.1 - Oct 3
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- verified

People

(Reporter: jdescottes, Assigned: Honza)

References

Details

(Keywords: regression, Whiteboard: [devtools-html])

Attachments

(2 files, 4 obsolete files)

(In reply to Fred Lin [:gasolin] from Bug 1260552 comment #61) > Quick test the patch and overall works great! I'll try turn .xul to .html > based on it and find whats missing in bug 1292592 (de-xul sourceeditor). > > During test I found the `hide sidebar animation` is missing when tap the > sidebar toggle. But `show sidebar animation` still works. We can fix it in > another bug anyway. Still true in the version that landed, there is no animation when collapsing the panel but there is one when expanding it. STRs: - open inspector - click on "collapse pane" ER: the sidebar collapse should be animated AR: the sidebar collapses immediately
Blocks: 1260552
Whiteboard: [devtools-html] [triage]
Flags: qe-verify?
Attached patch bug1304700.patch (obsolete) (deleted) — Splinter Review
Thanks for the report! Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attachment #8793834 - Flags: review?(jdescottes)
Iteration: --- → 52.1 - Oct 3
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
Comment on attachment 8793834 [details] [diff] [review] bug1304700.patch Review of attachment 8793834 [details] [diff] [review]: ----------------------------------------------------------------- The test the beginning of the method becomes racy with this change: if (flags.visible == !pane.classList.contains("pane-collapsed")) { if (flags.callback) { flags.callback(); } return; } In vertical layout, if I click quickly on the toggle icon, I can easily lock the panel in a half collapsed state (negative margins, but no "pane-collapsed" classname). We can use the "animated" attribute in combination with "pane-collapsed" to know if the panel is being hidden. Or we could add another classname or attribute, that is not linked to the animation. ::: devtools/client/shared/widgets/view-helpers.js @@ +295,5 @@ > if (flags.callback) { > flags.callback(); > } > + if (!flags.visible) { > + pane.classList.add("pane-collapsed"); This code path is only reached if flags.animated is true, so this breaks non-animated panels (netmonitor for instance).
Attachment #8793834 - Flags: review?(jdescottes)
Attached patch bug1304700.patch (deleted) — Splinter Review
(In reply to Julian Descottes [:jdescottes] from comment #2) > Comment on attachment 8793834 [details] [diff] [review] > bug1304700.patch > > Review of attachment 8793834 [details] [diff] [review]: > ----------------------------------------------------------------- > > The test the beginning of the method becomes racy with this change: Ah, good catch! I am attaching new patch - make sure the Net panel works. - avoid toggles when the animation is in progress. Honza
Attachment #8793834 - Attachment is obsolete: true
Attachment #8794160 - Flags: review?(jdescottes)
Comment on attachment 8794160 [details] [diff] [review] bug1304700.patch Review of attachment 8794160 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks for fixing this!
Attachment #8794160 - Flags: review?(jdescottes) → review+
Thanks for the review! Honza
Keywords: checkin-needed
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/333c888c9665 Fix side bar panel animation; r=jdescottes
Keywords: checkin-needed
Flags: qe-verify? → qe-verify+
QA Contact: cristian.comorasu
Backed out for failing devtools pane tests like browser_dbg_instruments-pane-collapse.js: https://hg.mozilla.org/integration/fx-team/rev/76fd7dbb20d5 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=a18668c1ff8da7e0b6b6945a4186448762064d7c Failure log (there are more pane related failures in other devtools logs): https://treeherder.mozilla.org/logviewer.html#?job_id=11709219&repo=fx-team [task 2016-09-23T16:46:26.805964Z] 16:46:26 INFO - 208 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js | The instruments pane should at this point be visible. - [task 2016-09-23T16:46:26.807157Z] 16:46:26 INFO - 209 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js | The debugger view panes should still initially be preffed as hidden. - [task 2016-09-23T16:46:26.807921Z] 16:46:26 INFO - 210 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js | The options menu item should still not be checked. - [task 2016-09-23T16:46:26.808936Z] 16:46:26 INFO - 211 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js | The instruments pane has an incorrect width after collapsing. - [task 2016-09-23T16:46:26.810043Z] 16:46:26 INFO - 212 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js | The instruments pane has an incorrect left margin after collapsing. - [task 2016-09-23T16:46:26.811057Z] 16:46:26 INFO - 213 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js | The instruments pane has an incorrect right margin after collapsing. - [task 2016-09-23T16:46:26.812243Z] 16:46:26 INFO - 214 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js | The instruments pane has an incorrect attribute after an animated collapsing. - [task 2016-09-23T16:46:26.813217Z] 16:46:26 INFO - 215 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js | The instruments pane should not be visible after collapsing. - [task 2016-09-23T16:46:26.814177Z] 16:46:26 INFO - Stack trace: [task 2016-09-23T16:46:26.815223Z] 16:46:26 INFO - chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js:testInstrumentsPaneCollapse:88 [task 2016-09-23T16:46:26.816271Z] 16:46:26 INFO - chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js:test/<:31 [task 2016-09-23T16:46:26.817311Z] 16:46:26 INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:Handler.prototype.process:937 [task 2016-09-23T16:46:26.818250Z] 16:46:26 INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:this.PromiseWalker.walkerLoop:816
Flags: needinfo?(odvarko)
Attached patch bug1304700-tests.patch (obsolete) (deleted) — Splinter Review
Here is a patch for the failing test. Julian, I am not entirely sure why the waitForTick() for stack clean up is needed, but I don't see any problem with `ViewHelper.togglePane()` and also, the tests is related to the old Debugger front end. Honza
Flags: needinfo?(odvarko)
Attachment #8794805 - Flags: review?(jdescottes)
(In reply to Jan Honza Odvarko [:Honza] from comment #8) > Created attachment 8794805 [details] [diff] [review] > bug1304700-tests.patch > > Here is a patch for the failing test. > > Julian, I am not entirely sure why the waitForTick() for stack clean up is > needed, but I don't see any problem with `ViewHelper.togglePane()` and also, > the tests is related to the old Debugger front end. > Waiting here is needed because no layout/reflow has been triggered after calling: > gDebugger.DebuggerView.toggleInstrumentsPane({ visible: true, animated: false }); Even if the panel is supposed to be expanded, its computed values are still the same as if it was collapsed, and the animation never happens. Instead of waiting, I think it would be safer to trigger a reflow (by calling getBoundingClientRect on instrumentsPane for instance) and add a comment to explain why this is needed here.
Comment on attachment 8794805 [details] [diff] [review] bug1304700-tests.patch Review of attachment 8794805 [details] [diff] [review]: ----------------------------------------------------------------- Another test fails for similar reasons : devtools/client/netmonitor/test/browser_net_pane-collapse.js The fix should be similar. Let's do one last round of review with both tests fixed + green try. ::: devtools/client/debugger/test/mochitest/browser_dbg_instruments-pane-collapse.js @@ +69,5 @@ > ok(!instrumentsPane.classList.contains("pane-collapsed") && > !instrumentsPaneToggleButton.classList.contains("pane-collapsed"), > "The instruments pane should at this point be visible."); > > + yield waitForTick(); See my previous comment, trigger a layout/reflow via getBoundingClientRect or similar. @@ +75,3 @@ > gDebugger.DebuggerView.toggleInstrumentsPane({ visible: false, animated: true }); > > + yield waitForTransitionEnd(instrumentsPane); use yield once(instrumentsPane, "transitionend"); instead :) @@ +171,5 @@ > + *   Node to observe > + * @return {Promise} promise that will resolve upon receiving > + * 'transitionend' event > + */ > +function waitForTransitionEnd(target) { Helper not needed since we have the "once" helper available globally in tests
Attachment #8794805 - Flags: review?(jdescottes)
Attached patch bug1304700-tests.patch (obsolete) (deleted) — Splinter Review
(In reply to Julian Descottes [:jdescottes] from comment #11) > Comment on attachment 8794805 [details] [diff] [review] > bug1304700-tests.patch > > Review of attachment 8794805 [details] [diff] [review]: > ----------------------------------------------------------------- All done, new patch attached. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6519099e787 Honza
Attachment #8794805 - Attachment is obsolete: true
Attachment #8794940 - Flags: review?(jdescottes)
Comment on attachment 8794940 [details] [diff] [review] bug1304700-tests.patch Review of attachment 8794940 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me thanks! Just one minor comment below. ::: devtools/client/netmonitor/test/browser_net_pane-collapse.js @@ +54,5 @@ > detailsPaneToggleButton.classList.contains("pane-collapsed"), > "The details pane should not be visible after collapsing."); > > + // Trigger reflow to make sure the UI is in required state. > + document.documentElement.getBoundingClientRect(); I don't think this one is needed, can you check without it ?
Attachment #8794940 - Flags: review?(jdescottes) → review+
Attached patch bug1304700.test-timeout.patch (obsolete) (deleted) — Splinter Review
There is still a test timeout on debug builds. I am pretty this is because the debugger test is now using both add_task() and closeDebuggerAndFinish(). Both helpers try to finish() the test which might lead to issues such as this one. Trees are closed at the moment so I couldn't push to try, but feel free to fold the patch attached here and push to try, hopefully it should fix it.
Flags: needinfo?(odvarko)
Pushed to try this morning: https://treeherder.mozilla.org/#/jobs?repo=try&revision=170b30e9408f91c155e3f7269c542df576a906d7 Some weird unrelated docker failures, but the timeout seems to be gone.
Attached patch bug1304700-tests.patch (deleted) — Splinter Review
I also tested with Try: Your patch included: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2a85dbaec94 The second getBoundingClientRect() removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3266f79615b All looks good (except unrelated failures) I am attaching updated patch: - Using Task.spawn() - Second getBoundingClientRect() removed. Honza
Attachment #8794940 - Attachment is obsolete: true
Attachment #8795025 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8795229 - Flags: review?(jdescottes)
Comment on attachment 8795229 [details] [diff] [review] bug1304700-tests.patch Review of attachment 8795229 [details] [diff] [review]: ----------------------------------------------------------------- Works for me if try is green!
Attachment #8795229 - Flags: review?(jdescottes) → review+
Thanks for the review! One more try push for other OSes https://treeherder.mozilla.org/#/jobs?repo=try&revision=deabbca9e365 Honza
The previous try-push looks good. Honza
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I've managed to reproduce this issue on a Nightly build 52.0a1 (2016-09-26) from a few days ago. (there was no animation on Collapse pane) This is verified fixed on latest Nightly 52.0a1 (2016-09-29) using Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.10.4 Yosemite.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: