Implement new animation for opening/closing the arrow panels

ASSIGNED
Assigned to

Status

()

Toolkit
XUL Widgets
P1
normal
ASSIGNED
3 months ago
10 hours ago

People

(Reporter: jaws, Assigned: sfoster)

Tracking

(Blocks: 1 bug)

unspecified
Points:
3
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-animation])

MozReview Requests

()

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

Attachments

(2 attachments, 7 obsolete attachments)

Created attachment 8852934 [details]
Doorhanger01.mp4

This animation will replace the slide-in (bug 767133) and fade-in (bug 767861) of the arrow-panels.

There are four proposals for animating the doorhanger:
Proposal #1: The panel should appear instantly, but slowly move down about 5 pixels until coming to a rest.

Proposal #2: The panel should appear instantly, but the contents of the menu should move down about 5 pixels until coming to a rest.

Proposal #3: The panel should appear instantly, but moving down about 5 pixels then back up 5 pixels but slower on its way back up.

Proposal #4: The panel should appear instantly, but moving down about 5 pixels while appearing in a linear speed.
(Reporter)

Updated

3 months ago
Blocks: 1349423
Created attachment 8852935 [details]
Doorhanger02.mp4
Created attachment 8852936 [details]
Doorhanger03.mp4
Created attachment 8852937 [details]
Doorhanger04.mp4
(Reporter)

Updated

3 months ago
Assignee: nobody → sfoster
Status: NEW → ASSIGNED

Updated

3 months ago
Whiteboard: [photon]
(Assignee)

Comment 4

2 months ago
Created attachment 8857004 [details]
Icon Rollover - Door Hanger.mp4

The selected door hanger / arrow-panel animations
Attachment #8852934 - Attachment is obsolete: true
Attachment #8852935 - Attachment is obsolete: true
Attachment #8852936 - Attachment is obsolete: true
Attachment #8852937 - Attachment is obsolete: true
(Reporter)

Updated

2 months ago
Whiteboard: [photon] → [photon-animation]
(Reporter)

Updated

2 months ago
Points: --- → 3

Updated

2 months ago
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
(Assignee)

Comment 5

2 months ago
This build includes a draft patch which implements the fade/slide-in animation on the various doorhangers/arrow-panels:

https://archive.mozilla.org/pub/firefox/try-builds/sfoster@mozilla.com-0891de7a46592c7588bd63e688694bde42d881ca/try-macosx64/

For whatever reason, when I go to download and run the firefox-55.0a1.en-US.mac.dmg from here, I get a "Nightly is damaged" error, but others were able to run it just fine so hopefully you can too.

I'm aware of a number of issues I need to address here, I'm mostly looking for feedback on the animation itself. I set a 100ms overall duration.
Flags: needinfo?(epang)
Flags: needinfo?(amlee)
(Assignee)

Comment 6

2 months ago
I did some profiling to try and track down the skipped frames I'm seeing sometimes with this animation. One of the things that stuck out was adjustArrowPosition, which :jaws points out is filed as bug 1296442.
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 months ago
Feedback from Amy & Eric was to increase the slide/offset distance to ~8px (was 6px). That change is included in the patch in attachment 8858468 [details]
Flags: needinfo?(epang)
Flags: needinfo?(amlee)

Comment 9

2 months ago
I would back out a few parts of bug 610545 (which changed from a simple slide and fade animation to a more complicated scaling animation) because it introduced some regressions.

Comment 10

2 months ago
mozreview-review
Comment on attachment 8858468 [details]
Bug 1352075 - (WIP) Fade and slide in arrow-panel animation.

https://reviewboard.mozilla.org/r/130428/#review133164

