355846 - Add a pref to disable sliding effect for alerts

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Todd Vierling]

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.

Comment 1

[Todd Vierling]

The Winstripe-based theme mentioned to resolve some of the undesirables (1)-(3) is now an Addon:

https://addons.mozilla.org/firefox/3479/

Comment 2

[Todd Vierling]

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.

Comment 3

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

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.

Comment 4

[Todd Vierling]

"ue" keyword added.  This is a pretty adverse user experience problem in environments as described above.

Comment 5

[Javi Rueda :javirid]

Attachment: Patch for bug 355846

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.

Comment 6

[Michael Ventnor]

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.

Comment 7

[Javi Rueda :javirid]

Attachment: Patch v1.1

With corrections from Comment 6.

Comment 8

[Javi Rueda :javirid]

(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 9

[Michael Ventnor]

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.

Comment 10

[Dão Gottwald [:dao]]

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.

Comment 11

[Javi Rueda :javirid]

(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.

Comment 12

[Dão Gottwald [:dao]]

... and if alerts.slideIncrement isn't set in the default prefs file, getIntPref will throw, and none of the subsequent prefs will be read.

Comment 13

[Javi Rueda :javirid]

(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)

Comment 14

[Dão Gottwald [:dao]]

No, the whole try/catch block is just wrong.

Comment 15

[Dão Gottwald [:dao]]

The default prefs should probably be set in all.js, then you don't need to catch exceptions at all.

Comment 16

[Javi Rueda :javirid]

Attachment: Patch v1.2

Preferences set in all.js to remove the try/catch block.

Comment 17

[Dão Gottwald [:dao]]

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?

Comment 18

[Javi Rueda :javirid]

Attachment: Patch v1.2.1

Correction for last patch, where I forgot to check for "false" instead of "true".

Comment 19

[Javi Rueda :javirid]

ping

Comment 20

[Neil Deakin]

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.

Comment 21

[Javi Rueda :javirid]

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.

Comment 22

[Javi Rueda :javirid]

(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.

Comment 23

[Dão Gottwald [:dao]]

There's a better chance for getting feedback if you attach a new patch and request review again.

Comment 24

[Javi Rueda :javirid]

Attachment: Patch

See Comment 20 and comment 21

Comment 25

[Neil Deakin]

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.

Comment 26

[Javi Rueda :javirid]

(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?

Comment 27

[Dão Gottwald [:dao]]

If everything is addressed, you can add the checkin-needed keyword. Make sure that patches that shouldn't land are marked as obsolete.

Comment 28

[Ed Morley [:emorley]]

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

Comment 29

[Ed Morley [:emorley]]

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.
}

Comment 30

[Javi Rueda :javirid]

(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.

Comment 31

[Javi Rueda :javirid]

Attachment: Patch v1.3

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.

Comment 32

[Neil Deakin]

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.

Comment 33

[Javi Rueda :javirid]

(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.

Comment 34

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/87a72f57507e

Comment 35

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/87a72f57507e