285904 - preferences transparency/fading violates aqua UI specs

Status: VERIFIED FIXED

(Toolkit :: Preferences, defect)

Description

[Josh Aas]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050308 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050308 Firefox/1.0+

Preference panes in aqua resize in transition, but they do not fade in and out
using transparency. That "trick" makes FF look less native in addition to
wasting people's time.

Reproducible: Always

Steps to Reproduce:
1. Open preferences
2. Move between panes

Actual Results:  
Along with the gradual resize on transition, panes fade in and out using
transparency.

Expected Results:  
Panes should not have transparency fading.

Comment 1

[Martin Creutziger [:MMx]]

I am using "Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2)
Gecko/20050313 Firefox/1.0+" right now and, additionally to the fade-in this bug
is about, the animation is _really_ slow. Is this addressed with this bug, too,
or should I file a seperate one?

Comment 2

[Josh Aas]

I was going to file a bug about the really slow transition - this bug is not for
that. If you could file one that would be great.

Comment 3

[Josh Aas]

Attachment: fix v1.0

Asking Mano for review before I ask for a peer review.

This patch does 3 things:
1) does fading quicky, after animation is completed
2) speeds up the animation
3) cleans up some code by using more precise function names and removing unused
variables

I do not have a Windows machine to test this on, and Mac isn't set up to
function correctly without animation, so I need a Windows tester before I
request peer review. Mano - can you test this on mac and windows and look over
the code?

Most notably, this will not solve bug 283713. A patch for that should probably
happen after this.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Patch works fine on Windows. The fade in effect, when enabled, is less
noticeable, and "wastes less time" like you said. I like it.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 177980 [details] [diff] [review]
fix v1.0

Yeah, this is much better (and even usable :) ).

@@ -641,16 +641,10 @@  (animated method)
	 <![CDATA[
	   this._multiplier = aNewPane.contentHeight > aOldPane.contentHeight ?
1 : -1;
	   var sizeDelta = Math.abs(aOldPane.contentHeight -
aNewPane.contentHeight);
-	   this._needsSizing = sizeDelta > this._animateIncrement;

	   this._animateRemainder = sizeDelta % this._animateIncrement;

-	   // Compute the opacity step, using 0.2 as a maximum step size. 
-	   this._opacityStepSize = sizeDelta != 0 ? this._animateIncrement /
sizeDelta : 1.0;
-	   if (this._opacityStepSize > 0.2)
-	     this._opacityStepSize = 0.2;
-
-	   this._setUpTimer(aOldPane.contentHeight);
+	   this._setUpAnimationTimer(aOldPane.contentHeight);
	 ]]>
	 </body>
       </method>

This is an edge case, but IMO the fade timer might be on while you're setting
up the animation timer. Something like this, in the animate method should be
fine:

if (this._animateTimer)
  this._animateTimer.cancel();
if (this._fadeTimer)
  this._fadeTimer.cancel();

(and then, remove the |(!this.*Timer)| checks from setUp*Timer methods)

>+          if (!this._animateTimer) 
>+            this._animateTimer = Components.classes["@mozilla.org/timer;1"]
>                                     .createInstance(Components.interfaces.nsITimer);

wrong indent, should be:

>+            this._animateTimer = Components.classes["@mozilla.org/timer;1"]
>                                            .createInstance(Components.interfaces.nsITimer);



same for those 
------
>+          this._animateTimer.initWithCallback(this, this._animateDelay, 
>+                                       Components.interfaces.nsITimer.TYPE_REPEATING_SLACK);

>+            this._fadeTimer = Components.classes["@mozilla.org/timer;1"]
>+                                    .createInstance(Components.interfaces.nsITimer);

>+          else
>+            this._fadeTimer.cancel();
>+          
>+          this._fadeTimer.initWithCallback(this, this._fadeDelay, 
>                                        Components.interfaces.nsITimer.TYPE_REPEATING_SLACK);
-------


I'll test the animated/non-animated state on windows tomorrow (note that these
changes shoudn't really affect non-animated mode, but ok).

otherwise, looks good.

Comment 6

[Josh Aas]

> This is an edge case, but IMO the fade timer might be on while you're setting
> up the animation timer.

This wouldn't matter. setUpFadeTimer() cancels the timer before starting it
again. You wouldn't be seeing the pane left in a half-opaque state, and next
time it was swapped in, the way the fade code works is to work up to 1.0 so it
wouldn't matter. I hope that makes sense.

Comment 7

[Asaf Romano (gone)]

(In reply to comment #6)
> > This is an edge case, but IMO the fade timer might be on while you're setting
> > up the animation timer.
> 
> This wouldn't matter. setUpFadeTimer() cancels the timer before starting it
> again. You wouldn't be seeing the pane left in a half-opaque state, and next
> time it was swapped in, the way the fade code works is to work up to 1.0 so it
> wouldn't matter. I hope that makes sense.

Maybe I wans't clear, the new code still allows fade-during-sizing, if you're
switching between panes extremely fast (i.e. click on a pane during a fade of
another one). It's pretty minor, and we may do want this behavior (in this
case), but it's worth a // comment ;)

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 177980 [details] [diff] [review]
fix v1.0


>         <body>
>         <![CDATA[
>-          if (!this._timer) 
>-            this._timer = Components.classes["@mozilla.org/timer;1"]
>+          if (!this._animateTimer) 
>+            this._animateTimer = Components.classes["@mozilla.org/timer;1"]
>                                     .createInstance(Components.interfaces.nsITimer);

style nit, bad alignment.

>           else
>-            this._timer.cancel();
>+            this._animateTimer.cancel();
>           this._currentHeight = aStartHeight;
>-          this._animateState = 0;
>           
>-          this._timer.initWithCallback(this, this._animateDelay, 
>+          this._animateTimer.initWithCallback(this, this._animateDelay, 
>+                                       Components.interfaces.nsITimer.TYPE_REPEATING_SLACK);

weird indentation here too.  This is almost just evil, but I'm sure there's a
better compromise that fits prevailing style better.

>+        ]]>
>+        </body>
>+      </method>
>+      
>+      <method name="_setUpFadeTimer">
>+        <body>
>+        <![CDATA[
>+          if (!this._fadeTimer) 
>+            this._fadeTimer = Components.classes["@mozilla.org/timer;1"]
>+                                    .createInstance(Components.interfaces.nsITimer);

and here

>+          else
>+            this._fadeTimer.cancel();
>+          
>+          this._fadeTimer.initWithCallback(this, this._fadeDelay, 
>                                        Components.interfaces.nsITimer.TYPE_REPEATING_SLACK);

and here.

Please try to be aware of the prevailing style and respect it wherever
possible.  I'll trust that you'll clean this up a bit for checkin, but
otherwise it looks good.

Comment 9

[Josh Aas]

fixed indentation, landed on trunk

Comment 10

[Asaf Romano (gone)]

Comment on attachment 177980 [details] [diff] [review]
fix v1.0

restoring flags