Panels anchored to bottom have weird animation

VERIFIED FIXED in Firefox 57

Status

()

Toolkit
XUL Widgets
P1
normal
VERIFIED FIXED
11 months ago
10 months ago

People

(Reporter: shorlander, Assigned: ewright)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-photon-animation])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

11 months ago
Created attachment 8901272 [details]
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.

Updated

11 months ago
Whiteboard: [photon-animation] → [photon-animation] [triage]
Blocks: 1352075

Updated

11 months ago
Priority: -- → P3
Whiteboard: [photon-animation] [triage] → [photon-animation]

Updated

11 months ago
Flags: qe-verify?
Whiteboard: [photon-animation] → [reserve-photon-animation]
(Assignee)

Updated

11 months ago
Assignee: nobody → ewright

Updated

11 months ago
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 2

11 months ago
This is because of some rules added in Bug 1352075. Requesting a review from some folks involved in that bug.

Comment 3

11 months ago
mozreview-review
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)

Comment 4

11 months ago
: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)
Comment hidden (mozreview-request)

Comment 6

11 months ago
(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 7

11 months ago
mozreview-review
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)

Comment 8

11 months ago
(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 9

11 months ago
Comment on attachment 8903284 [details]
Bug 1393870 - Panels anchored to bottom have weird animation.

Deferring review to Sam here.
Attachment #8903284 - Flags: review?(dtownsend)

Comment 10

11 months ago
(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 hidden (mozreview-request)

Comment 12

11 months ago
mozreview-review
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+

Comment 14

11 months ago
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
Last Resolved: 11 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

11 months ago
Iteration: --- → 57.3 - Sep 19

Updated

10 months ago
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev

Comment 16

10 months ago
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
status-firefox57: fixed → verified

Updated

10 months ago
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.