1177524 - [Control Center] Improve subview animations

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Tim Taubert [:ttaubert] (inactive)]

The animation looks a little bumpy, let's improve it.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Original state

Two problems here:

1) When closing the subview the content immediately disappears before it moves out to the right.

2) When closing the subview the "More Information" button jumps to the top and then slides down again.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1177524-Apply-max-height-0-after-transitioning-t.patch

This fixes the jumping button.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: State with patch 1

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1177524-Don-t-hide-the-subview-until-after-the-t.patch

Wanted to apply that same patch to the main menu, but esp. the devtools button seems to handle subviews itself and then they still disappear.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: State with patch 2

Comment 6

[:Gijs (he/him)]

Comment on attachment 8626315 [details] [diff] [review]
0001-Bug-1177524-Apply-max-height-0-after-transitioning-t.patch

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

::: browser/components/customizableui/content/panelUI.xml
@@ +13,5 @@
>        <stylesheet src="chrome://browser/content/customizableui/panelUI.css"/>
>      </resources>
>      <content>
>        <xul:box anonid="viewContainer" class="panel-viewcontainer" xbl:inherits="panelopen,viewtype,transitioning">
> +        <xul:stack anonid="viewStack" xbl:inherits="viewtype,transitioning" viewtype="main" class="panel-viewstack">

I wonder if this will help simplifying some of the CSS.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8626323 [details] [diff] [review]
0002-Bug-1177524-Don-t-hide-the-subview-until-after-the-t.patch

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

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +29,5 @@
>  .panel-mainview[panelid=identity-popup] {
>    min-width: 30em;
>  }
>  
> +#identity-popup-securityView:not([mainview]):not([current]) {

I think we should probably use this everywhere? Can you update the onSubviewHidden thing to fire after the transition finishes (assuming that doesn't cause "interesting" races with clicking to open a view right after closing one...).

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1177524-Don-t-hide-the-subview-until-after-the-t.patch, v2

Ok that took me a while to come up with but it works for the control center, the history subview, and the devtools subview now.

We're basically replacing display:none with visibility:collapse to preserve that non-visible elements do not affect the layout, but we can transition from visiblity:visible for the currently shown subview to visibility:collapse after the transition is done.

Moved the clearSubview() calls for history and devtools subviews to onPanelShowing() so that their contents stick around for the transition.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1177524-Don-t-hide-the-subview-until-after-the-t.patch, v3

Can do with one rule less actually.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8627513 [details] [diff] [review]
0002-Bug-1177524-Don-t-hide-the-subview-until-after-the-t.patch, v3

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

LGTM for now. Part of me wonders if/how this works with keyboard a11y because now those items linger around. We could probably kill post-transition but that's more work, let's punt on that for now and we can revisit if necessary.

::: browser/base/content/browser.css
@@ +113,5 @@
>    transition: transform 150ms;
>  }
>  
>  panelview:not([mainview]):not([current]) {
> +  transition: visibility 0s linear 150ms;

Pretty please make a CSS variable for the 150ms so that we reuse it between the transform on which this is waiting, and here. :-)

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/85a8d6b4838d
https://hg.mozilla.org/integration/fx-team/rev/e5d227b6f424

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/85a8d6b4838d
https://hg.mozilla.org/mozilla-central/rev/e5d227b6f424

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Merge of both for Aurora uplift

Approval Request Comment
[Feature/regressing bug #]: bug 1170759
[User impact if declined]: Subpar and jumpy animations when showing and hiding subviews in the control center. 
[Describe test coverage new/current, TreeHerder]: No test coverage for animations.
[Risks and why]: Low risk, touches only the animation itself.
[String/UUID change made/needed]: None.

Comment 14

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed in Fx42.0a1, 2015-07-06.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8629205 [details] [diff] [review]
Merge of both for Aurora uplift

Approving for uplift as it has been in m-c for a while.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01207e133bd1

Comment 17

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 41b3 Win 7, Ubuntu 14.04, OS X 10.10.