Implement new animation for opening/closing the arrow panels

ASSIGNED
Assigned to

Status

()

Toolkit
XUL Widgets
P1
normal
ASSIGNED
a month ago
5 days ago

People

(Reporter: jaws, Assigned: sfoster)

Tracking

(Blocks: 1 bug)

unspecified
Points:
3
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-animation])

MozReview Requests

()

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

Attachments

(2 attachments, 4 obsolete attachments)

Created attachment 8852934 [details]
Doorhanger01.mp4

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

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

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

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

Proposal #4: The panel should appear instantly, but moving down about 5 pixels while appearing in a linear speed.
Blocks: 1349423
Created attachment 8852935 [details]
Doorhanger02.mp4
Created attachment 8852936 [details]
Doorhanger03.mp4
Created attachment 8852937 [details]
Doorhanger04.mp4
Assignee: nobody → sfoster
Status: NEW → ASSIGNED

Updated

a month ago
Whiteboard: [photon]
(Assignee)

Comment 4

18 days ago
Created attachment 8857004 [details]
Icon Rollover - Door Hanger.mp4

The selected door hanger / arrow-panel animations
Attachment #8852934 - Attachment is obsolete: true
Attachment #8852935 - Attachment is obsolete: true
Attachment #8852936 - Attachment is obsolete: true
Attachment #8852937 - Attachment is obsolete: true
Whiteboard: [photon] → [photon-animation]
Points: --- → 3

Updated

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

Comment 5

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

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

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

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

Comment 6

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

Comment 8

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

Comment 9

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

Comment 10

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

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

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

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

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

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

Comment 11

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

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

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

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

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

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

Updated

11 days ago
Iteration: --- → 55.4 - May 1
(Reporter)

Comment 12

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Comment 14

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

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

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

from xul.css:

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

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

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

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

I meant this:

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

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

16 even...

Comment 17

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

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

Comment 18

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

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

Comment 19

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

Thanks for pointing this out. I can reproduce this as well and its definitely not intended. I'll look into it.
You need to log in before you can comment on or make changes to this bug.