Closed Bug 1355927 Opened 7 years ago Closed 7 years ago

Notification bars should use the photon animation curve when appearing/disappearing

Categories

(Toolkit :: Themes, enhancement, P1)

Product:
Toolkit
Component:
Themes
Version:
55 Branch
Type:
enhancement
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Iteration:
55.5 - May 15
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jaws, Assigned: squib)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-animation])

Attachments

(1 file)

Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing
59 bytes, text/x-review-board-request
jaws
: review+
dao
: review+
Details 
Similar to bug 1355589, we should use the same animation bezier-curve when opening and closing notification bars.
Flags: qe-verify+
Priority: -- → P2
QA Contact: jwilliams
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 55.4 - May 1
Iteration: 55.4 - May 1 → 55.5 - May 15
Depends on: 1362128
Component: General → Themes
Product: Firefox → Toolkit
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

Requesting an additional review for the changes to notification.xml.

In testing out my code, I realized that you couldn't disable animations when showing a new notification; the option was there, but it animated anyway. With this patch, you can disable animations for both showing and hiding notifications, and I hook into this with the new `toolkit.cosmeticAnimations.enabled` pref.

I manually tested this on http://www.popuptest.com/popuptest1.html with both states of the animation pref and everything seems to be working. I'll also kick off a try build to make sure I didn't break any tests. If you (or :jaws) would like additional tests anywhere, just let me know.
Attachment #8865935 - Flags: review?(dao+bmo)
No longer depends on: 1362128
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

https://reviewboard.mozilla.org/r/135924/#review140668

r=me with the following two changes

