Last Comment Bug 665368 - Indeterminate progress bars look wrong in Windows Classic
: Indeterminate progress bars look wrong in Windows Classic
Status: VERIFIED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: All Windows 7
: -- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-19 11:10 PDT by Kai Liu
Modified: 2011-08-23 01:54 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
affected
fixed
fixed


Attachments
the two classic styles (1.58 KB, image/png)
2011-06-22 03:37 PDT, Kai Liu
no flags Details
Indeterminate style for Windows Classic Theme (5.59 KB, patch)
2011-06-22 07:52 PDT, Mounir Lamouri (:mounir)
jmathies: review+
kliu: feedback+
jst: approval‑mozilla‑aurora+
Details | Diff | Review
two animation frames from XP themed (684 bytes, image/png)
2011-06-22 08:59 PDT, Kai Liu
no flags Details
indeterminate progress bar (8.36 KB, image/jpeg)
2011-08-23 00:45 PDT, Mihaela Velimiroviciu (:mihaelav)
no flags Details

Description Kai Liu 2011-06-19 11:10:50 PDT
They look like a static 100%-full progress bar, with no animation or other forms of indication to show that it is in an indeterminate state.

There are two options here:

1) Implement an animated indeterminate progress chunk in ClassicDrawWidgetBackground.

2) Modify ClassicThemeSupportsWidget so that native rendering for <progress>  is disabled and we fall back to a CSS-only rendering.  This requires that bug 665367 be fixed.  There are two minor pitfalls: the color of the bar that is hard-coded into forms.css does not match the native color used by ClassicDrawWidgetBackground and, depending on the animation used in forms.css, it may or may not match the native animation used by Windows itself.
Comment 1 Mounir Lamouri (:mounir) 2011-06-22 02:19:13 PDT
(In reply to comment #0)
> They look like a static 100%-full progress bar, with no animation or other
> forms of indication to show that it is in an indeterminate state.
> 
> There are two options here:
> 
> 1) Implement an animated indeterminate progress chunk in
> ClassicDrawWidgetBackground.

Let's do that ;)
Comment 2 Kai Liu 2011-06-22 03:37:53 PDT
Created attachment 541008 [details]
the two classic styles

There are two styles of "classic" progress bars: chunked and smooth.

In both cases, the width is static: it remains the same regardless of the width of the progress bar.  For chunked, it's 5 chunks, and for smooth, it's 5 pixels.

The speed at which indeterminate progress bars move in Windows is adjustable.  By default, it is one unit per 30ms, but native programs can change this in the PBM_SETMARQUEE message.  The size of the unit, however, depends on the style.  For the chunked style, it moves one chunk per interval, and for the smooth style, it moves one pixel per interval.  So the chunked style moves considerably faster, in terms of pixels, than the smooth style.

I personally favor the smooth style: it's easier to draw and the long length of the chunked style makes it less suitable for short progress bars.
Comment 3 Mounir Lamouri (:mounir) 2011-06-22 03:41:31 PDT
Are you using Windows XP? I got something else on Windows 7.
Comment 4 Kai Liu 2011-06-22 03:55:10 PDT
Yes, I was using XP.  I just ran my test program and Win7, and you're right: it's different.  It always uses the smooth style regardless of whether the PBS_SMOOTH flag is set, the width is a static 39px regardless of the width of the bar, and each increment, it moves by 8px.
Comment 5 Kai Liu 2011-06-22 03:55:49 PDT
s/and Win7/on Win7/
Comment 6 Mounir Lamouri (:mounir) 2011-06-22 05:29:11 PDT
Kai, on Windows 7, when indeterminate, it goes from left to right and when part of the bar is hidden at the right, it's shown on the left, like this:
[ . . . . . .=====.]
[== . . . . . . ===]
[ ===== . . . . . .]

Can you confirm it's the same on Windows XP?
Comment 7 Kai Liu 2011-06-22 07:32:39 PDT
Yes, it is.(In reply to comment #6)
> Can you confirm it's the same on Windows XP?

Yes, it is.
Comment 8 Mounir Lamouri (:mounir) 2011-06-22 07:52:31 PDT
Created attachment 541049 [details] [diff] [review]
Indeterminate style for Windows Classic Theme

Kai, could you tell me if that looks correct on Windows XP?
On Windows 7, IMO, it's very close enough to the indeterminate progress bars I've seen.
Comment 9 Kai Liu 2011-06-22 08:42:11 PDT
Comment on attachment 541049 [details] [diff] [review]
Indeterminate style for Windows Classic Theme

Yes, it looks great in XP.  The bar used by native apps is shorter in XP, but I like using the same bar length for all versions of Windows, since the 5px bar that XP uses is probably too short.

>+// Speed (px per ms) of the animation for indeterminate progress bars (Windows Classic).
>+static const double kProgressClassicIndeterminateSpeed = 0.0875;

For all XP styles (themed, chunked classic, and smooth classic) and Windows 7 classic, with each animation frame, it moves the bar by 1/5 the length of the bar.  I think the reason that Microsoft used 1/5 of the length per animation frame is so that the chunks and the spaces between the chunks in XP's themed progress bar and XP's chunked classic progress bar lined up at each frame of the animation with the chunks in the previous frame--so that the animation looks a bit smoother.  In Windows 7 themed, the movement per animation frame does not follow this 1/5 length convention.

So there really isn't a "correct" animation speed.  The px moved per animation frame varied with the style.  And the framerate itself can set by applications when they turn on this indeterminate/marquee style (defaulting to 1 frame / 30 ms if none is specified).  So maybe it would be better if we just used the same speed for all the different animation styles instead of having a different speed for classic?

(And if QueueAnimatedContentForRefresh gives us a framerate that is sufficiently stable and consistent, then we maybe could consider making the themed XP style move 1 chunk per frame so that the chunks line up in each frame.)
Comment 10 Kai Liu 2011-06-22 08:59:30 PDT
Created attachment 541061 [details]
two animation frames from XP themed

This is just an example to illustrate my previous post.  These are two screenshots, from two consecutive animation frames (my test program used a very slow framerate :)) in themed XP (classic with the chunked style looks similar).  This is just to show that Microsoft was thinking about alignment, not about the actual speed when they determined the movement speed of their indeterminate progress bars.
Comment 11 Jim Mathies [:jimm] 2011-07-13 08:46:58 PDT
Comment on attachment 541049 [details] [diff] [review]
Indeterminate style for Windows Classic Theme

