preferences transparency/fading violates aqua UI specs

VERIFIED FIXED in mozilla1.8beta2

Status

()

Toolkit
Preferences
--
minor
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

unspecified
mozilla1.8beta2
PowerPC
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
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.
(Assignee)

Updated

13 years ago
Assignee: bugs → joshmoz
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?
(Assignee)

Comment 2

13 years ago
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.
(Assignee)

Comment 3

13 years ago
Created attachment 177980 [details] [diff] [review]
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.
Attachment #177980 - Flags: review?(bugs.mano)
Patch works fine on Windows. The fade in effect, when enabled, is less
noticeable, and "wastes less time" like you said. I like it.
Version: unspecified → Trunk
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.
Attachment #177980 - Flags: review?(bugs.mano) → review+
(Assignee)

Comment 6

13 years ago
> 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.
(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 ;)
(Assignee)

Updated

13 years ago
Attachment #177980 - Flags: superreview?(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.
Attachment #177980 - Flags: superreview?(mconnor) → superreview+
(Assignee)

Comment 9

13 years ago
fixed indentation, landed on trunk
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox1.1
Status: RESOLVED → VERIFIED
Component: OS Integration → Preferences
Flags: superreview+
Flags: review+
Product: Firefox → Toolkit
Target Milestone: Firefox1.1 → mozilla1.8beta2
Version: Trunk → unspecified
Comment on attachment 177980 [details] [diff] [review]
fix v1.0

restoring flags
Attachment #177980 - Flags: second-review+
Attachment #177980 - Flags: first-review+

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.