::: browser/base/content/browser.css:1258
(Diff revision 1)
>  #context-navigation > .menuitem-iconic > .menu-accel-container {
>    display: none;
>  }
>  
> +notification.animated {
> +  /* This overrides the transition in toolkit/content/xul.css to use the Photon

This comment should just read:
/* This overrides the transition defined in toolkit/content/xul.css */

We should try to stay away from using the word "Photon" in code because the name will quickly get out of date.

::: toolkit/content/widgets/notification.xml:273
(Diff revision 1)
>          <body>
>            <![CDATA[
>              this._finishAnimation();
>  
>              var height = aNotification.boxObject.height;
> -            var skipAnimation = aSkipAnimation || (height == 0);
> +            var skipAnimation = aSkipAnimation || (height == 0) ||

nit, while you're changing this line please drop the parens around the `height == 0` since they're not necessary.
Attachment #8865935 - Flags: review?(jaws) → review+
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

https://reviewboard.mozilla.org/r/135924/#review140682

::: browser/base/content/browser.css:1257
(Diff revision 1)
>  #context-navigation > .menuitem-iconic > .menu-iconic-text,
>  #context-navigation > .menuitem-iconic > .menu-accel-container {
>    display: none;
>  }
>  
> +notification.animated {

I think we should probably just change this in xul.css rather than overriding in browser.css.
Attachment #8865935 - Flags: review?(dao+bmo)
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

https://reviewboard.mozilla.org/r/135924/#review140708

::: browser/base/content/browser.css:1257
(Diff revision 1)
>  #context-navigation > .menuitem-iconic > .menu-iconic-text,
>  #context-navigation > .menuitem-iconic > .menu-accel-container {
>    display: none;
>  }
>  
> +notification.animated {

I did it this way since --animation-easing-function is in /browser/, not /toolkit/. Trying to use --animation-easing-function from /toolkit/ would break Thunderbird. I could define it in /toolkit/ by using the cubic-bezier(...) value though...

::: browser/base/content/browser.css:1258
(Diff revision 1)
>  #context-navigation > .menuitem-iconic > .menu-accel-container {
>    display: none;
>  }
>  
> +notification.animated {
> +  /* This overrides the transition in toolkit/content/xul.css to use the Photon

Fixed.

::: toolkit/content/widgets/notification.xml:273
(Diff revision 1)
>          <body>
>            <![CDATA[
>              this._finishAnimation();
>  
>              var height = aNotification.boxObject.height;
> -            var skipAnimation = aSkipAnimation || (height == 0);
> +            var skipAnimation = aSkipAnimation || (height == 0) ||

Fixed.
(In reply to Jim Porter (:squib) from comment #6)
> ::: browser/base/content/browser.css:1257
> (Diff revision 1)
> >  #context-navigation > .menuitem-iconic > .menu-iconic-text,
> >  #context-navigation > .menuitem-iconic > .menu-accel-container {
> >    display: none;
> >  }
> >  
> > +notification.animated {
> 
> I did it this way since --animation-easing-function is in /browser/, not
> /toolkit/. Trying to use --animation-easing-function from /toolkit/ would
> break Thunderbird. I could define it in /toolkit/ by using the
> cubic-bezier(...) value though...

You could define the CSS variable in xul.css.
True. I might go that way, but I'm asking the Thunderbird devs what they think. If they want the old animation, it might be nicer for us to define the new animation in /browser/ so it doesn't trickle down to TB.
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

https://reviewboard.mozilla.org/r/135924/#review140682

> I think we should probably just change this in xul.css rather than overriding in browser.css.

Moved this to xul.css since the TB team is ok with the new animation.
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

https://reviewboard.mozilla.org/r/135924/#review140788

::: toolkit/content/widgets/notification.xml:243
(Diff revision 3)
>                  this.removeNotification(notifications[n]);
>              }
>              this.currentNotification = null;
>  
>              // Must clear up any currently animating notification
> -            if (aImmediate)
> +            if (aImmediate || !this._allowAnimation)

I don't think I see the point of this change. I thought maybe this should be 'aImmediate && this._allowAnimation' instead, but then I saw that _finishAnimation checks this._animating.
Attachment #8865935 - Flags: review+
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

https://reviewboard.mozilla.org/r/135924/#review140788

> I don't think I see the point of this change. I thought maybe this should be 'aImmediate && this._allowAnimation' instead, but then I saw that _finishAnimation checks this._animating.

It's an edge case, but I think we'd need it if we created a notification *with* animations, then immediately disabled animations and closed the noficiation via `removeAllNotifications` before the original animation finished. Without this change, I'm pretty sure `this._animating` would remain true after `removeAllNotifications` returned.
(In reply to Jim Porter (:squib) from comment #12)
> Comment on attachment 8865935 [details]
> Bug 1355927 - Notification bars should use the photon animation curve when
> appearing/disappearing
> 
> https://reviewboard.mozilla.org/r/135924/#review140788
> 
> > I don't think I see the point of this change. I thought maybe this should be 'aImmediate && this._allowAnimation' instead, but then I saw that _finishAnimation checks this._animating.
> 
> It's an edge case, but I think we'd need it if we created a notification
> *with* animations, then immediately disabled animations and closed the
> noficiation via `removeAllNotifications` before the original animation
> finished. Without this change, I'm pretty sure `this._animating` would
> remain true after `removeAllNotifications` returned.

Makes sense. Could you please add a brief comment on this above that line?
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

https://reviewboard.mozilla.org/r/135924/#review140788

> It's an edge case, but I think we'd need it if we created a notification *with* animations, then immediately disabled animations and closed the noficiation via `removeAllNotifications` before the original animation finished. Without this change, I'm pretty sure `this._animating` would remain true after `removeAllNotifications` returned.

I've added a comment explaining this in a bit more detail.
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

Re-requesting review since I think I've changed enough that it should have another quick review pass... hopefully the interdiff shows the changes correctly.
Attachment #8865935 - Flags: review+ → review?(jaws)
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

https://reviewboard.mozilla.org/r/135924/#review141124
Attachment #8865935 - Flags: review+
Comment on attachment 8865935 [details]
Bug 1355927 - Notification bars should use the photon animation curve when appearing/disappearing

https://reviewboard.mozilla.org/r/135924/#review141214
Attachment #8865935 - Flags: review?(jaws) → review+
Pushed by squibblyflabbetydoo@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9240e979a42a
Notification bars should use the photon animation curve when appearing/disappearing  r=dao,jaws
Whooops, I just realized that this should have gotten review from enndeakin per the comment in xul.css. However, his Bugzilla nick says he's out until August 9, so hopefully the fact that I got review from two people is sufficient here. If not, we can back this out.
https://hg.mozilla.org/mozilla-central/rev/9240e979a42a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Could you please provide a spec for what to expect from a QA perspective? Thanks
Flags: needinfo?(jaws)
You can use http://www.popuptest.com/popuptest2.html to test the notification bar appearing and disappearing. You can compare with an old Nightly prior to this change. The animation should *appear* quicker, but other than that there shouldn't be any noticeable changes.

Also, the animation should not occur if the toolkit.cosmeticAnimations.enabled pref is set to false.
Flags: needinfo?(jaws)
I did notice a minor ux issue; the Notification Bar is reloaded every time the page is reloaded when toolkit.cosmeticAnimations.enabled = "true"

When toolkit.cosmeticAnimations.enabled = "false" the Notification Bar stays.

See video below:

https://jwilliams-softvision.tinytake.com/sf/MTY3NjM1MV81NjA2ODQz

Please reopen if this is worth reopening. I'll hold off on Verifying until I hear back.
Flags: needinfo?(jaws)
Can you file a separate bug for that? I don't want to back out the patch that landed here, and we usually only reopen if the patch is backed out. I agree that it looks like something we could/should fix, though it will be low priority.
Flags: needinfo?(jaws)
Got it. I will do that now.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1371426
Depends on: 1371426
See Also: 1371426 →
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: