Closed Bug 355846 Opened 16 years ago Closed 11 years ago

Add a pref to disable sliding effect for alerts

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: tv, Assigned: foss)

Details

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061003 Firefox/2.0

When using a remote visual display protocol (VNC, RDP on Windows, and remote X11), it's preferable to minimize "extra" visual effects.  Examples of the things that are highly undesirable in this situation are:

1. Gradients
2. High color icons
3. Mouse-hover highlights
4. Smooth scrolling/sliding

In Firefox 2.0 RC2, all of (1), (2), and (3) can be addressed with a third party theme that basically reverts the "visual refresh":

http://kmgerich.com/2006/08/05/firefox-1x-classic-themes-available/

However, (4) is a big problem.  Smooth scrolling of the content window already has a preference, but two other prefs are missing:

* smooth sliding of alert boxes in lower right (you can "speed it up" to reduce the number of pixel slides involved, but it's still a pain)

* smooth sliding of the alert bar at top (new effect in Firefox 2, which is VERY painful due to the smooth-sliding of the entire content window)

I would really like to see prefs, even if only available in prefs.js/about:config, to disable smooth scrolling for the alert boxes (long-standing issue) and the alert bar (to re-enable 1.5's effect of a single large jump).

This is classified as a bug, as the alert bar behavior is a regression vs. 1.5 in my eyes (1.5 did not exhibit this slowdown over remote visual protocols).


Reproducible: Always

Steps to Reproduce:
1. Use a remote visual protocol (VNC, RDP, X11) to run Firefox 2.0 RC2 with more latency than "on the same LAN".
2. Take any action that causes an alert box to slide on the bottom right, or the alert bar to slide down from the top.
3. Note how slow this is.

Actual Results:  
Over a long distance link, the alert bar basically brought the session to a halt as the window slide effect liasted for 20 seconds.


Expected Results:  
With preferences for the noted smooth slide/scroll situations appropriately set, the alert box should flash on and off the screen with no sliding, and the alert bar should have jumped down in one UI action as it did in Firefox 1.5.
Version: unspecified → 2.0 Branch
The Winstripe-based theme mentioned to resolve some of the undesirables (1)-(3) is now an Addon:

https://addons.mozilla.org/firefox/3479/
Alias: smooth-disable
This is not only still a problem in 3.0; it's now *worse* with the introduction of all sorts of alerts in sliding alert bars.  Hence bumped up severity and moved version to 3.0 branch.

Please can we have a way to turn off smooth window sliding?  RDP and VNC aren't meant for smooth scrolling, but they both can handle complete window movements ("copyrect" in VNC parlance) quite well.  It would be much nicer to be able to make the alert bar "pop" the window contents downward the full number of pixels in a single jump, rather than smoothly, when using a remote display.
Severity: normal → major
Version: 2.0 Branch → 3.0 Branch
IMO it's a good idea. I don't like very much the smooth effect of sliding windows. It should be added a way to disable it, through a theme or an user style.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: perf, polish
Version: 3.0 Branch → Trunk
"ue" keyword added.  This is a pretty adverse user experience problem in environments as described above.
Keywords: ue
Attached patch Patch for bug 355846 (obsolete) — Splinter Review
Adds a preference "alerts.disableSliddingEffect" to defaults/Firefox.js and, depending of if it is truo or false, acts as supposed. If it is "true" (the user set value, because, by default, it is "false"), then the alert box just pops, waits 1x slide time and then just closes.
Attachment #549613 - Flags: review?(ventnor.bugzilla)
Comment on attachment 549613 [details] [diff] [review]
Patch for bug 355846

I wonder if, as a future project, converting this alert to the JS animation API would help, especially in terms of X traffic.

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>+pref("alerts.disableSliddingEffect", false);

Spelled "Sliding"

>diff --git a/toolkit/components/alerts/resources/content/alert.js b/toolkit/components/alerts/resources/content/alert.js
> function animateCloseAlert()
> {
>-  if (gCurrentSize > 1)
>+  if (gCurrentSize > 1 && (!(gDisableSlideAlert)))

Too many parentheses, just !gDisableSlideAlert will do.
Attachment #549613 - Flags: review?(ventnor.bugzilla) → review-
Attached patch Patch v1.1 (obsolete) — Splinter Review
With corrections from Comment 6.
Attachment #549613 - Attachment is obsolete: true
Attachment #549746 - Flags: review?(ventnor.bugzilla)
(In reply to comment #6)
> 
> I wonder if, as a future project, converting this alert to the JS animation
> API would help, especially in terms of X traffic.

http://hacks.mozilla.org/2010/08/more-efficient-javascript-animations-with-mozrequestanimationframe/

Do you mean using this? It seems easy to integrate. I will take a look into this. In the meantime, will you create a new bug?
Comment on attachment 549746 [details] [diff] [review]
Patch v1.1

Yes, that's it, but this page is more up to date:

https://developer.mozilla.org/en/DOM/window.mozRequestAnimationFrame

since it mentions the simpler callback version.
Attachment #549746 - Flags: review?(ventnor.bugzilla) → review+
Severity: major → normal
Component: Preferences → General
Flags: checkin?(dao)
Keywords: perf, polish, ue
Product: Firefox → Toolkit
QA Contact: preferences → general
Comment on attachment 549746 [details] [diff] [review]
Patch v1.1

>--- a/toolkit/components/alerts/resources/content/alert.js
>+++ b/toolkit/components/alerts/resources/content/alert.js
>@@ -43,17 +43,17 @@ const NS_ALERT_TOP = 4;
> 
> var gFinalSize;
> var gCurrentSize = 1;
> 
> var gSlideIncrement = 1;
> var gSlideTime = 10;
> var gOpenTime = 3000; // total time the alert should stay up once we are done animating.
> var gOrigin = 0; // Default value: alert from bottom right, sliding in vertically.
>-
>+var gDisableSlideAlert = false;
> var gAlertListener = null;
> var gAlertTextClickable = false;
> var gAlertCookie = "";
> 
> function prefillAlertInfo()
> {
>   // unwrap all the args....
>   // arguments[0] --> the image src url
>@@ -96,16 +96,17 @@ function onAlertLoad()
>   try 
>   {
>     var prefService = Components.classes["@mozilla.org/preferences-service;1"].getService();
>     prefService = prefService.QueryInterface(Components.interfaces.nsIPrefService);
>     var prefBranch = prefService.getBranch(null);
>     gSlideIncrement = prefBranch.getIntPref("alerts.slideIncrement");
>     gSlideTime = prefBranch.getIntPref("alerts.slideIncrementTime");
>     gOpenTime = prefBranch.getIntPref("alerts.totalOpenTime");
>+    gDisableSlideAlert = prefBranch.getBoolPref("alerts.disableSlidingEffect");
>   }
>   catch (ex)
>   {
>   }

This is broken. It means that if you don't set all of alerts.slideIncrement, alerts.slideIncrementTime and alerts.totalOpenTime, setting alerts.disableSlidingEffect won't take effect.
Attachment #549746 - Flags: review-
(In reply to comment #10)
> > var gFinalSize;
> > var gCurrentSize = 1;
> > 
> > var gSlideIncrement = 1;
> > var gSlideTime = 10;
> > var gOpenTime = 3000; // total time the alert should stay up once we are done animating.

> > var gOrigin = 0; // Default value: alert from bottom right, sliding in vertically.

[...]

> >     var prefService = Components.classes["@mozilla.org/preferences-service;1"].getService();
> >     prefService = prefService.QueryInterface(Components.interfaces.nsIPrefService);
> >     var prefBranch = prefService.getBranch(null);
> >     gSlideIncrement = prefBranch.getIntPref("alerts.slideIncrement");
> >     gSlideTime = prefBranch.getIntPref("alerts.slideIncrementTime");
> >     gOpenTime = prefBranch.getIntPref("alerts.totalOpenTime");

> This is broken. It means that if you don't set all of alerts.slideIncrement,
> alerts.slideIncrementTime and alerts.totalOpenTime, setting
> alerts.disableSlidingEffect won't take effect.

alerts.slideIncrement is read to gSlideIncrement, alerts.slideIncrementTime to gSlideTime and alerts.totalOpenTime to gOpenTime. All three are set in the code before reading the preference.
... and if alerts.slideIncrement isn't set in the default prefs file, getIntPref will throw, and none of the subsequent prefs will be read.
(In reply to comment #12)
> ... and if alerts.slideIncrement isn't set in the default prefs file,
> getIntPref will throw, and none of the subsequent prefs will be read.

Defaulting to true when setting gDisableSlideAlert? (not on my development computer now to post a patch directly)
No, the whole try/catch block is just wrong.
The default prefs should probably be set in all.js, then you don't need to catch exceptions at all.
Attached patch Patch v1.2 (obsolete) — Splinter Review
Preferences set in all.js to remove the try/catch block.
Attachment #549746 - Attachment is obsolete: true
Attachment #550424 - Flags: review?(dao)
Attachment #550424 - Attachment is patch: true
Comment on attachment 550424 [details] [diff] [review]
Patch v1.2

> function animateCloseAlert()
> {
>-  if (gCurrentSize > 1)
>+  if (gCurrentSize > 1 && gDisableSlideAlert)
>   {
>     animate(-gSlideIncrement);
>     setTimeout(animateCloseAlert, gSlideTime);
>   }
>   else
>     closeAlert();
> }

Don't you want !gDisableSlideAlert here?
Assignee: nobody → leofigueres
Attached patch Patch v1.2.1 (obsolete) — Splinter Review
Correction for last patch, where I forgot to check for "false" instead of "true".
Attachment #550424 - Attachment is obsolete: true
Attachment #550472 - Flags: review?(dao)
Attachment #550424 - Flags: review?(dao)
Attachment #550472 - Flags: review?(enndeakin)
Attachment #550472 - Flags: review?(dao)
Attachment #550472 - Flags: feedback+
Comment on attachment 550472 [details] [diff] [review]
Patch v1.2.1

> function animateAlert()
> {
>   if (gCurrentSize < gFinalSize)
>   {
>-    animate(gSlideIncrement);
>+    if (gDisableSlideEffect)
>+      animate(gFinalSize);
>+    else
>+      animate(gSlideIncrement);

I'm not familar with the alert code, but it looks ok.

The animate and aimateAlert functions look like they doesn't handle if gCurrentSize + step ends up being larger than gFinalSize, and allows the size to end up being gFinalSize plus some amount of step, so calling animate(gFinalSize) might not be quite correct.

It seems like it would be easier to just return early from onAlertLoad without changing the window size or calling the animation timeout.
Attachment #550472 - Flags: review?(enndeakin) → review+
Is this code better?

  if (!gDisableSlideAlert)
    setTimeout(animateAlert, gSlideTime);
  else
  {
    animate(gFinalSize-gCurrentSize);
    setTimeout(animateCloseAlert, gOpenTime); // We need to pause the process so the box is seen for a lapse of time.
  }

It goes where we call to the animateAlert. Then, the animate and animateAlert will be left without changes. The animateCloseAlert needs to be modified (with the same code of the patch) because a pause is needed before the box is closed.

In order to avoid the mentioned problem with the gFinalSize+step, the animate function is called with the final size minus the size of origin. I have not hard-coded "1" so any future change could be done more easely.
(In reply to Neil Deakin from comment #20)

> The animate and aimateAlert functions look like they doesn't handle if
> gCurrentSize + step ends up being larger than gFinalSize, and allows the
> size to end up being gFinalSize plus some amount of step, so calling
> animate(gFinalSize) might not be quite correct.

So is this a new bug? By default, every step is of 1. If the step equals to 1, there should be no problem. But, if somebody changes manually the default setting, the bug you mentioned appears.

> 
> It seems like it would be easier to just return early from onAlertLoad
> without changing the window size or calling the animation timeout.

See Comment 21 for the feedback I need, as I have tried to implement the code this way.
There's a better chance for getting feedback if you attach a new patch and request review again.
Attached patch Patch (obsolete) — Splinter Review
See Comment 20 and comment 21
Attachment #554121 - Flags: review?(enndeakin)
Comment on attachment 554121 [details] [diff] [review]
Patch

>-  setTimeout(animateAlert, gSlideTime);
>+  if (!gDisableSlideAlert)
>+    setTimeout(animateAlert, gSlideTime);
>+  else
>+  {
>+    animate(gFinalSize-gCurrentSize);

Add some spaces around the '-'

But since this patch doesn't really fix the underlying issue, I think the first patch is better.

As I mentioned earlier, if any extra cleanup is done here, it should be to ensure that the window size isn't changed at all when the setting is disabled.
Attachment #554121 - Flags: review?(enndeakin)
(In reply to Neil Deakin from comment #25)

> But since this patch doesn't really fix the underlying issue, I think the
> first patch is better.

Doesn't fix the issue? I have tested it, and the window (the alert box) pops up and then dissapears, without any efect. Wasn't it the problem indicated in the description of this bug?

This and that patch are very similar. In this one I tried to do the way you indicated: before calling the animation function. If the final code is easier to understand for future coders, there is no problem from my point of view to do it.

> 
> As I mentioned earlier, if any extra cleanup is done here, it should be to
> ensure that the window size isn't changed at all when the setting is
> disabled.

I introduced a change in this version of the patch to try to work-around the bug of the sliceIncrement preference setting.

Then, if you prefer the other version of the patch, and there is still a + in the review, it is ok to close the bug and ask for chekin?

Is it ok now, Dao?
If everything is addressed, you can add the checkin-needed keyword. Make sure that patches that shouldn't land are marked as obsolete.
Attachment #554121 - Attachment is obsolete: true
Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=9a567d609584

Assuming all green, will push to inbound after.

I'm happy to fix it this time, but for new patches could you make sure the patch format (context, commit message, author etc) is correct when requesting checkin-needed - thanks! :-)
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Status: NEW → ASSIGNED
Keywords: checkin-needed
The try run in comment 28 was busted due to another checkin-needed patch in that push.

However, even after submitting again without that other patch, this still fails try on Windows:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=9a900f3a77eb

Perma-orange M5
http://tbpl.allizom.org/php/getParsedLog.php?id=6057258#error0
{
WARNING: Subdocument container has no content: file e:/builds/moz2_slave/try-w32-dbg/build/layout/base/nsDocumentViewer.cpp, line 2402
WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file e:/builds/moz2_slave/try-w32-dbg/build/gfx/layers/d3d9/Nv3DVUtils.cpp, line 84
JavaScript error: chrome://global/content/alerts/alert.js, line 151: gDisableSlideEffect is not defined
--DOMWINDOW == 92 (0B8B9090) [serial = 288] [outer = 082769E0] [url = http://mochi.test:8888/tests/security/ssl/stricttransportsecurity/test_stricttransportsecurity.html]
--DOMWINDOW == 91 (0C833858) [serial = 319] [outer = 00000000] [url = http://mochi.test:8888/tests/security/ssl/stricttransportsecurity/test_sts_privatebrowsing.html]
--DOMWINDOW == 90 (0856BCA0) [serial = 360] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 89 (0856B910) [serial = 358] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 88 (0856B580) [serial = 356] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 87 (0856B1F0) [serial = 354] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 86 (0B8BA098) [serial = 351] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 85 (0CAD4520) [serial = 345] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 84 (0CAD46E8) [serial = 343] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 83 (0CC12AD8) [serial = 341] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 82 (0CC11C98) [serial = 339] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 81 (0CC13588) [serial = 336] [outer = 00000000] [url = about:blank]
--DOMWINDOW == 80 (0C832DA8) [serial = 324] [outer = 00000000] [url = about:blank]
249 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/alerts/test/test_alerts.html | Test timed out.
}
Alias: smooth-disable
Summary: Smooth scroll/slide disable preferences needed → Add a pref to disable sliding effect for alerts
(In reply to Ed Morley [:edmorley] from comment #29)
> The try run in comment 28 was busted due to another checkin-needed patch in
> that push.
> 
> However, even after submitting again without that other patch, this still
> fails try on Windows:
> http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=9a900f3a77eb
> 
> Perma-orange M5
> http://tbpl.allizom.org/php/getParsedLog.php?id=6057258#error0
> {
> WARNING: Subdocument container has no content: file
> e:/builds/moz2_slave/try-w32-dbg/build/layout/base/nsDocumentViewer.cpp,
> line 2402
> WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file
> e:/builds/moz2_slave/try-w32-dbg/build/gfx/layers/d3d9/Nv3DVUtils.cpp, line
> 84
> JavaScript error: chrome://global/content/alerts/alert.js, line 151:
> gDisableSlideEffect is not defined

My fault. It should be gDisableSlideAlert (being consistent with the preference key name). Will fix the patch, test it on my equipment and ask for a review.

Thank you, Ed. Next time will follow the guidelines for the checkin-needed keyword.
Attached patch Patch v1.3Splinter Review
Disabling the sliding effect if set on the about:config preference, avoids to resize the alert box if new size is greater than final size. In this case, the new size will be just the final size.

Patch follows (at least tries to) guidelines by Bonardo.
Attachment #550472 - Attachment is obsolete: true
Attachment #554890 - Flags: review?(dao)
Attachment #554890 - Flags: review?(dao) → review?(enndeakin)
Comment on attachment 554890 [details] [diff] [review]
Patch v1.3

(In reply to Javi Rueda from comment #26)
> (In reply to Neil Deakin from comment #25)
> 
> > But since this patch doesn't really fix the underlying issue, I think the
> > first patch is better.
> 
> Doesn't fix the issue? I have tested it, and the window (the alert box) pops
> up and then dissapears, without any efect. Wasn't it the problem indicated
> in the description of this bug?

Sorry, I was referring to my comment to "ensure that the window size isn't changed at all when the setting is disabled"

The patch here fixing this particular bug is good.
Attachment #554890 - Flags: review?(enndeakin) → review+
(In reply to Neil Deakin from comment #32)
> Comment on attachment 554890 [details] [diff] [review]
> Patch v1.3
> Sorry, I was referring to my comment to "ensure that the window size isn't
> changed at all when the setting is disabled"
> 

Thank you for reviewing the code, Neil. Sorry for misunderstand you.

Inside animateAlert, the modified code activates only when the preference is "true", so any other value will run the code as normally it would do (animating the box). Also, in the animate itself, it will try to asure that the final size is not greater than it should be. This last is about another previous comment.

> The patch here fixing this particular bug is good.

Then, asking for check-in this patch.
Flags: in-testsuite?
http://hg.mozilla.org/mozilla-central/rev/87a72f57507e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.