(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

Thanks for the report! Honza

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

(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

Comment on attachment 8794160 [details] [diff] [review] bug1304700.patch Review of attachment 8794160 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks for fixing this!

Thanks for the review! Honza

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/333c888c9665 Fix side bar panel animation; r=jdescottes

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

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eacc30530e3e Honza

(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

(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

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 ?

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.

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.

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

Comment on attachment 8795229 [details] [diff] [review] bug1304700-tests.patch Review of attachment 8795229 [details] [diff] [review]: ----------------------------------------------------------------- Works for me if try is green!

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

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/c149c74b5001 Fix side bar panel animation. r=jdescottes https://hg.mozilla.org/integration/fx-team/rev/754a7acf26ad Fix tests. r=jdescottes

https://hg.mozilla.org/mozilla-central/rev/c149c74b5001 https://hg.mozilla.org/mozilla-central/rev/754a7acf26ad

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.