786125 - alert showing/hiding animation is janky (nsIAlertsService)

Status: VERIFIED FIXED

Description

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

The animation for showing and hiding the notifications is very janky. I believe it uses a JS timer to interpolate the positions along a bezier curve. We should either switch to using CSS transitions or rethink the necessity of these animations.

Comment 1

[Shane Caraveo (:mixedpuppy)]

is this a social api bug, or a panel bug?  iirc, we have no custom css in the social patches after landing the flyout.

Comment 2

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

Technically this is a Toolkit > Notifications bug, as the toast notifications are part of Toolkit. This bug is more evident with Social API though.

Comment 3

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

Attachment: Rough WIP patch

This is the rough WIP patch that I had started a couple weeks ago.

Comment 4

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

Attachment: Patch

This patch changes the animation to a fade-in animation and removes the slide in effect. It also offsets the alerts to be 10px from each closest edge since they are not sliding in anymore.

Comment 5

[Dão Gottwald [::dao]]

Comment on attachment 665636 [details] [diff] [review]
Patch

You forgot gnomestripe/global/alerts/alert.css. Note that a) we don't support translucent windows on Linux yet and b) your code depends on a transition happening.

Comment 6

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

Attachment: Patch v2

Thanks for catching that Dao. I added a separate code path for Linux which only toggles visibility.

Comment 7

[Dão Gottwald [::dao]]

Comment on attachment 666016 [details] [diff] [review]
Patch v2

>--- a/toolkit/themes/gnomestripe/global/alerts/alert.css
>+++ b/toolkit/themes/gnomestripe/global/alerts/alert.css
>@@ -59,8 +59,13 @@ label {
> }
> 
> .alertBox[orient="vertical"] > .alertTextBox {
>   -moz-padding-start: 5px;
>   -moz-padding-end: 5px;
>   margin-bottom: 8px;
>   -moz-box-align: center;
> }
>+
>+.alertBox[hide] {
>+  visibility: hidden;
>+  transition-delay: 4s;
>+}

Does this even work? If it does, it's too magical for my taste, as you never specify any other transition-* properties.

Comment 8

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

Yeah, it works. I can change it to
> transition: 0s visibility ease 4s;
so it is less magical :)

Comment 9

[Dão Gottwald [::dao]]

Comment on attachment 666016 [details] [diff] [review]
Patch v2

>+  if (navigator.platform.toLowerCase().contains("linux")) {
>+    alertBox.addEventListener("transitionend", function hideAlert() {
>+      alertBox.removeEventListener("transitionend", hideAlert, false);
>+      closeAlert();
>+    }, false);
>+    alertBox.setAttribute("hide", true);
>+  } else {
>+    alertBox.addEventListener("transitionend", function beginFadeOut() {
>+      alertBox.removeEventListener("transitionend", beginFadeOut, false);
>+      alertBox.removeAttribute("fadeIn");
>+      alertBox.addEventListener("transitionend", function endFadeOut() {
>+        alertBox.removeEventListener("transitionend", endFadeOut, false);
>+        closeAlert();
>+      }, false);
>+      alertBox.setAttribute("fadeOut", true);
>+    }, false);
>+    alertBox.setAttribute("fadeIn", true);
>   }

This case differentiation isn't really sane. A third-party theme could also do on Windows what you're doing on Linux. (In fact it probably should if it wants to work across platforms.) Can you make this code just not care about the fade-in transition and set a generic visibility transition in xul.css that winstripe can override with a visibility+opacity transition? In the transitionend listener, make sure event.propertyName is "visibility".

Comment 10

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

Attachment: Patch v3

With feedback from Frank, I switched to using a CSS animation which allows Windows to use a more complex animation while leaving a default animation of just hiding the alert when the animation is complete.

Comment 11

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

Attachment: Patch v3.1

With some minor tweaks to the CSS animation rules as recommended by Frank to reduce duplication.

Comment 12

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

Attachment: Patch v3.1.1

Removed an unused variable.

Comment 13

[Dão Gottwald [::dao]]

Comment on attachment 666888 [details] [diff] [review]
Patch v3.1.1

>--- a/toolkit/content/xul.css
>+++ b/toolkit/content/xul.css

>+/************ alert *************/
>+
>+#alertBox[animate] {
>+  animation-duration: 4s;
>+  animation-fill-mode: both;
>+  animation-name: alert-animation;
>+}
>+
>+@keyframes alert-animation {
>+  to {
>+    visibility: hidden;
>+  }
>+}

Let's add toolkit/components/alerts/resources/content/alert.css for this. I shouldn't have recommended xul.css, sorry.

>--- a/toolkit/themes/winstripe/global/alerts/alert.css
>+++ b/toolkit/themes/winstripe/global/alerts/alert.css