::: toolkit/content/xul.css:443
(Diff revision 1)
>  
>  %ifndef MOZ_WIDGET_GTK
>  
> -panel[type="arrow"]:not([animate="false"]) {
> -  transform: scale(.4);
> -  opacity: 0;
> +panel[type="arrow"] .panel-arrowbox {
> +  padding-top: 0;
> +  padding-bottom: 0;

.panel-arrowbox doesn't seem to have padding by default, so this seems like a no-op?

::: toolkit/content/xul.css:466
(Diff revision 1)
>  }
>  
> -panel[type="arrow"][animate="cancel"] {
> -  transform: none;
> +/* panel slides down */
> +panel[type="arrow"][side="top"] {
> +  /* NOTE: (sfoster) some panels adjust the alignment of the panel/arrow to their anchor icon using margin-top,
> +           so I'm using important for now - margin-top may not be the right solution ; */

What exactly are you trying to do here? Neil Deakin or I may be able to help.
(Assignee)

Comment 11

2 months ago
> > +panel[type="arrow"] .panel-arrowbox {
> > +  padding-top: 0;
> > +  padding-bottom: 0;
> 
> .panel-arrowbox doesn't seem to have padding by default, so this seems like
> a no-op?

I'm relying on padding being 0 and will set either top/bottom below, depending on the side attribute value, so my instinct is to be explicit in this rule. It doesnt matter though. 

> > +  /* NOTE: (sfoster) some panels adjust the alignment of the panel/arrow to their anchor icon using margin-top,
> > +           so I'm using important for now - margin-top may not be the right solution ; */
> 
> What exactly are you trying to do here? Neil Deakin or I may be able to help.

Here's how this is supposed to work: I'm going to animate the panel in by (assume from top down) sliding the panel from a starting point 8px above position, down into the correct position. My understanding is to have a shot a 60fps, I want this animation to happen inside the popup, rather than moving the popup itself. 

I create the room in the popup to do so by adding to the padding in the arrowbox, above the arrow image. The negative margin-top pulls it all up by the same amount. This is the resting/open state, where the top of the arrow lines up correctly with the anchor. When :not([animate="false"]):not([animate="open"]), the wrapper element .viewcontainer is moved up to -8px using translateY which represents the start of the animation. Over the 100ms animation duration, the translateY value is reduced to 0.

The problem that note addresses is I noticed some(?) instances of this arrowpanel binding also use a negative margin-top value, presumably to better line up the arrow with the anchor icon. However, I just now went to find an example and came up empty, so this may be a non-issue. I will need to adapt slightly for the bookmarks popup which currently has margin-top: -4px.
Flags: needinfo?(enndeakin)
Flags: needinfo?(dao+bmo)

Updated

2 months ago
Iteration: --- → 55.4 - May 1
(Reporter)

Comment 12

2 months ago
mozreview-review
Comment on attachment 8858468 [details]
Bug 1352075 - (WIP) Fade and slide in arrow-panel animation.

https://reviewboard.mozilla.org/r/130428/#review134634

I'm fine with this approach. I'm not sure of a way to do this with a different approach. Some minor notes below.

::: toolkit/content/xul.css:447
(Diff revision 1)
> +  will-change: transform, opacity;
> +  transition-property: opacity, transform;

nit, please update will-change to have the same order as the properties below.

::: toolkit/content/xul.css:450
(Diff revision 1)
>  
> -panel[type="arrow"][animate="open"] {
> +panel[type="arrow"] > .panel-arrowcontainer {
> +  will-change: transform, opacity;
> +  transition-property: opacity, transform;
> +  transition-duration: 70ms, 100ms;
> +  transition-timing-function: cubic-bezier(.07,.95,0,1);

When the nubmer of timing-functions is less than the number of properties, 'ease' will be used to for the missing timing-functions. Please change the rule to:

> transition-timing-function: cubic-bezier(.07,.95,0,1), cubic-bezier(.07,.95,0,1);

::: toolkit/content/xul.css:451
(Diff revision 1)
> -panel[type="arrow"][animate="open"] {
> +panel[type="arrow"] > .panel-arrowcontainer {
> +  will-change: transform, opacity;
> +  transition-property: opacity, transform;
> +  transition-duration: 70ms, 100ms;
> +  transition-timing-function: cubic-bezier(.07,.95,0,1);
> +  transition-delay: 0ms,0ms;

The delay defaults to 0ms so this line can be removed.
Attachment #8858468 - Flags: review?(jaws)
(In reply to Sam Foster [:sfoster] from comment #11)
> > > +  /* NOTE: (sfoster) some panels adjust the alignment of the panel/arrow to their anchor icon using margin-top,
> > > +           so I'm using important for now - margin-top may not be the right solution ; */
> > 
> > What exactly are you trying to do here? Neil Deakin or I may be able to help.
> 
> Here's how this is supposed to work: I'm going to animate the panel in by
> (assume from top down) sliding the panel from a starting point 8px above
> position, down into the correct position. My understanding is to have a shot
> a 60fps, I want this animation to happen inside the popup, rather than
> moving the popup itself. 
> 
> I create the room in the popup to do so by adding to the padding in the
> arrowbox, above the arrow image. The negative margin-top pulls it all up by
> the same amount. This is the resting/open state, where the top of the arrow
> lines up correctly with the anchor. When
> :not([animate="false"]):not([animate="open"]), the wrapper element
> .viewcontainer is moved up to -8px using translateY which represents the
> start of the animation. Over the 100ms animation duration, the translateY
> value is reduced to 0.
> 
> The problem that note addresses is I noticed some(?) instances of this
> arrowpanel binding also use a negative margin-top value, presumably to
> better line up the arrow with the anchor icon. However, I just now went to
> find an example and came up empty, so this may be a non-issue. I will need
> to adapt slightly for the bookmarks popup which currently has margin-top:
> -4px.

We had previously implemented a slide-in animation in bug 767133. It used transform. Is there some reason why we can't use that here?

I think the concern you noted is a valid one. Also, since he worked on bug 610545, I think you'll want Neil's review. In fact for relatively complicated xul.css changes like this one, you should really get his review -- see the note at the top of that file.

(In reply to Tim Nguyen :ntim from comment #9)
> I would back out a few parts of bug 610545 (which changed from a simple
> slide and fade animation to a more complicated scaling animation) because it
> introduced some regressions.

Yep, we should check if we can get rid of some of that code if it's not needed anymore.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 14

2 months ago
> We had previously implemented a slide-in animation in bug 767133. It used
> transform. Is there some reason why we can't use that here?

Thanks for pointing that out. That patch seems to simply animate the translateY/X on .panel-arrowcontainer. I've not built this yet but it looks like that will clip the content as it slides in, rather than the whole popup appearing to drop into place. 

> I think the concern you noted is a valid one. Also, since he worked on bug
> 610545, I think you'll want Neil's review. In fact for relatively
> complicated xul.css changes like this one, you should really get his review
> -- see the note at the top of that file.

from xul.css:

> * This file should also not contain any app specific styling.  Defaults for
> * widgets of a particular application should be in that application's style
> * sheet

That makes good sense and I'm happy to move this work to another .css file. Unless I duplicate it across each platform's popup.css its not clear where to move it to though?  The existing animation is implemented in xul.css. Should that move to browser.inc.css? Or a new browser/themes/popup.css? If we  conflate a CSS refactoring with this animation change, there will be more cooks in the kitchen and the complexity of this bug will increase considerably.  If its necessary lets do it, but lets hear from Neal first.
(In reply to Sam Foster [:sfoster] from comment #14)
> > We had previously implemented a slide-in animation in bug 767133. It used
> > transform. Is there some reason why we can't use that here?
> 
> Thanks for pointing that out. That patch seems to simply animate the
> translateY/X on .panel-arrowcontainer. I've not built this yet but it looks
> like that will clip the content as it slides in, rather than the whole popup
> appearing to drop into place.

We had shipped this from Firefox 17 to 30, pretty sure there was no visible clipping. It's possible that the opacity animation (bug 767861) masked it, though.

> > I think the concern you noted is a valid one. Also, since he worked on bug
> > 610545, I think you'll want Neil's review. In fact for relatively
> > complicated xul.css changes like this one, you should really get his review
> > -- see the note at the top of that file.
> 
> from xul.css:
> 
> > * This file should also not contain any app specific styling.  Defaults for
> > * widgets of a particular application should be in that application's style
> > * sheet

I meant this:

 * THIS FILE IS LOCKED DOWN.  YOU ARE NOT ALLOWED TO MODIFY IT WITHOUT FIRST
 * HAVING YOUR CHANGES REVIEWED BY enndeakin@gmail.com

I think it's fine for the arrow panel animation to continue to be implemented in xul.css, as long as your implementation doesn't add constraints that would make this a bad idea.
(In reply to Dão Gottwald [::dao] from comment #15)
> (In reply to Sam Foster [:sfoster] from comment #14)
> > > We had previously implemented a slide-in animation in bug 767133. It used
> > > transform. Is there some reason why we can't use that here?
> > 
> > Thanks for pointing that out. That patch seems to simply animate the
> > translateY/X on .panel-arrowcontainer. I've not built this yet but it looks
> > like that will clip the content as it slides in, rather than the whole popup
> > appearing to drop into place.
> 
> We had shipped this from Firefox 17

16 even...
When I tested the latest patch on Mac the popup reopens when I click on the triggering (Open) button. Without this patch this does not happen. The testcase I used was:

<button label="Open"
        oncommand="document.getElementById('panel').openPopup(this, 'after_start')"/>
<panel id="panel" type="arrow">
  <button label="Hello"/>
</panel>
Flags: needinfo?(enndeakin)
(Assignee)

Comment 18

2 months ago
> (In reply to Tim Nguyen :ntim from comment #9)
> > I would back out a few parts of bug 610545 (which changed from a simple
> > slide and fade animation to a more complicated scaling animation) because it
> > introduced some regressions.
> 
> Yep, we should check if we can get rid of some of that code if it's not
> needed anymore.

I'm actually not seeing anything from bug 610545 that I can remove. This new animation is fundamentally the same as the existing one in that it transitions 2 properties (opacity and transform). So although it completes sooner and looks simpler, the code to support it should be the same.
(Assignee)

Comment 19

2 months ago
(In reply to Neil Deakin from comment #17)
> When I tested the latest patch on Mac the popup reopens when I click on the
> triggering (Open) button. Without this patch this does not happen. The
> testcase I used was:
> 
> <button label="Open"
>         oncommand="document.getElementById('panel').openPopup(this,
> 'after_start')"/>
> <panel id="panel" type="arrow">
>   <button label="Hello"/>
> </panel>

Thanks for pointing this out. I can reproduce this as well and its definitely not intended. I'll look into it.
(Assignee)

Comment 20

2 months ago
(In reply to Neil Deakin from comment #17)
> When I tested the latest patch on Mac the popup reopens when I click on the
> triggering (Open) button. Without this patch this does not happen. The
> testcase I used was:
> 
> <button label="Open"
>         oncommand="document.getElementById('panel').openPopup(this,
> 'after_start')"/>
> <panel id="panel" type="arrow">
>   <button label="Hello"/>
> </panel>

The patch moves the transition from the poupup (outer) node, to its "container" child node. (i.e. comment #18 is not quite correct - there are 2 animations, but on a different element) This means that nsLayoutUtils::HasCurrentTransitions(popupFrame) returns false when a hiding transition is taking place: http://searchfox.org/mozilla-central/source/layout/xul/nsXULPopupManager.cpp#1619

I'm not sure of the best way to fix this. One possibility is to add a way to look up where the transition is taking place and check that element for current transitions. Another possibility is to add something like a "canceling" animate attribute value, and wait for that to change from the popupManager to know when to call HidePopupCallback. 

The decision to transition the container element rather than the popup element is driven by the nature of the animation itself, and also a goal to ensure the animation happens on the compositor on all platforms - to give us the best chance of a smooth 60fps animation. If the possible options aren't going to work out, option 3 is to leave the popupmanager and arrow-panel binding as-is and just adjust the existing animations in xul.css. Your thoughts Neil?
Flags: needinfo?(enndeakin)

Updated

2 months ago
Iteration: 55.4 - May 1 → 55.5 - May 15
The easiest is probably to just check nsLayoutUtils::HasCurrentTransitions() on both the popup and its first child frame and treat an animation as present if a transition exists on either one.
Flags: needinfo?(enndeakin)
(In reply to Sam Foster [:sfoster] from comment #20)
> ensure the animation happens on the compositor on all platforms - to give us
> the best chance of a smooth 60fps animation.

It's not clear to me why a transform on the panel wouldn't do this. Could you explain?
(Assignee)

Comment 23

2 months ago
(In reply to Dão Gottwald [::dao] from comment #22)
> (In reply to Sam Foster [:sfoster] from comment #20)
> > ensure the animation happens on the compositor on all platforms - to give us
> > the best chance of a smooth 60fps animation.
> 
> It's not clear to me why a transform on the panel wouldn't do this. Could
> you explain?

The reasoning I'm following is given in bug 1291457. Reading it again, I need to validate my interpretation of that though. If that entire popup window is non-accelerated it will presumably make no difference which of the elements gets the transition? In which case a simpler patch on this bug will work (and bringing 1291457 into scope for photon may be the better path.) I'm not sure how this applies to other platforms though. 

Markus, can you advise here? The new animation shown in attachment 8857004 [details] is a short opacity fade in, and the panel slides (transforms) about 8px into place: 

  transition-property: opacity, transform;
  transition-duration: 70ms, 100ms;
  transition-timing-function: cubic-bezier(.07,.95,0,1);

..i.e. we don't need the transform-origin anymore. Does it make any difference if this animation takes place on the <panel> or one of its children? We also have the option here to slide the contents into the panel meaning we don't necessarily need to move the window and its shadow, only fade it in/out, if that helps.
Flags: needinfo?(mstange)
(In reply to Sam Foster [:sfoster] from comment #23)
> If that entire popup
> window is non-accelerated it will presumably make no difference which of the
> elements gets the transition?

That's correct. The entire popup window is currently non-accelerated, so it makes no difference which element gets the transition.

> We also have the option here to slide the contents into the panel
> meaning we don't necessarily need to move the window and its shadow, only
> fade it in/out, if that helps.

That would help a lot! The biggest problem in bug 1291457 (as far as I'm concerned) is the fact that we need to use private APIs in order to be able to transform the window including its shadow. But if we just need to change the window's *opacity*, and the shadow can stay in place, then we don't need private APIs.
Flags: needinfo?(mstange)

Updated

a month ago
Iteration: 55.5 - May 15 → 55.6 - May 29
Hey Sam, can you put a patch up for review here that only does the opacity change?
Flags: needinfo?(sfoster)
(Assignee)

Comment 26

a month ago
(In reply to Jared Wein [:jaws] (behind on reviews and bugmail) (please needinfo? me) from comment #25)
> Hey Sam, can you put a patch up for review here that only does the opacity
> change?

It needs a little more work than that. Currently the transform-origin property is being used as a proxy for identifying an arrow-panel element type in c++ land, so removing the transform means finding a different way to do this. I think.
Flags: needinfo?(sfoster)
(In reply to Sam Foster [:sfoster] from comment #26)
> (In reply to Jared Wein [:jaws] (behind on reviews and bugmail) (please
> needinfo? me) from comment #25)
> > Hey Sam, can you put a patch up for review here that only does the opacity
> > change?
> 
> It needs a little more work than that. Currently the transform-origin
> property is being used as a proxy for identifying an arrow-panel element
> type in c++ land, so removing the transform means finding a different way to
> do this. I think.

Can we leave the tranform-origin property as-is but just remove it from the transition?

Updated

25 days ago
Iteration: 55.6 - May 29 → 55.7 - Jun 12
(Assignee)

Comment 28

22 days ago
(In reply to Tim Nguyen :ntim from comment #9)
> I would back out a few parts of bug 610545 (which changed from a simple
> slide and fade animation to a more complicated scaling animation) because it
> introduced some regressions.

I've looked through the patches on 610545. They include the TransitionEnder and generally making opening/closing async/event-driven. I dont think that part changes even with a simpler animation. Having a single property transition does weed out a class of potential bugs and some complexity as we'll only get the one transitionend event. But I've not spotted any changes/simpliciations that I can make to take advantage of that outside of the CSS itself. Were there specific regressions you had in mind?
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 29

22 days ago
Created attachment 8873657 [details] [diff] [review]
WIP patch to animate opacity only when opening/closing arrow-panels

I'm tracking down a test failure in toolkit/content/tests/chrome/test_arrowpanel.xul, in the meantime Gijs, could you take a look over this and see if this is what you would expect? We're replacing the current fade and transform-out-from-corner animation on all arrow-panels and the boomarks menu with a short opacity transition using the photon easing. 

The idea with this simplified patch is to get a baseline landed in nightly & under MOZ_PHOTON_ANIMATIONS in which we avoid animations on these panels/menu which can't be run on the compositor. I.e. no animation (or minimal animation) is better than janky animation. And then triage the task of adding the movement back in - per platform - weighing the effort vs. reward. 

The current treatment on nightly actually runs just fine on the reference photon hardware, but is known to jank sometimes on OSX for example.
Attachment #8873657 - Flags: feedback?(gijskruitbosch+bugs)
(Assignee)

Updated

22 days ago
Attachment #8858468 - Attachment is obsolete: true
(In reply to Sam Foster [:sfoster] from comment #29)
> The idea with this simplified patch is to get a baseline landed in nightly &
> under MOZ_PHOTON_ANIMATIONS in which we avoid animations on these
> panels/menu which can't be run on the compositor. I.e. no animation (or
> minimal animation) is better than janky animation. And then triage the task
> of adding the movement back in - per platform - weighing the effort vs.
> reward.

Is there evidence for the current animation being janky?

Comment 31

22 days ago
(In reply to Sam Foster [:sfoster] from comment #28)
> Were there specific regressions you had in mind?

I recall there is/was a regression about scrollbars showing up at the wrong times. I can't seem to find the bug number though.
Flags: needinfo?(ntim.bugs)

Comment 32

22 days ago
(In reply to Tim Nguyen :ntim from comment #31)
> (In reply to Sam Foster [:sfoster] from comment #28)
> > Were there specific regressions you had in mind?
> 
> I recall there is/was a regression about scrollbars showing up at the wrong
> times. I can't seem to find the bug number though.

bug 940733? This still happens today, but it's after the panel is already open. There's also bug 994194 and bug 1088637.

Comment 33

22 days ago
Comment on attachment 8873657 [details] [diff] [review]
WIP patch to animate opacity only when opening/closing arrow-panels

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

I see no visual difference between this and no animation at all (though the latter breaks some things, like clicking the anchor to close a panel). I don't know if that's caused by the duration or the easing function or what, but to me it feels like this makes panels appear instantly, at which point, well, why bother having a transition?
Attachment #8873657 - Flags: feedback?(gijskruitbosch+bugs) → feedback-
(Assignee)

Comment 34

22 days ago
(In reply to Dão Gottwald [::dao] from comment #30)
> Is there evidence for the current animation being janky?

There is bug 1291457 (OSX). We also currently exclude linux/GTK from any animation/transition of these popups - it would be nice to do something there. I have not seen jank on windows personally and I just checked the existing animations on the reference hardware (win10) yesterday. But I wasn't running a ton of other stuff so that may not be conclusive.
GTK is excluded due to lack of support for transparent windows at the time. Things have changed since then, but I'm not sure how widespread old window managers without support for transparency might still be. Anyway, this is unrelated to animation jank.
(Assignee)

Comment 36

22 days ago
(In reply to Dão Gottwald [::dao] from comment #35)
> GTK is excluded due to lack of support for transparent windows at the time.
> Things have changed since then, but I'm not sure how widespread old window
> managers without support for transparency might still be. Anyway, this is
> unrelated to animation jank.

OK, that's good to know. So we are really talking about finding a reasonable compromise for OSX, and AFAICT we should be good to implement the animation as-designed for Windows. I've done some testing and 150-180ms seems to be the sweet spot for OSX - giving a slight transition to the popup opening without introducing noticeable lag between the user click and the result. As we already use 150ms, I'll run with that. I don't really like fragmenting the user experience - and CSS rules - like this, but these are different platforms so I guess that's just the reality of it.

Updated

18 days ago
Depends on: 1370131

Updated

18 days ago
Depends on: 1291457
No longer depends on: 1370131

Updated

11 days ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
(Assignee)

Comment 37

10 days ago
Notes from UX review with :amylee and :epang: 

Looks great on Windows

On the mac, check on menu sub-panel closing - there is some flickering? 
(:sfoster) not sure this patch affects that, but worth checking there's no regression

Some panels seem to show faster than others
(:sfoster) They all use the same animation-duration and the photon easing curve, but we may be missing frames if the panel takes time to construct and/or display?
(Assignee)

Comment 38

10 days ago
Here's a build with 0 animation for opening/closing the arrow-panels on OSX: https://archive.mozilla.org/pub/firefox/try-builds/sfoster@mozilla.com-6d292b09ef605abde095fb32d7fdbdd4a785cb1f/try-macosx64/

Feedback from UX (:epang)

    11:52 .. think it’s much better then the fade
    11:52 though if we can have the same motions as on windows that’s still our best option
    11:53 @sfoster but I would say this is a safe backup.

We'll plan a follow-up bug to enable the animations when bug 1291457 / bug 1370034 land.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 days ago
Attachment #8877721 - Attachment is obsolete: true
Attachment #8877721 - Flags: review?(dtownsend)
Attachment #8877721 - Flags: review?(dao+bmo)
(Assignee)

Comment 42

10 days ago
:mossop, I need your blessing on the xul.css changes as Neal is out.
(Assignee)

Comment 43

10 days ago
I think this is ready for review. The work in bug 1291457 / bug 1370034 should allow us to land a follow-up patch to remove the exclusion of OSX and give it the same transform animation as windows. 

The try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6085b79ffb89c18eedffb02ad196a4d6624e358&selectedJob=104439542 shows some test timeouts in browser/components/extensions/test/browser/browser_ext_browserAction_popup.js  but as this patch doesnt change the duration or fundamental nature of the popup animation (the same element is animating opacity and transform) I'm not sure what to make of it.
(Assignee)

Comment 44

9 days ago
Comment on attachment 8877720 [details]
Bug 1352075 - Add Photon arrow-panel animation for Mac+Windows.

In-coming update to this patch, to remove/cleanup some of the idefs.
Attachment #8877720 - Attachment is obsolete: true
Attachment #8877720 - Flags: review?(dtownsend)
Attachment #8877720 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(In reply to :Gijs from comment #33)
> Comment on attachment 8873657 [details] [diff] [review]
> WIP patch to animate opacity only when opening/closing arrow-panels
> 
> Review of attachment 8873657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see no visual difference between this and no animation at all (though the
> latter breaks some things, like clicking the anchor to close a panel). I
> don't know if that's caused by the duration or the easing function or what,
> but to me it feels like this makes panels appear instantly, at which point,
> well, why bother having a transition?

Is the latest patch still using the same animation that Gijs wasn't seeing?

When I try it, I do see the animation, but it's not easily recognizable as such. Panels seem to just quickly jerk downwards a bit in a way that looks like it could also be an unintended glitch. It's not clear to me that this is better than the current animation, and I agree that at this point we might as well drop the animation altogether.
(Assignee)

Comment 47

9 days ago
> Is the latest patch still using the same animation that Gijs wasn't seeing?

No, we removed the opacity-only animation for OSX as it was too subtle and just felt like the panel opened a bit slower than you might expect. The new fade-and-drop-into-place animation only plays on Windows at this point. I did go through that with :amylee and :epang, but lets get their input again. We noted that the animation does not play as smoothly the first time. I assume this is because we lazily initialize these panels, and I'm not sure what if anything we can do about that. 

> When I try it, I do see the animation, but it's not easily recognizable as
> such. Panels seem to just quickly jerk downwards a bit in a way that looks
> like it could also be an unintended glitch. It's not clear to me that this
> is better than the current animation, and I agree that at this point we
> might as well drop the animation altogether.

I've tested this on the reference windows laptop, and to me the opening animation seems quite smooth, and eases the appearance of the panel quite nicely. I guess I disagree that no animation would be better than the new animation. We are planning on enabling this animation on OSX as well as soon as bug 1291457 has landed. Amy/Eric can you add your assessment to this bug? Despite the time invested, the goal remains to make the user experience better, and we need some consensus that this patch represents an improvement before we proceed (or not)
Flags: needinfo?(epang)

Comment 48

8 days ago
mozreview-review
Comment on attachment 8877720 [details]
Bug 1352075 - Add Photon arrow-panel animation for Mac+Windows.

https://reviewboard.mozilla.org/r/149150/#review153648

r+ for the xul.css piece
Attachment #8877720 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 49

8 days ago
Summarizing conversation on slack with :epang, re: comment #46. I'm going to try adjusting the easing function on the opacity animation so its not so steep (maybe ease-out), and extend both to 200ms. No other substantive changes to the patch.
(Assignee)

Comment 50

8 days ago
(In reply to Sam Foster [:sfoster] from comment #49)
> Summarizing conversation on slack with :epang, re: comment #46. I'm going to
> try adjusting the easing function on the opacity animation so its not so
> steep (maybe ease-out), and extend both to 200ms. No other substantive
> changes to the patch.

Try build with these changes: 
https://archive.mozilla.org/pub/firefox/try-builds/sfoster@mozilla.com-6218855b787f47512cb13a9c5edf37b3c6574813/
(In reply to Dão Gottwald [::dao] from comment #46)
> (In reply to :Gijs from comment #33)
> > Comment on attachment 8873657 [details] [diff] [review]
> > WIP patch to animate opacity only when opening/closing arrow-panels
> > 
> > Review of attachment 8873657 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I see no visual difference between this and no animation at all (though the
> > latter breaks some things, like clicking the anchor to close a panel). I
> > don't know if that's caused by the duration or the easing function or what,
> > but to me it feels like this makes panels appear instantly, at which point,
> > well, why bother having a transition?
> 
> Is the latest patch still using the same animation that Gijs wasn't seeing?
> 
> When I try it, I do see the animation, but it's not easily recognizable as
> such. Panels seem to just quickly jerk downwards a bit in a way that looks
> like it could also be an unintended glitch. It's not clear to me that this
> is better than the current animation, and I agree that at this point we
> might as well drop the animation altogether.


Thanks Dão, I appreciate the feedback! I'm working on finding a balance between having the motion be recognizable/intended yet fast/snappy. With the correct balance we gain the feeling of refinement and polish.  As well a sense that someone has put some thought and design into the browser.  IMHO the animation is a huge improvement over the current animation, but I agree we need to evaluate it against no animation.  With no animation - yes it's faster, but we lose smoothness and refinement.  I've started tweaking the motion and I'm confident we'll be able to find a balance. Stay tuned...
Flags: needinfo?(epang)
Hey Sam,

Can you update the css to the following?  It feels much smoother and much more intention.
Give it a try and let me know what you think.


panel[type="arrow"]:not([animate="false"]) {
  opacity: 0;
  transform: translateY(-70px);
  transition-property: transform, opacity;
  transition-duration: 0.18s, 0.18s;
  trainsition-delay: 0, 0;
  transition-timing-function:
    var(--animation-easing-function), ease-out;
}

panel[type="arrow"][side="bottom"]:not([animate="false"]) {
  transform: translateY(10px);
}

panel[type="arrow"][animate="open"] {
  opacity: 1.0;
  transform: none;
  transition-property: transform, opacity;
  transition-duration: 0.20s, 0.10s;
  trainsition-delay: 0, 0;
  transition-timing-function:
    var(--animation-easing-function), ease-in-out;
}
Flags: needinfo?(sfoster)
Comment hidden (mozreview-request)
(Assignee)

Comment 54

4 days ago
Thanks Eric, that looks pretty good. 

:Dao, I've updated the patch for review using the animation values from comment #52. This hopefully strikes a better balance between being fast/snappy and intentional-looking.
Flags: needinfo?(sfoster) → needinfo?(dao+bmo)
(Assignee)

Updated

4 days ago
Attachment #8873657 - Attachment is obsolete: true
Since this is probably going to land after bug 1291457 merges to central, I think it makes sense to rebase your patch on top of that bug and to make the animation apply on Mac as well.
(In reply to Sam Foster [:sfoster] from comment #47)
> We noted that the animation does not play as smoothly the first time. I
> assume this is because we lazily initialize these panels, and I'm not sure
> what if anything we can do about that.

I think this was part of the problem I saw. This seems pretty bad. Users don't necessarily bookmark multiple pages, or open the identity panel multiple times in a session, etc., so there's a good chance that they'll see the crippled animation more often than the intended one.
Flags: needinfo?(dao+bmo)
And yes, this needs to be rebased now that bug 1291457 has landed.
Attachment #8877720 - Flags: review?(dao+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 59

3 days ago
Re-requesting review, as the patch now adds OSX support with the -moz-window-transform and -moz-window-opacity properties, building on :mstange's changes in bug 1291457. 

(In reply to Dão Gottwald [::dao] from comment #56)
> (In reply to Sam Foster [:sfoster] from comment #47)
> > We noted that the animation does not play as smoothly the first time. I
> > assume this is because we lazily initialize these panels, and I'm not sure
> > what if anything we can do about that.
> 
> I think this was part of the problem I saw. This seems pretty bad. Users
> don't necessarily bookmark multiple pages, or open the identity panel
> multiple times in a session, etc., so there's a good chance that they'll see
> the crippled animation more often than the intended one.

I agree we should try and address this. I need to establish if this patch actually makes the problem worse as I would rather tackle this in a separate bug and avoid scope creep here if possible. I think the old animation masks these issues because the popup window is scaled very small as it opens, so its harder to spot any jitter. Regardless, if the user experience is ultimately worse for this patch, I'm open to ideas for how to fix it.
(Assignee)

Comment 60

3 days ago
:mstange, :epang, I've got builds for windows and osx up at https://archive.mozilla.org/pub/firefox/try-builds/sfoster@mozilla.com-9227441e3548ed109b222603551506c96f8f764a/, could you possible try them (or the patch) to see if the animation matches your expectations? On OSX I'm seeing a little stutter right at the end of the transform, like it moves the last 2-3 pixels in one step.
Flags: needinfo?(mstange)
Flags: needinfo?(epang)
I see the same thing. I think the window server is rounding the translation value of window transforms that are pure translations (i.e. which don't contain scale or rotation) to device pixels, and it might be rounding down instead of rounding to nearest. Tomorrow I'll try to find out whether doing our own rounding for translations works around the issue.

Comment 62

3 days ago
mozreview-review
Comment on attachment 8877720 [details]
Bug 1352075 - Add Photon arrow-panel animation for Mac+Windows.

https://reviewboard.mozilla.org/r/149150/#review156124

I tested this again on Windows 10 and it does look better.

::: browser/base/content/browser.css:1063
(Diff revision 5)
>    border: none;
>    /* The popup inherits -moz-image-region from the button, must reset it */
>    -moz-image-region: auto;
>  }
>  
> -%ifdef MOZ_WIDGET_COCOA
> +%if defined(MOZ_PHOTON_ANIMATIONS)

ifdef

::: browser/base/content/browser.css:1065
(Diff revision 5)
>    -moz-image-region: auto;
>  }
>  
> -%ifdef MOZ_WIDGET_COCOA
> -
> +%if defined(MOZ_PHOTON_ANIMATIONS)
> +/* Photon arrow-panel animations */
> +%if defined(MOZ_WIDGET_COCOA)

ifdef

::: browser/base/content/browser.css:1102
(Diff revision 5)
> +}
> +
> +#BMB_bookmarksPopup[arrowposition] {
> +  -moz-window-transform-origin: 50% 50% 0;
> +}
> +%elif !defined(MOZ_WIDGET_GTK)

elifndef

::: browser/base/content/browser.css:1212
(Diff revision 5)
>  #BMB_bookmarksPopup[arrowposition="before_end"]:-moz-locale-dir(ltr),
>  #BMB_bookmarksPopup[arrowposition="before_start"]:-moz-locale-dir(rtl) {
>    transform-origin: calc(100% - 20px) bottom;
>  }
>  %endif
>  

I think you're missing an endif here?

::: toolkit/content/xul.css:446
(Diff revision 5)
>  
>  panel[type="arrow"] {
>    -moz-binding: url("chrome://global/content/bindings/popup.xml#arrowpanel");
>  }
>  
> -%ifdef MOZ_WIDGET_COCOA
> +%if defined(MOZ_PHOTON_ANIMATIONS)

ifdef

::: toolkit/content/xul.css:447
(Diff revision 5)
>  panel[type="arrow"] {
>    -moz-binding: url("chrome://global/content/bindings/popup.xml#arrowpanel");
>  }
>  
> -%ifdef MOZ_WIDGET_COCOA
> -
> +%if defined(MOZ_PHOTON_ANIMATIONS)
> +/* Photon arrow-panel animations */

Please get rid of "Photon" as the ifdef already makes this clear and the photon codename isn't going to be a useful reference post-57.

::: toolkit/content/xul.css:448
(Diff revision 5)
>    -moz-binding: url("chrome://global/content/bindings/popup.xml#arrowpanel");
>  }
>  
> -%ifdef MOZ_WIDGET_COCOA
> -
> +%if defined(MOZ_PHOTON_ANIMATIONS)
> +/* Photon arrow-panel animations */
> +%if defined(MOZ_WIDGET_COCOA)

ifdef

::: toolkit/content/xul.css:486
(Diff revision 5)
> +
> +panel[arrowposition] {
> +  -moz-window-transform-origin: 50% 50% 0;
> +}
> +
> +%elif !defined(MOZ_WIDGET_GTK)

elifndef
Attachment #8877720 - Flags: review?(dao+bmo) → review-
(In reply to Sam Foster [:sfoster] from comment #60)
> :mstange, :epang, I've got builds for windows and osx up at
> https://archive.mozilla.org/pub/firefox/try-builds/sfoster@mozilla.com-
> 9227441e3548ed109b222603551506c96f8f764a/, could you possible try them (or
> the patch) to see if the animation matches your expectations? On OSX I'm
> seeing a little stutter right at the end of the transform, like it moves the
> last 2-3 pixels in one step.

yeah i'm seeing the same thing.  It definitely feels like a glitch.  Let's see what Markus can do, if we can't fix it then we might have to remove the animation on osx.
Flags: needinfo?(epang)
I spoke to Stephen and the feedback was that it was better to keep the animation (even with the initial opening glitch) than removing it altogether. Hopefully we can get it fixed though.
Depends on: 1375232
(In reply to Markus Stange [:mstange] from comment #61)
> Tomorrow I'll try to find out whether doing
> our own rounding for translations works around the issue.

It does, and I've put a patch to do that into bug 1375232.
Flags: needinfo?(mstange)
Comment hidden (mozreview-request)

Comment 67

2 days ago
mozreview-review
Comment on attachment 8877720 [details]
Bug 1352075 - Add Photon arrow-panel animation for Mac+Windows.

https://reviewboard.mozilla.org/r/149150/#review156648

::: browser/base/content/browser.css:1126
(Diff revision 6)
> +  transform: none;
> +}
> +
> +#BMB_bookmarksPopup[arrowposition] {
> +  transform-origin: 50% 50% 0;
> +}

What's the point of this rule?
(Assignee)

Comment 68

a day ago
(In reply to Dão Gottwald [::dao] from comment #67)
> ::: browser/base/content/browser.css:1126
> (Diff revision 6)
> > +  transform: none;
> > +}
> > +
> > +#BMB_bookmarksPopup[arrowposition] {
> > +  transform-origin: 50% 50% 0;
> > +}
> 
> What's the point of this rule?

Turns out it has none. I had some old notes suggesting we needed to keep this but just clarified with :mstange that it is not necessary. I'll update the patch to remove it here and in xul.css
Comment hidden (mozreview-request)

Comment 70

15 hours ago
mozreview-review
Comment on attachment 8877720 [details]
Bug 1352075 - Add Photon arrow-panel animation for Mac+Windows.

https://reviewboard.mozilla.org/r/149150/#review157256
Attachment #8877720 - Flags: review?(dao+bmo) → review+

Comment 71

15 hours ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/252482fcc324
Add Photon arrow-panel animation for Mac+Windows. r=dao,mossop
I had to back this out for being the apparent cause of a bunch of browser-chrome test failures on Windows
https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=6ca048e09deae06ac1cecbe14619cd2a289da1b3&noautoclassify&filter-searchStr=cb648dfde71a07cdd56e186e89f2015d8a5e0e79&group_state=expanded&selectedJob=109616167
https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=6ca048e09deae06ac1cecbe14619cd2a289da1b3&noautoclassify&filter-searchStr=528f4f8bc0fede89c5b66d0851d2620e3def5953&group_state=expanded
https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=6ca048e09deae06ac1cecbe14619cd2a289da1b3&noautoclassify&filter-searchStr=7e790c758f5a98d234b6177e9907953c723e1f9e&group_state=expanded&selectedJob=109649943

(There was some build failure in the mix there, thus missing some of the interim test results)

https://hg.mozilla.org/integration/autoland/rev/8f840a75f3ffd9429273d98911a432e166a7d901
Flags: needinfo?(sfoster)
(Assignee)

Comment 73

10 hours ago
(In reply to Wes Kocher (:KWierso) from comment #72)
> I had to back this out for being the apparent cause of a bunch of
> browser-chrome test failures on Windows

Ok thanks. I have seen similar test results earlier in earlier try runs but hadn't been able to conclude my patch was causing them. That looks pretty conclusive; I'll investigate.
Flags: needinfo?(sfoster)
You need to log in before you can comment on or make changes to this bug.