Closed Bug 1393870 Opened 7 years ago Closed 7 years ago

Panels anchored to bottom have weird animation

Categories

(Toolkit :: UI Widgets, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- verified

People

(Reporter: shorlander, Assigned: ewright)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(2 files)

Attached video Bottom anchored panels
Panels that are opened anchored to the bottom have a weird and janky close animation.

STR:
1) Go into Customize Mode
2) Open the panels from Themes or Density
3) Close the panels
4) Watch them slide off the bottom of the window

Expected Results:
Close the panel should just fade away like it does for top anchored panels.
Whiteboard: [photon-animation] → [photon-animation] [triage]
Priority: -- → P3
Whiteboard: [photon-animation] [triage] → [photon-animation]
Flags: qe-verify?
Whiteboard: [photon-animation] → [reserve-photon-animation]
Assignee: nobody → ewright
Status: NEW → ASSIGNED
Priority: P3 → P1
This is because of some rules added in Bug 1352075. Requesting a review from some folks involved in that bug.
Comment on attachment 8903284 [details]
Bug 1393870 - Panels anchored to bottom have weird animation.

https://reviewboard.mozilla.org/r/175072/#review180220

::: toolkit/content/xul.css
(Diff revision 1)
>    transition-duration: 0.18s, 0.18s;
>    transition-timing-function:
>      var(--animation-easing-function), ease-out;
>  }
>  
> -panel[type="arrow"][side="bottom"]:not([animate="false"]) {

Removing this rule will have the effect that panels opening from the bottom edge will animate in from the top, rather than from the bottom - which is not what we want.

::: toolkit/content/xul.css:494
(Diff revision 1)
>    transform: none;
>    transition-timing-function:
>      var(--animation-easing-function), ease-in-out;
>  }
>  
>  panel[type="arrow"][animate="cancel"] {

I think the bug here is with this rule's specificify. As it has a lower specificity than the rule: panel[type="arrow"]:not([animate="false"]) this rule is never being applied. 

2nd, the intention as seen in this cancel rule (which gets applied when the panel is hidden) is for the panel to transition away in the vertical direction it came in from. I.e. towards the top edge for drop-down panels, towards the bottom edge for drop-up panels. If UX prefers the opacity-only transition, we should do that here.
Attachment #8903284 - Flags: review?(sfoster)
:shorlander, :epang, I believe this was always supposed to animate back to the edge it emerged from. And its a bug that we're seeing opacity only close animation at all. At least that was the intention in the code as written. Do you prefer an opacity-only close transition?
Flags: needinfo?(shorlander)
Flags: needinfo?(epang)
(In reply to Sam Foster [:sfoster] from comment #4)
> :shorlander, :epang, I believe this was always supposed to animate back to
> the edge it emerged from. And its a bug that we're seeing opacity only close
> animation at all. At least that was the intention in the code as written. Do
> you prefer an opacity-only close transition?

We should see a drop up for opening a panel (reverse of a drop down). For close we should see the same opacity only fade out that we do for drop down panels, there's no need to animate back to the edge it emerged from.
Flags: needinfo?(shorlander)
Flags: needinfo?(epang)
Comment on attachment 8903284 [details]
Bug 1393870 - Panels anchored to bottom have weird animation.

https://reviewboard.mozilla.org/r/175072/#review180664

Thanks for looking at this, its almost there. I was confused about the behavior reported in this bug and noticed a thing: if you look at e.g. the #customization-uidensity-menu panel *before it has been opened in a session*, it has a side="top" attribute. After first use that is corrected to side="bottom". So, even after we get this CSS fixed, it will appear to slide up rather than down the first time this panel is shown. I think we'll want a separate bug for that though, and keep this one about getting the correct closing behavior.

::: toolkit/content/xul.css:454
(Diff revision 2)
>     shadow, and they don't affect the window's "shape", so the system doesn't
>     have to recompute the shadow shape during the animation. This makes them a
>     lot faster. In fact, Gecko no longer triggers shadow shape recomputations
>     for repaints.
>     These properties are not implemented on other platforms. */
>  panel[type="arrow"]:not([animate="false"]) {

needs [side] in the selector to get specificity right

::: toolkit/content/xul.css:463
(Diff revision 2)
>    transition-duration: 0.18s, 0.18s;
>    transition-timing-function:
>      var(--animation-easing-function), ease-out;
>  }
>  
>  panel[type="arrow"][side="bottom"]:not([animate="false"]) {

Because we do need to give the [side="bottom"] case this overridden transform value, the default panel[type="arrow"]:not([animate="false"]) selector needs [side] in it too so the specificity matches. This was pointed out in reviews of my original patch, but got lost along the way and landed without it.

::: toolkit/content/xul.css:475
(Diff revision 2)
>    -moz-window-transform: none;
>    transition-timing-function:
>      var(--animation-easing-function), ease-in-out;
>  }
>  
> +panel[type="arrow"][side="bottom"][animate="cancel"],

We don't need special treatment of [side="bottom"] panels here. They should all have transform: none, which ensures only opacity is animated as they close

::: toolkit/content/xul.css:482
(Diff revision 2)
>    -moz-window-transform: none;
>  }
>  
>  %elifndef MOZ_WIDGET_GTK
>  
>  panel[type="arrow"]:not([animate="false"]) {

same here, this selector needs to include [side] to have the correct specificity

::: toolkit/content/xul.css:503
(Diff revision 2)
>    transform: none;
>    transition-timing-function:
>      var(--animation-easing-function), ease-in-out;
>  }
>  
> +panel[type="arrow"][side="bottom"][animate="cancel"],

And same here, this can just be panel[type="arrow"[side][animate="cancel"]
Attachment #8903284 - Flags: review?(sfoster)
(In reply to Sam Foster [:sfoster] from comment #7)
> So, even after we get this CSS fixed, it will appear to
> slide up rather than down the first time this panel is shown. 

Correction: it will appear to slide down rather than up.
Comment on attachment 8903284 [details]
Bug 1393870 - Panels anchored to bottom have weird animation.

Deferring review to Sam here.
Attachment #8903284 - Flags: review?(dtownsend)
(In reply to Sam Foster [:sfoster] from comment #4)
> :shorlander, :epang, I believe this was always supposed to animate back to
> the edge it emerged from. And its a bug that we're seeing opacity only close
> animation at all. At least that was the intention in the code as written.

Just for posterity's sake, and anyone following along, I misread the code and this is/was not true. Adding the transform: none to the [cancel] (hiding) rule means while the panel hides, it will not animate transform, i.e. it should not slide out and only fade out. The selector specificity for all the rules involved is what is breaking this.
Comment on attachment 8903284 [details]
Bug 1393870 - Panels anchored to bottom have weird animation.

https://reviewboard.mozilla.org/r/175072/#review181382

Looks great. Tested on OSX and windows. Will need to get some try runs before landing as this was touchy code but I
Attachment #8903284 - Flags: review?(sfoster) → review+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4ba970fd7ba
Panels anchored to bottom have weird animation. r=sfoster
https://hg.mozilla.org/mozilla-central/rev/a4ba970fd7ba
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev
I was able to reproduce this issue with the following Nightly build (20170901150353).

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 (20170915100121)

This issue is verified as fixed with the latest Nightly build on 9/15/2017.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.