> .alertBox {
>   border: 1px solid threedshadow;
>+  border-radius: 3px;
>   background-color: -moz-Dialog;
>   min-height: 50px;
>   padding: 8px;
>+  transition-timing-function: ease-out;
> }

transition-timing-function appears to be an unneeded leftover.

Comment 14

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

Attachment: Patch v4

Comment 15

[Dão Gottwald [::dao]]

Comment on attachment 667179 [details] [diff] [review]
Patch v4

>+  alertBox.addEventListener("animationend", function hideAlert(e) {
>+    if (e.animationName == "alert-animation") {

s/e/event/

>-.alertBox[origin="top"] {
>-  border-top: none;
>-  border-bottom-left-radius: 3px;
>-  border-bottom-right-radius: 3px;
>-}
>-
>-.alertBox[origin="right"] {
>-  border-right: none;
>-  border-bottom-left-radius: 3px;
>-  border-top-left-radius: 3px;
>-}
>-
>-.alertBox[origin="bottom"] {
>-  border-bottom: none;
>-  border-top-left-radius: 3px;
>-  border-top-right-radius: 3px;
>-}
>-
>-.alertBox[origin="left"] {
>-  border-left: none;
>-  border-bottom-right-radius: 3px;
>-  border-top-right-radius: 3px;
>-}

You should revert http://hg.mozilla.org/mozilla-central/diff/ea6eb14209b1/toolkit/components/alerts/resources/content/alert.js as the origin attribute isn't useful anymore.

Comment 16

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

Attachment: Patch v4.1

Comment 17

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 667179 [details] [diff] [review]
Patch v4

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

What Dao said.

::: toolkit/components/alerts/resources/content/alert.css
@@ +1,1 @@
> +#alertBox[animate] {

File needs a license header.

::: toolkit/components/alerts/resources/content/alert.js
@@ -65,5 @@
> -{
> -  gSlideIncrement     = Services.prefs.getIntPref("alerts.slideIncrement");
> -  gSlideTime          = Services.prefs.getIntPref("alerts.slideIncrementTime");
> -  gOpenTime           = Services.prefs.getIntPref("alerts.totalOpenTime");
> -  gDisableSlideEffect = Services.prefs.getBoolPref("alerts.disableSlidingEffect");

We need to keep supporting the disableSlidingEffect pref (some history in bug 355846) - just like we have prefs to disable similar types of behaviour elsewhere (smooth scrolling, etc). Turns out this type of animation utterly *kills* performance in some situations that using CSS transitions can't help with (thin-clients/remote management/etc - particularly important in some organisations/schools/etc).

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 667315 [details] [diff] [review]
Patch v4.1

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

Bah, this is what I get for getting distracted half way through a review. See comment 17.

Comment 19

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

Attachment: Patch v4.2

Now only showing the fade animation if alerts.disableSlidingEffect is false.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 667330 [details] [diff] [review]
Patch v4.2

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

r+ with animation name tweak, as discussed on IRC.

Comment 21

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

Attachment: Patch for checkin

Comment 22

[Dão Gottwald [::dao]]

Comment on attachment 667341 [details] [diff] [review]
Patch for checkin

>--- a/toolkit/components/alerts/resources/content/alert.xul
>+++ b/toolkit/components/alerts/resources/content/alert.xul
>@@ -1,24 +1,26 @@
> <?xml version="1.0"?>
> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this
>    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> 
>+<?xml-stylesheet href="chrome://global/content/alerts/alert.css" type="text/css"?>
> <?xml-stylesheet href="chrome://global/skin/alerts/alert.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>

chrome://global/skin/alerts/alert.css already imports chrome://global/skin/.

It's slightly confusing that "animate" is set when you want no animation (i.e. the "immediate" case). I'm also not really happy with having alert-animation-hifi in addition to alert-animation ("hifi" doesn't make any sense, by the way; it means enhanced [sound] quality). Can you just bypass all the animation stuff and use setTimeout when the pref is set?

Comment 23

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

(In reply to Dão Gottwald [:dao] from comment #22)
> It's slightly confusing that "animate" is set when you want no animation
> (i.e. the "immediate" case). I'm also not really happy with having
> alert-animation-hifi in addition to alert-animation ("hifi" doesn't make any
> sense, by the way; it means enhanced [sound] quality). Can you just bypass
> all the animation stuff and use setTimeout when the pref is set?

Bypassing the animation and using setTimeout will needlessly complicate the code. It would also require sniffing for the platform since on Linux the presence of [immediate] doesn't make any difference.

Switching the names to be less confusing is fine, but I don't know why there is a problem with the current approach.

Comment 24

[Dão Gottwald [::dao]]

(In reply to Jared Wein [:jaws] from comment #23)
> (In reply to Dão Gottwald [:dao] from comment #22)
> > It's slightly confusing that "animate" is set when you want no animation
> > (i.e. the "immediate" case). I'm also not really happy with having
> > alert-animation-hifi in addition to alert-animation ("hifi" doesn't make any
> > sense, by the way; it means enhanced [sound] quality). Can you just bypass
> > all the animation stuff and use setTimeout when the pref is set?
> 
> Bypassing the animation and using setTimeout will needlessly complicate the
> code.

What code? What's needed beyond setTimeout(closeAlert, 4000)?

> It would also require sniffing for the platform since on Linux the
> presence of [immediate] doesn't make any difference.

No, Linux would be covered by toolkit/components/alerts/resources/content/alert.css when the animation isn't disabled via the pref.

Comment 25

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

Attachment: Patch 4.3

Thanks for the feedback, I like this patch better now that there isn't multiple animation names.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 667816 [details] [diff] [review]
Patch 4.3

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

r+ with the following:

::: toolkit/components/alerts/resources/content/alert.js
@@ +85,5 @@
>  
>    window.moveTo(x, y);
>  
> +  if (Services.prefs.getBoolPref("alerts.disableSlidingEffect")) {
> +    setTimeout(closeAlert, 4000);

Magic number should be a const at the start of the file.

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d946b507ebc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/984026798238

Comment 28

[Dão Gottwald [::dao]]

Wouldn't have hurt to wait for my review and add it to the commit message after I spent significant time on this.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d946b507ebc2
https://hg.mozilla.org/mozilla-central/rev/984026798238

Comment 30

[Philip Chee]

   1.12 -// pref to control the alert notification 
   1.13 -pref("alerts.slideIncrement", 1);
   1.14 -pref("alerts.slideIncrementTime", 10);
   1.15 -pref("alerts.totalOpenTime", 4000);
   1.16 -

So doesn't this affect:
http://mxr.mozilla.org/comm-central/source/mozilla/js/src/tests/e4x/Regress/regress-308111.js#58

And then there are left over prefs in b2g/android-mobile/xul-mobile/ etc etc.

Comment 31

[Philip Chee]

    1.12 +  const ALERT_DURATION_IMMEDIATE = 4000;
And if you are going to use a magic number, you could make it preffable and revive |alerts.totalOpenTime|

Comment 32

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

It doesn't affect that test because that test just needed a large e4x tree.

alerts.totalOpenTime is still used in Firefox for Android. I filed bug 798217 to remove the unused preferences.

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Philip Chee from comment #31)
>     1.12 +  const ALERT_DURATION_IMMEDIATE = 4000;
> And if you are going to use a magic number, you could make it preffable and
> revive |alerts.totalOpenTime|

Having it has a pref would be of limited usefulness, as that number only affects alerts when the fade in/out animation is disabled, which isn't the default.

Comment 34

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

Comment on attachment 667816 [details] [diff] [review]
Patch 4.3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): needed for social api, required for bug 796111 which was already aurora approved
User impact if declined: 
Testing completed (on m-c, etc.): been on m-c for a few days now
Risk to taking this patch (and alternatives if risky): none expected
String or UUID changes made by this patch: none

Comment 35

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

Comment on attachment 667816 [details] [diff] [review]
Patch 4.3

Flagging Dave for superreview to get his OK on uplifting this and bug 796111 to mozilla-beta (fx17).

Comment 36

[Alex Keybl [:akeybl]]

Comment on attachment 667816 [details] [diff] [review]
Patch 4.3

[Triage Comment]
https://bugzilla.mozilla.org/show_bug.cgi?id=796111#c21

Comment 37

[Justin Dolske [:Dolske]]

Comment on attachment 667816 [details] [diff] [review]
Patch 4.3

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

Stealing sr, as this is trying to land in time for the first beta. This seems pretty unlikely to have addon compat issues, so let's get it in.

Comment 38

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

Landed on beta-17:
https://hg.mozilla.org/releases/mozilla-beta/rev/e7aeda1be4b1
https://hg.mozilla.org/releases/mozilla-beta/rev/ec941b003e7e

Comment 39

[[e-mail]]

Since you've made winstripe different from gnomestripe, what do you recommend to the authors of third-party themes?

Comment 40

[Anthony Hughes (:ashughes) [QA]]

Is there something QA can do to verify this fix?

Comment 41

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

Attachment: Simple add-on for testing notification alerts

If you'd like to verify the results, you can install this attached add-on to both Firefox 16 and Firefox 17+. This add-on adds a toolbar button that when pressed will show a notification.

In Firefox 16 and lower, this uses a slide-in animation that gets slower the more notifications are present. In Firefox 17 and higher, the presence of many notifications should not slow down Firefox.

Comment 42

[Paul Silaghi, QA [:pauly]]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 using the STR in comment 41. The slide-in animation didn't get slower with multiple notification in Firefox 17 in comparison with FF 16.0.1