665368 - Indeterminate progress bars look wrong in Windows Classic

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Kai Liu]

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

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

Attachment: 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)]

Are you using Windows XP? I got something else on Windows 7.

Comment 4

[Kai Liu]

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]

s/and Win7/on Win7/

Comment 6

[Mounir Lamouri (:mounir)]

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]

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

Attachment: 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]

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]

Attachment: 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]]

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

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

You were actually right, I used 60. I will change this 60 to 30 for consistency.

Comment 14

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

http://hg.mozilla.org/mozilla-central/rev/0ef7a3e8d959

Comment 15

[Mounir Lamouri (:mounir)]

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

Pushed to aurora aka Firefox 7:
http://hg.mozilla.org/releases/mozilla-aurora/rev/32d78225a916

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: 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)]

Yes. You could have compare the progress bar to the ones you see in the system too.

Comment 19

[Mihaela Velimiroviciu (:mihaelav)]

Considering comment 18 setting resolution to VERIFIED-FIXED