alert showing/hiding animation is janky (nsIAlertsService)

VERIFIED FIXED in Firefox 17

Status

()

defect
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

Trunk
mozilla18
All
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17+ verified)

Details

(Whiteboard: [Fx17 for Social API][Fx17])

Attachments

(2 attachments, 10 obsolete attachments)

12.29 KB, patch
Unfocused
: review+
dao
: review+
Dolske
: superreview+
Details | Diff | Splinter Review
87.67 KB, application/x-xpinstall
Details
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.
Whiteboard: [Fx17 for Social API] → [Fx17 for Social API][Fx17]
is this a social api bug, or a panel bug?  iirc, we have no custom css in the social patches after landing the flyout.
Technically this is a Toolkit > Notifications bug, as the toast notifications are part of Toolkit. This bug is more evident with Social API though.
Blocks: 774506
No longer blocks: 763718
Component: Themes → XUL Widgets
Summary: Notification showing/hiding animation is janky → alert showing/hiding animation is janky (nsIAlertsService)
Posted patch Rough WIP patch (obsolete) — Splinter Review
This is the rough WIP patch that I had started a couple weeks ago.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #665210 - Attachment is obsolete: true
Attachment #665636 - Flags: review?(neil)
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.
Attachment #665636 - Flags: review?(neil)
Posted patch Patch v2 (obsolete) — Splinter Review
Thanks for catching that Dao. I added a separate code path for Linux which only toggles visibility.
Attachment #665636 - Attachment is obsolete: true
Attachment #666016 - Flags: review?(felipc)
Attachment #666016 - Flags: review?(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.
Yeah, it works. I can change it to
> transition: 0s visibility ease 4s;
so it is less magical :)
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".
Attachment #666016 - Flags: review?(dao) → review-
Posted patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #666016 - Attachment is obsolete: true
Attachment #666016 - Flags: review?(felipc)
Attachment #666763 - Flags: review?(dao)
Posted patch Patch v3.1 (obsolete) — Splinter Review
With some minor tweaks to the CSS animation rules as recommended by Frank to reduce duplication.
Attachment #666763 - Attachment is obsolete: true
Attachment #666763 - Flags: review?(dao)
Attachment #666886 - Flags: review?(dao)
Posted patch Patch v3.1.1 (obsolete) — Splinter Review
Removed an unused variable.
Attachment #666886 - Attachment is obsolete: true
Attachment #666886 - Flags: review?(dao)
Attachment #666888 - Flags: review?(dao)
Attachment #666888 - Flags: review?(bmcbride)
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.
Attachment #666888 - Flags: review?(dao) → review-
Posted patch Patch v4 (obsolete) — Splinter Review
Attachment #666888 - Attachment is obsolete: true
Attachment #666888 - Flags: review?(bmcbride)
Attachment #667179 - Flags: review?(dao)
Attachment #667179 - Flags: review?(bmcbride)
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.
Posted patch Patch v4.1 (obsolete) — Splinter Review
Attachment #667179 - Attachment is obsolete: true
Attachment #667179 - Flags: review?(dao)
Attachment #667179 - Flags: review?(bmcbride)
Attachment #667315 - Flags: review?(dao)
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).
Attachment #667179 - Attachment is obsolete: false
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.
Attachment #667315 - Flags: review-
Attachment #667179 - Attachment is obsolete: true
Posted patch Patch v4.2 (obsolete) — Splinter Review
Now only showing the fade animation if alerts.disableSlidingEffect is false.
Attachment #667315 - Attachment is obsolete: true
Attachment #667315 - Flags: review?(dao)
Attachment #667330 - Flags: review?(bmcbride)
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.
Attachment #667330 - Flags: review?(bmcbride) → review+
Posted patch Patch for checkin (obsolete) — Splinter Review
Attachment #667330 - Attachment is obsolete: true
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?
Attachment #667341 - Flags: review-
(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.
(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.
Posted patch Patch 4.3Splinter Review
Thanks for the feedback, I like this patch better now that there isn't multiple animation names.
Attachment #667341 - Attachment is obsolete: true
Attachment #667816 - Flags: review?(dao)
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.
Attachment #667816 - Flags: review?(dao) → review+
Wouldn't have hurt to wait for my review and add it to the commit message after I spent significant time on this.
Attachment #667816 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d946b507ebc2
https://hg.mozilla.org/mozilla-central/rev/984026798238
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
   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.
    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|
Depends on: 798217
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.
(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 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
Attachment #667816 - Flags: approval-mozilla-aurora?
Attachment #667816 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Attachment #667816 - Flags: superreview?(dtownsend+bugmail)
Comment on attachment 667816 [details] [diff] [review]
Patch 4.3

[Triage Comment]
https://bugzilla.mozilla.org/show_bug.cgi?id=796111#c21
Attachment #667816 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta+
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.
Attachment #667816 - Flags: superreview?(dtownsend+bugmail) → superreview+
Since you've made winstripe different from gnomestripe, what do you recommend to the authors of third-party themes?
Is there something QA can do to verify this fix?
Whiteboard: [Fx17 for Social API][Fx17] → [Fx17 for Social API][Fx17][qa?]
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.
Keywords: verifyme
Whiteboard: [Fx17 for Social API][Fx17][qa?] → [Fx17 for Social API][Fx17]
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 995351
Depends on: 1308051
You need to log in before you can comment on or make changes to this bug.