Closed Bug 1352075 Opened 7 years ago Closed 7 years ago

Implement new animation for opening/closing the arrow panels

Categories

(Toolkit :: UI Widgets, enhancement, P1)

enhancement
Points:
3

Tracking

()

VERIFIED FIXED
mozilla57
Iteration:
57.1 - Aug 15
Tracking Status
firefox57 --- fixed

People

(Reporter: jaws, Assigned: sfoster)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [photon-animation])

Attachments

(2 files, 10 obsolete files)

186.26 KB, video/mp4
Details
59 bytes, text/x-review-board-request
dao
: review+
mossop
: review+
Details
Attached video Doorhanger01.mp4 (obsolete) —
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.
Attached video Doorhanger02.mp4 (obsolete) —
Attached video Doorhanger03.mp4 (obsolete) —
Attached video Doorhanger04.mp4 (obsolete) —
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Whiteboard: [photon]
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
Whiteboard: [photon] → [photon-animation]
Points: --- → 3
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
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)
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.
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)
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 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.
> > +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)
Iteration: --- → 55.4 - May 1
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)
> 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)
> (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.
(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.
(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)
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?
(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)
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)
(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?
Iteration: 55.6 - May 29 → 55.7 - Jun 12
(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)
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)
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?
(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)
(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 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-
(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.
(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.
Depends on: 1370131
Depends on: 1291457
No longer depends on: 1370131
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
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?
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.
Attachment #8877721 - Attachment is obsolete: true
Attachment #8877721 - Flags: review?(dtownsend)
Attachment #8877721 - Flags: review?(dao+bmo)
:mossop, I need your blessing on the xul.css changes as Neal is out.
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.
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)
(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.
> 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 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+
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.
(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)
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)
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)
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.
: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 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 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?
(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 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+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/252482fcc324
Add Photon arrow-panel animation for Mac+Windows. r=dao,mossop
(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)
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
(In reply to Sam Foster [:sfoster] from comment #73)
> (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.

I took a look at your patch and made the following changes:

In browser-places.js, I added a popupshowing and transitionend event listener. In popupshowing I set the "animating" attribute if animations are enabled. Then in the transitionend event listener I remove this attribute. In xul.css, pointer-events are disabled while the "animating" attribute is set. This fixes the browser_bookmark_popup.js failures, since we were getting a mouseout event when the popup appeared which was canceling the timeout.

In xul.css, I changed the selector for the default non-animating properties to include [side] in the selector. This fixes the browser_ext_browserAction_popup_resize.js failures for me, which was happening because the test uses a popup that has [side="bottom"]. This failed because there is other CSS that you have defined that has a higher specificity and sets `transform: translateY(70px);` for [side="bottom"] popups. This new selector now has equal specificity and overrides the `translateY(70px)` because it appears later in the CSS file.
Comment on attachment 8883659 [details]
Bug 1352075 - Implement new animation for opening/closing the arrow panels.

Cancelling request for now. Lets look at the try results and talk about this Monday.
Attachment #8883659 - Flags: review?(sfoster)
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Depends on: 1380065
Attachment #8877720 - Attachment is obsolete: true
Comment on attachment 8883659 [details]
Bug 1352075 - Implement new animation for opening/closing the arrow panels.

I've folded the new changes in this patch into another patch.
Attachment #8883659 - Attachment is obsolete: true
Attachment #8888972 - Attachment is obsolete: true
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment on attachment 8889577 [details]
Bug 1352075 - Implement new animation for opening/closing the arrow panels.

https://reviewboard.mozilla.org/r/160598/#review166312

r+ for xul.css changes
Comment on attachment 8889577 [details]
Bug 1352075 - Implement new animation for opening/closing the arrow panels.

https://reviewboard.mozilla.org/r/160598/#review166316

::: browser/base/content/browser-places.js:214
(Diff revision 2)
> +        if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> +          this.panel.setAttribute("animating", "true");
> +        }
> +        break;
> +      case "transitionend":
> +        if (aEvent.propertyName == "transform" &&

This looks like it will fail on Mac as we don't transition "transform" there.

::: toolkit/content/xul.css:509
(Diff revision 2)
> +  transform: none;
> +}
> +
> +%endif
> +panel[type="arrow"][animating] {
> +  pointer-events: none;

Did you mean to do this for all arrow panels or only the bookmarks panel? Only browser-places.js seems to be setting the animating attribute. If that's the intent, then this code shouldn't live in xul.css.
Attachment #8889577 - Flags: review?(dao+bmo) → review-
Blocks: 1380065
No longer depends on: 1380065
Comment on attachment 8889577 [details]
Bug 1352075 - Implement new animation for opening/closing the arrow panels.

https://reviewboard.mozilla.org/r/160598/#review168880

::: browser/components/places/content/menu.xml:601
(Diff revision 3)
>          } else {
>            disablePointerEvents = this.getAttribute("disablepointereventsfortransition") == "true";
>          }
>          if (!disablePointerEvents) {
>            this.style.removeProperty("pointer-events");
>          }

So this legacy code does what's now being taken care of by the animating attribute, right? Can you remove this?
Comment on attachment 8889577 [details]
Bug 1352075 - Implement new animation for opening/closing the arrow panels.

https://reviewboard.mozilla.org/r/160598/#review168880

> So this legacy code does what's now being taken care of by the animating attribute, right? Can you remove this?

I left this as-is as the animating attribute is only applied by the arrowpanel binding - not menus, so this is still required. It does indeed accomplish the same thing. I could consolidate the 2 solutions in a follow-up?
Comment on attachment 8889577 [details]
Bug 1352075 - Implement new animation for opening/closing the arrow panels.

https://reviewboard.mozilla.org/r/160598/#review166316

> Did you mean to do this for all arrow panels or only the bookmarks panel? Only browser-places.js seems to be setting the animating attribute. If that's the intent, then this code shouldn't live in xul.css.

Agreed, I moved this to popup.xml / xul.css for all the arrowpanels (not including bookmarks menu)
(In reply to Sam Foster [:sfoster] from comment #86)
> I could consolidate the 2 solutions in a follow-up?

Okay.
Comment on attachment 8889577 [details]
Bug 1352075 - Implement new animation for opening/closing the arrow panels.

https://reviewboard.mozilla.org/r/160598/#review169148
Attachment #8889577 - Flags: review?(dao+bmo) → review+
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cd72fa60a74bcceaf1b6ab86ad420b399559899&selectedJob=120179790

I'm seeing that jetpack test failure again. Unless that's a symptom of something I need to worry about, I'd like to propose disabling that test. Otherwise, is there anything else of concern here?
Flags: needinfo?(wkocher)
(In reply to Sam Foster [:sfoster] from comment #90)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5cd72fa60a74bcceaf1b6ab86ad420b399559899&selectedJob=1
> 20179790
> 
> I'm seeing that jetpack test failure again.

Is jetpack not dead yet?
Comment on attachment 8889577 [details]
Bug 1352075 - Implement new animation for opening/closing the arrow panels.

https://reviewboard.mozilla.org/r/160598/#review169326
Attachment #8889577 - Flags: review?(dtownsend) → review+
> Is jetpack not dead yet?

Yes in word, but not in deed. I'm blocking on the bug to disable the jetpack tests.
Depends on: 1386694
(In reply to Sam Foster [:sfoster] from comment #93)
> > Is jetpack not dead yet?
> 
> Yes in word, but not in deed. I'm blocking on the bug to disable the jetpack
> tests.

TBH, if that's the only thing blocking this bug, I would just add a skip-if for the relevant test(s) and move on.
Bug 1386694 just disabled most of the SDK tests.
I also took a look at dom/canvas/test/webgl-conf/generated/test_2_conformance__ogles__GL__mat__mat_041_to_046.html - I can't reproduce this failure locally, and I looking at the test I can see no plausible connection to changes in arrow panel timing. Crossing fingers and pushing again.
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd62bc7fffe4
Implement new animation for opening/closing the arrow panels. r=dao,mossop
https://hg.mozilla.org/mozilla-central/rev/cd62bc7fffe4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
https://hg.mozilla.org/projects/date/rev/cd62bc7fffe4a0e01621872eee968fe656d04224
Bug 1352075 - Implement new animation for opening/closing the arrow panels. r=dao,mossop
QA Contact: jwilliams → stefan.georgiev
I tested this on latest nightly (BuildId:20170810100255) and it is verified as fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1393870
You need to log in before you can comment on or make changes to this bug.