1352075 - Implement new animation for opening/closing the arrow panels

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, enhancement, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Doorhanger02.mp4

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Doorhanger03.mp4

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Doorhanger04.mp4

Comment 4

[Sam Foster [:sfoster] (he/him)]

Attachment: Icon Rollover - Door Hanger.mp4

The selected door hanger / arrow-panel animations

Comment 5

[Sam Foster [:sfoster] (he/him)]

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/[e-mail]-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.

Comment 6

[Sam Foster [:sfoster] (he/him)]

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 7

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1352075 - (WIP) Fade and slide in arrow-panel animation.

* Use vertical offset in arrowpanel and its arrowbox, and slide in the container
* Add WIP notes
* Known TODOs:
  * Apply similar treatment to bookmarks menu
  * left/right side arrowpanel instances?

Review commit: https://reviewboard.mozilla.org/r/130428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130428/

Comment 8

[Sam Foster [:sfoster] (he/him)]

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]

Comment 9

[Tim Nguyen :ntim]

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

[Dão Gottwald [:dao]]

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.

Comment 11

[Sam Foster [:sfoster] (he/him)]

> > +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.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 13

[Dão Gottwald [:dao]]

(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.

Comment 14

[Sam Foster [:sfoster] (he/him)]

> 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.

Comment 15

[Dão Gottwald [:dao]]

(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 [e-mail]

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.

Comment 16

[Dão Gottwald [:dao]]

(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...

Comment 17

[Neil Deakin]

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>

Comment 18

[Sam Foster [:sfoster] (he/him)]

> (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.

Comment 19

[Sam Foster [:sfoster] (he/him)]

(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.

Comment 20

[Sam Foster [:sfoster] (he/him)]

(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?

Comment 21

[Neil Deakin]

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.

Comment 22

[Dão Gottwald [:dao]]

(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?

Comment 23

[Sam Foster [:sfoster] (he/him)]

(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.

Comment 24

[Markus Stange [: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.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Hey Sam, can you put a patch up for review here that only does the opacity change?

Comment 26

[Sam Foster [:sfoster] (he/him)]

(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.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 28

[Sam Foster [:sfoster] (he/him)]

(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?

Comment 29

[Sam Foster [:sfoster] (he/him)]

Attachment: 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.

Comment 30

[Dão Gottwald [:dao]]

(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

[Tim Nguyen :ntim]

(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.

Comment 32

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

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?

Comment 34

[Sam Foster [:sfoster] (he/him)]

(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.

Comment 35

[Dão Gottwald [:dao]]

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.

Comment 36

[Sam Foster [:sfoster] (he/him)]

(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.

Comment 37

[Sam Foster [:sfoster] (he/him)]

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?

Comment 38

[Sam Foster [:sfoster] (he/him)]

Here's a build with 0 animation for opening/closing the arrow-panels on OSX: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-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 39

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1352075 - Add Photon arrow-panel animation for Mac+Windows.

Review commit: https://reviewboard.mozilla.org/r/149150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149150/

Comment 40

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1352075 - Add Photon arrow-panel animation for Windows only.

* Use new panel animation when opening arrow-panels (including bookmarks menu) to fade in and drop into position
* Linux/GTK is already excluded. Also exclude OSX due to jank. See bug 1291457
* The same element has its opacity and transform properties animated as before, so it doesnt change fundamentally
* New animation is non-directional (i.e. LTR vs. RTL)

Review commit: https://reviewboard.mozilla.org/r/149152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149152/

Comment 41

[Sam Foster [:sfoster] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149150/diff/1-2/

Comment 42

[Sam Foster [:sfoster] (he/him)]

:mossop, I need your blessing on the xul.css changes as Neal is out.

Comment 43

[Sam Foster [:sfoster] (he/him)]

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 44

[Sam Foster [:sfoster] (he/him)]

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.

Comment 45

[Sam Foster [:sfoster] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149150/diff/2-3/

Comment 46

[Dão Gottwald [:dao]]

(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.

Comment 47

[Sam Foster [:sfoster] (he/him)]

> 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)

Comment 48

[Dave Townsend [:mossop]]

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

Comment 49

[Sam Foster [:sfoster] (he/him)]

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.

Comment 50

[Sam Foster [:sfoster] (he/him)]

(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/[e-mail]-6218855b787f47512cb13a9c5edf37b3c6574813/

Comment 51

[Eric Pang [:epang] UX]

(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...

Comment 52

[Eric Pang [:epang] UX]

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;
}

Comment 53

[Sam Foster [:sfoster] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149150/diff/3-4/

Comment 54

[Sam Foster [:sfoster] (he/him)]

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.

Comment 55

[Markus Stange [:mstange]]

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.

Comment 56

[Dão Gottwald [:dao]]

(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.

Comment 57

[Dão Gottwald [:dao]]

And yes, this needs to be rebased now that bug 1291457 has landed.

Comment 58

[Sam Foster [:sfoster] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149150/diff/4-5/

Comment 59

[Sam Foster [:sfoster] (he/him)]

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.

Comment 60

[Sam Foster [:sfoster] (he/him)]

:mstange, :epang, I've got builds for windows and osx up at https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-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.

Comment 61

[Markus Stange [:mstange]]

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

[Dão Gottwald [:dao]]

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

Comment 63

[Eric Pang [:epang] UX]

(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/[e-mail]-
> 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.

Comment 64

[Amy Lee [:amylee] UX]

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.

Comment 65

[Markus Stange [:mstange]]

(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.

Comment 66

[Sam Foster [:sfoster] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149150/diff/5-6/

Comment 67

[Dão Gottwald [:dao]]

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?

Comment 68

[Sam Foster [:sfoster] (he/him)]

(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 69

[Sam Foster [:sfoster] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149150/diff/6-7/

Comment 70

[Dão Gottwald [:dao]]

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

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

Comment 71

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/252482fcc324
Add Photon arrow-panel animation for Mac+Windows. r=dao,mossop

Comment 72

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

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

Comment 73

[Sam Foster [:sfoster] (he/him)]

(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.

Comment 74

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1352075 - Implement new animation for opening/closing the arrow panels.

Review commit: https://reviewboard.mozilla.org/r/154572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154572/

Comment 75

[Jared Wein [:jaws] (please needinfo? me)]

(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 76

[Jared Wein [:jaws] (please needinfo? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154572/diff/1-2/

Comment 77

[Sam Foster [:sfoster] (he/him)]

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.

Comment 78

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1352075 - Implement new animation for opening/closing the arrow panels.

* Use new panel animation when opening arrow-panels (including bookmarks menu) to fade in and drop into position
* Make animation of bookmarks menu conditional on an animate attribute != "false" like arrowpanel binding
* Wait for popupshown rather than transitionend in bookmark reparenting test
* Linux/GTK is (still) excluded
* The same element has its opacity and transform properties animated as before, so it doesnt change fundamentally
* Add animating attribute to the panel to disable pointer-events during the transition (via :jaws)
* Fix specificity of CSS rules for panels/bookmarks-menu on edges other than the top (via :jaws)
* New animation is non-directional (i.e. LTR vs. RTL)

Review commit: https://reviewboard.mozilla.org/r/159988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159988/

Comment 79

[Sam Foster [:sfoster] (he/him)]

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.

Comment 80

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1352075 - Implement new animation for opening/closing the arrow panels.

* Use new panel animation when opening arrow-panels (including bookmarks menu) to fade in and drop into position
* Linux/GTK is (still) excluded
* The same element has its opacity and transform properties animated as before, so it doesnt change fundamentally
* New animation is non-directional (i.e. LTR vs. RTL)

This was landed then backed out due to test failures. New since last review:

* Add animating attribute to the panel to disable pointer-events during the transition (via :jaws)
* Builds on bug 1380065 and the toolkit.cosmeticAnimations.enabled pref to make arrow-panel animation conditional on that pr
* Make animation of bookmarks menu conditional on an animate attribute != "false" like arrowpanel binding
* Wait for popupshown rather than transitionend in bookmark reparenting test
* Fix specificity of CSS rules for panels/bookmarks-menu on edges other than the top (via :jaws)

Review commit: https://reviewboard.mozilla.org/r/160598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160598/

Comment 81

[Sam Foster [:sfoster] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160598/diff/1-2/

Comment 82

[Dave Townsend [:mossop]]

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 83

[Dão Gottwald [:dao]]

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.

Comment 84

[Sam Foster [:sfoster] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160598/diff/2-3/

Comment 85

[Dão Gottwald [:dao]]

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 86

[Sam Foster [:sfoster] (he/him)]

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 87

[Sam Foster [:sfoster] (he/him)]

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)

Comment 88

[Dão Gottwald [:dao]]

(In reply to Sam Foster [:sfoster] from comment #86)
> I could consolidate the 2 solutions in a follow-up?

Okay.

Comment 89

[Dão Gottwald [:dao]]

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

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

Comment 90

[Sam Foster [:sfoster] (he/him)]

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?

Comment 91

[Dão Gottwald [:dao]]

(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 92

[Dave Townsend [:mossop]]

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

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

Comment 93

[Sam Foster [:sfoster] (he/him)]

> Is jetpack not dead yet?

Yes in word, but not in deed. I'm blocking on the bug to disable the jetpack tests.

Comment 94

[:Gijs (he/him)]

(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.

Comment 95

[Dave Townsend [:mossop]]

Bug 1386694 just disabled most of the SDK tests.

Comment 96

[Sam Foster [:sfoster] (he/him)]

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.

Comment 97

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd62bc7fffe4
Implement new animation for opening/closing the arrow panels. r=dao,mossop

Comment 98

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cd62bc7fffe4

Comment 99

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/cd62bc7fffe4a0e01621872eee968fe656d04224
Bug 1352075 - Implement new animation for opening/closing the arrow panels. r=dao,mossop

Comment 100

[Abe - QA (:Abe_LV)]

I tested this on latest nightly (BuildId:20170810100255) and it is verified as fixed.