Closed
Bug 786125
Opened 9 years ago
Closed 9 years ago
alert showing/hiding animation is janky (nsIAlertsService)
Categories
(Toolkit :: XUL Widgets, defect)
Tracking
()
VERIFIED
FIXED
mozilla18
People
(Reporter: jaws, Assigned: jaws)
References
Details
(Whiteboard: [Fx17 for Social API][Fx17])
Attachments
(2 files, 10 obsolete files)
12.29 KB,
patch
|
Unfocused
:
review+
dao
:
review+
Dolske
:
superreview+
akeybl
:
approval-mozilla-beta+
|
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.
Assignee | ||
Updated•9 years ago
|
Whiteboard: [Fx17 for Social API] → [Fx17 for Social API][Fx17]
Comment 1•9 years ago
|
||
is this a social api bug, or a panel bug? iirc, we have no custom css in the social patches after landing the flyout.
Assignee | ||
Comment 2•9 years ago
|
||
Technically this is a Toolkit > Notifications bug, as the toast notifications are part of Toolkit. This bug is more evident with Social API though.
Assignee | ||
Updated•9 years ago
|
Updated•9 years ago
|
Component: Themes → XUL Widgets
Summary: Notification showing/hiding animation is janky → alert showing/hiding animation is janky (nsIAlertsService)
Assignee | ||
Comment 3•9 years ago
|
||
This is the rough WIP patch that I had started a couple weeks ago.
Assignee | ||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #665636 -
Flags: review?(neil)
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
Yeah, it works. I can change it to
> transition: 0s visibility ease 4s;
so it is less magical :)
Comment 9•9 years ago
|
||
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-
Assignee | ||
Comment 10•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
tracking-firefox17:
--- → ?
Assignee | ||
Comment 11•9 years ago
|
||
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)
Assignee | ||
Comment 12•9 years ago
|
||
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 13•9 years ago
|
||
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-
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #666888 -
Attachment is obsolete: true
Attachment #666888 -
Flags: review?(bmcbride)
Attachment #667179 -
Flags: review?(dao)
Attachment #667179 -
Flags: review?(bmcbride)
Comment 15•9 years ago
|
||
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.
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #667179 -
Attachment is obsolete: true
Attachment #667179 -
Flags: review?(dao)
Attachment #667179 -
Flags: review?(bmcbride)
Attachment #667315 -
Flags: review?(dao)
Comment 17•9 years ago
|
||
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 18•9 years ago
|
||
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-
Updated•9 years ago
|
Attachment #667179 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
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 20•9 years ago
|
||
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+
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #667330 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
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-
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 23•9 years ago
|
||
(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.
Updated•9 years ago
|
Comment 24•9 years ago
|
||
(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.
Assignee | ||
Comment 25•9 years ago
|
||
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 26•9 years ago
|
||
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+
Assignee | ||
Comment 27•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d946b507ebc2 https://hg.mozilla.org/integration/mozilla-inbound/rev/984026798238
Comment 28•9 years ago
|
||
Wouldn't have hurt to wait for my review and add it to the commit message after I spent significant time on this.
Updated•9 years ago
|
Attachment #667816 -
Flags: review+
Comment 29•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d946b507ebc2 https://hg.mozilla.org/mozilla-central/rev/984026798238
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
![]() |
||
Comment 30•9 years ago
|
||
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•9 years ago
|
||
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|
Assignee | ||
Comment 32•9 years ago
|
||
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•9 years ago
|
||
(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.
Assignee | ||
Comment 34•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #667816 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 35•9 years ago
|
||
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 36•9 years ago
|
||
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 37•9 years ago
|
||
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+
Assignee | ||
Comment 38•9 years ago
|
||
Landed on beta-17: https://hg.mozilla.org/releases/mozilla-beta/rev/e7aeda1be4b1 https://hg.mozilla.org/releases/mozilla-beta/rev/ec941b003e7e
status-firefox17:
--- → fixed
Comment 39•9 years ago
|
||
Since you've made winstripe different from gnomestripe, what do you recommend to the authors of third-party themes?
Comment 40•9 years ago
|
||
Is there something QA can do to verify this fix?
Whiteboard: [Fx17 for Social API][Fx17] → [Fx17 for Social API][Fx17][qa?]
Assignee | ||
Comment 41•9 years ago
|
||
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]
Comment 42•9 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•