1304700 - No animation when collapsing HTML splitter

Status: VERIFIED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Julian Descottes [:jdescottes]]

(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

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1304700.patch

Thanks for the report!

Honza

Comment 2

[Julian Descottes [:jdescottes]]

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

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1304700.patch

(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 4

[Julian Descottes [:jdescottes]]

Comment on attachment 8794160 [details] [diff] [review]
bug1304700.patch

Review of attachment 8794160 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks for fixing this!

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the review!

Honza

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/333c888c9665
Fix side bar panel animation; r=jdescottes

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: 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.

Honza

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Honza

Comment 10

[Julian Descottes [: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 11

[Julian Descottes [:jdescottes]]

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

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1304700-tests.patch

(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 13

[Julian Descottes [: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 ?

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: bug1304700.test-timeout.patch

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.

Comment 15

[Julian Descottes [:jdescottes]]

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.

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1304700-tests.patch

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 17

[Julian Descottes [: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!

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the review!

One more try push for other OSes
https://treeherder.mozilla.org/#/jobs?repo=try&revision=deabbca9e365

Honza

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The previous try-push looks good.

Honza

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Ciprian Georgiu, Desktop QA]

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.