Review of attachment 541049 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/windows/nsNativeThemeWin.cpp
@@ +3383,5 @@
> +      /**
> +       * The indeterminate rendering is animated: the bar goes from one side to
> +       * another.
> +       */
> +      if (!QueueAnimatedContentForRefresh(aFrame->GetContent(), 30)) {

Should we set a standard here for frame rates? :volkmar used 60 in a previous patch, and in retrospect maybe that didn't need to be so high. I'm not even sure we need 30 in this case.

@@ +3400,5 @@
> +                          &tempValue);
> +      PRInt32 dx = 0;
> +
> +      // If the frame direction is RTL, we want to have the animation going RTL.
> +      // ratio is in [0.0; 1.0[ range, inverting it reverse the animation.

Minor comment nit '['
Comment 12 Mounir Lamouri (:mounir) 2011-07-21 10:49:41 PDT
(In reply to comment #11)
> Comment on attachment 541049 [details] [diff] [review] [review]
> Indeterminate style for Windows Classic Theme
> 
> Review of attachment 541049 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/src/windows/nsNativeThemeWin.cpp
> @@ +3383,5 @@
> > +      /**
> > +       * The indeterminate rendering is animated: the bar goes from one side to
> > +       * another.
> > +       */
> > +      if (!QueueAnimatedContentForRefresh(aFrame->GetContent(), 30)) {
> 
> Should we set a standard here for frame rates? :volkmar used 60 in a
> previous patch, and in retrospect maybe that didn't need to be so high. I'm
> not even sure we need 30 in this case.

I think I used 30 in a previous patch, not 60. I will double-check though. I used 30 for all progress element animation stuff in all platforms.

> @@ +3400,5 @@
> > +                          &tempValue);
> > +      PRInt32 dx = 0;
> > +
> > +      // If the frame direction is RTL, we want to have the animation going RTL.
> > +      // ratio is in [0.0; 1.0[ range, inverting it reverse the animation.
> 
> Minor comment nit '['

I don't understand why '[' is a neat. I put '[' because it's a mathematical sign to say that 1.0 is excluded.
Comment 13 Mounir Lamouri (:mounir) 2011-07-21 10:51:54 PDT
You were actually right, I used 60. I will change this 60 to 30 for consistency.
Comment 14 Marco Bonardo [::mak] 2011-07-25 05:57:24 PDT
http://hg.mozilla.org/mozilla-central/rev/0ef7a3e8d959
Comment 15 Mounir Lamouri (:mounir) 2011-07-25 09:56:48 PDT
Comment on attachment 541049 [details] [diff] [review]
Indeterminate style for Windows Classic Theme

Firefox 6 is going to introduce the progress element. It would be nice to reduce the time between the introduction of the progress element and have it working as excepted with Windows Classic Theme. If we move this patch to Aurora (Firefox 7) the difference will be 6 weeks. Otherwise, it is going to be 3 months. Given that this patch only touch Windows Classic code, I guess it is worth it.
Comment 16 Mounir Lamouri (:mounir) 2011-08-08 08:33:59 PDT
Pushed to aurora aka Firefox 7:
http://hg.mozilla.org/releases/mozilla-aurora/rev/32d78225a916
Comment 17 Mihaela Velimiroviciu (:mihaelav) 2011-08-23 00:45:07 PDT
Created attachment 555046 [details]
indeterminate progress bar

This is how the indeterminate progress bar looks on Win7 with Windows Classic Theme (in Downloads window); it goes as described in comment 6. Is this what the fix should do? If not, please provide some guidance to verify the fix.
Thanks!
Comment 18 Mounir Lamouri (:mounir) 2011-08-23 00:49:05 PDT
Yes. You could have compare the progress bar to the ones you see in the system too.
Comment 19 Mihaela Velimiroviciu (:mihaelav) 2011-08-23 01:54:09 PDT
Considering comment 18 setting resolution to VERIFIED-FIXED

Note You need to log in before you can comment on or make changes to this bug.