642846 - Improve the indeterminate progress bar rendering on Windows Vista and 7

Status: RESOLVED WONTFIX

(Core :: Widget: Win32, enhancement, P5)

Description

[Mounir Lamouri (:mounir)]

With bug 641905, the progress bar rendering will be a moving overlay like the native rendering but it's using PP_MOVEOVERLAY which is a very light green overlay. The native one is much more stronger. Though, there are no parts to draw with so if we want to have a similar rendering, we will have to simulate it.

See Windows documentation (with screenshots):
http://msdn.microsoft.com/en-us/library/bb760816(v=vs.85).aspx
http://msdn.microsoft.com/en-us/library/bb760842(v=vs.85).aspx
http://msdn.microsoft.com/en-us/library/bb760820(v=vs.85).aspx#PBS_MARQUEE

Comment 1

[Kai Liu]

The strange thing about this is that, if you peek inside the resource file where Microsoft stashes all of the theme images (aero.msstyles), there *is* an image for the marquee (id:670).  The resource file also has a table that maps these image IDs to part and state IDs, and that image is mapped to the default state of part 8 (i.e., the same as the PP_MOVEOVERLAY image).  So there are two images mapped to the same part and state in the resource file; I wonder how uxtheme is deciding that it should load the PP_MOVEOVERLAY image that we currently see instead of the marquee image that we want.  Or maybe it never loads the marquee image, and it's just there as an artifact.

It would be interesting to patch up uxtheme.dll to allow unsigned window styles, download and install a third-party window style, and modify the image associated with the marquee in that window style and see if it results in native apps rendering the marquee differently.  If it does, then Windows is still drawing from images in the resource file, and there must be some (apparently undocumented) programmatic way to retrieve and draw that part.  If it doesn't, then the presence of that image is an artifact and native apps are simulating it by applying some sort of filter to PP_FILL.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Kai Liu from comment #1)
> The strange thing about this is that, if you peek inside the resource file
> where Microsoft stashes all of the theme images (aero.msstyles), there *is*
> an image for the marquee (id:670).

aero.msstyles is just a dll with a different file extension, I'm thinking we should just load this dll up, extract the png image at the right index and paint it when needed. We could delay load the image on the first progress render and cache it in nsNativeThemeWin for the session.

Comment 4

[Jim Mathies [:jimm]]

Attachment: wip

Works, although I'm losing alpha in some cases, which I think is due to the underlying surface not supporting it depending on what backend we're using. I should be able to fix this by using the gfx context / cairo to do the blending.

Comment 5

[Jim Mathies [:jimm]]

Attachment: wip

minor cleanup.

Comment 6

[Jim Mathies [:jimm]]

Attachment: progress comparison

Hmm, even with the alpha fixed up, these don't look right. In fact I think the aero png is a different image than what you see in a net app. I'm wondering if this is worth all the trouble.

Comment 7

[Jim Mathies [:jimm]]

Attachment: aero styles png wip patch

Comment 8

[Jim Mathies [:jimm]]

Attachment: progress comparison

Stephen, seeking some feedback from a ux on this. I've been working on fixing progress bars on windows, which were pretty broken. Our current indeterminate progress (vista+) renders using a pulse graphic the theme library provides that isn't meant for the way in which we are using it. (it's actually the overlay pulse on determined progress bars.)

I found an indeterminate png resource in Windows aero styles library and loaded that manually to use in this case hoping it was the same graphic you see in common controls progress meters. Unfortunately it's not, it's much darker.

Attached are some screen shots of the three different progress meter styles. The outer set is the manually painted aero png, the second inner set is what we have now with the pulse graphic, and the inner progress is what we really want, but can't get.

I'm wondering if - 

1) we should just stick with what we have in the pulse graphic
2) use the aero styles png (I vote no, it's too dark)
3) try to create a new progress image manually that we can paint that matches the common controls progress.

For option 3 I would need some help from somebody who knows their way around photoshop! :)

Comment 9

[Jim Mathies [:jimm]]

Attachment: aero styles png

Comment 10

[Stephen Horlander [:shorlander]]

Option 3 seems doable. Would we need the entire progress bar recreated or just the green overlay part?

Comment 11

[Jim Mathies [:jimm]]

(In reply to Stephen Horlander from comment #10)
> Option 3 seems doable. Would we need the entire progress bar recreated or
> just the green overlay part?

Just the hazy green part, the track is drawn separately. I've attached the png I found in the aero styles library for reference.

Comment 12

[Jim Mathies [:jimm]]

Actually in looking at the images, there's a white shine that integrates into the green haze, and that has different heights depending on the orientation. So we might need the two images with the haze in slightly different positions. (I can screen cap whatever we need to look at in order to get the right metrics.)

Comment 13

[Jim Mathies [:jimm]]

Attachment: tracks

Both meter tracks.

Comment 14

[Jim Mathies [:jimm]]

Attachment: common ctrls version

For reference, here's the default graphic blown up a bit for horizontal meters.

Comment 15

[Jim Mathies [:jimm]]

Attachment: fix v.1

Here's the patch that will do this. I've used the aero styles png as a temporary filler.

Comment 16

[Jim Mathies [:jimm]]

Attachment: fix v.1

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 600804 [details] [diff] [review]
fix v.1

Hmm. Roc's on PTO. Joe, curious if you might be able to review this?

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 600804 [details] [diff] [review]
fix v.1

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

I have reviewed the parts I can; I rely upon Brian to review the bits I'm not qualified to.

::: widget/windows/nsNativeThemeWin.cpp
@@ +913,5 @@
> +
> +  mDidTryIndeterminateLoad = true;
> +
> +  // Currently only tested on Vista/Win7/Win8
> +  if (WinUtils::GetWindowsVersion() == WinUtils::WINXP_VERSION) {

<=?

@@ +923,5 @@
> +  if (nsUXThemeData::GetNativeThemeId() != WINTHEME_AERO) {
> +    return false;
> +  }
> +
> +#if 0

Do you need to keep this code?

@@ +978,5 @@
> +  }
> +
> +  container->CopyFrame(imgIContainer::FRAME_CURRENT,
> +                       imgIContainer::FLAG_SYNC_DECODE,
> +                       getter_AddRefs(mIndeterminateImage));

If you don't need access to the bits, use GetFrame().

Both of these return an rv that you can test.

@@ +1044,5 @@
> +  paintCtx->SetOperator(gfxContext::OPERATOR_OVER);
> +  paintCtx->ResetClip();
> +  paintCtx->Translate(gfxPoint(aOverlayRect->left, aOverlayRect->top));
> +  if (aVertical) {
> +    paintCtx->Rotate((M_PI/180)*-90);

Just use radians here. :) (-M_PI/2 if my reading is correct)

Comment 19

[Jim Mathies [:jimm]]

(In reply to Joe Drew (:JOEDREW!) from comment #18)
> Comment on attachment 600804 [details] [diff] [review]
> fix v.1
> > +  // Currently only tested on Vista/Win7/Win8
> > +  if (WinUtils::GetWindowsVersion() == WinUtils::WINXP_VERSION) {
> 
> <=?

We dropped support for 2K, WINXP_VERSION is the lowest version we support. 

> 
> @@ +923,5 @@
> > +  if (nsUXThemeData::GetNativeThemeId() != WINTHEME_AERO) {
> > +    return false;
> > +  }
> > +
> > +#if 0
> 
> Do you need to keep this code?

No not anymore.. Was keeping it around until I was sure we wanted to create our own png.


> @@ +978,5 @@
> > +  }
> > +
> > +  container->CopyFrame(imgIContainer::FRAME_CURRENT,
> > +                       imgIContainer::FLAG_SYNC_DECODE,
> > +                       getter_AddRefs(mIndeterminateImage));
> 
> If you don't need access to the bits, use GetFrame().
> 
> Both of these return an rv that you can test.

Will update.

> 
> @@ +1044,5 @@
> > +  paintCtx->SetOperator(gfxContext::OPERATOR_OVER);
> > +  paintCtx->ResetClip();
> > +  paintCtx->Translate(gfxPoint(aOverlayRect->left, aOverlayRect->top));
> > +  if (aVertical) {
> > +    paintCtx->Rotate((M_PI/180)*-90);
> 
> Just use radians here. :) (-M_PI/2 if my reading is correct)

Ok. Thanks!

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 600804 [details] [diff] [review]
fix v.1

canceling reviews for now, I have to touch this up a bit. The underlying image is in three parts, a cap, bottom, and a stretchy middle section.

Comment 21

[Jim Mathies [:jimm]]

Attachment: fix v.2

Comment 22

[Stephen Horlander [:shorlander]]

Comment on attachment 600160 [details]
progress comparison

Sent Jim the images for option 3

Comment 23

[Jim Mathies [:jimm]]

Attachment: fix v.3

Fixing a leak in that last patch. In the first rev I used an nsAutoTArray for the three images, but since nsNativeThemeWin is a global, it leaked the base array. So I just went with something simple that I can clear on the xpcom shutdown event.

Comment 24

[Brian R. Bondy [:bbondy]]

Comment on attachment 602125 [details] [diff] [review]
fix v.3

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

::: widget/windows/nsNativeThemeWin.cpp
@@ +923,5 @@
> +
> +  mDidTryIndeterminateLoad = true;
> +
> +  // Currently only tested on Vista/Win7/Win8
> +  if (WinUtils::GetWindowsVersion() == WinUtils::WINXP_VERSION) {

I prefer < Vista here because win2003 should act like XP when the theme service is enabled.

@@ +944,5 @@
> +  nsCOMPtr<imgITools> imgTools =
> +    do_CreateInstance("@mozilla.org/image/tools;1");
> +  NS_ENSURE_TRUE(imgTools, false);
> +
> +  for (int idx = 0; idx < 3; idx++) {

nit: replace 3 with mozilla::ArrayLength(bitmaps)

@@ +978,5 @@
> +    DeleteObject(pngImage);
> +    NS_ENSURE_SUCCESS(rv, false);
> +    nsRefPtr<gfxImageSurface> image = surf->GetAsImageSurface();
> +    NS_ENSURE_TRUE(image, false);
> +    mIndeterminateImage[idx] = image;

I don't understand why this won't leak by making mIndeterminateImage an nsRefPtr.  How does it know to free the 2nd and third?  I seen "this is a global object, so using an nsAutoTArray here will leak." but won't that free the first object?

Comment 25

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #24)
> Comment on attachment 602125 [details] [diff] [review]
> fix v.3
> 
> Review of attachment 602125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsNativeThemeWin.cpp
> @@ +923,5 @@
> > +
> > +  mDidTryIndeterminateLoad = true;
> > +
> > +  // Currently only tested on Vista/Win7/Win8
> > +  if (WinUtils::GetWindowsVersion() == WinUtils::WINXP_VERSION) {
> 
> I prefer < Vista here because win2003 should act like XP when the theme
> service is enabled.
> 
> @@ +944,5 @@
> > +  nsCOMPtr<imgITools> imgTools =
> > +    do_CreateInstance("@mozilla.org/image/tools;1");
> > +  NS_ENSURE_TRUE(imgTools, false);
> > +
> > +  for (int idx = 0; idx < 3; idx++) {
> 
> nit: replace 3 with mozilla::ArrayLength(bitmaps)

will do.
 
> @@ +978,5 @@
> > +    DeleteObject(pngImage);
> > +    NS_ENSURE_SUCCESS(rv, false);
> > +    nsRefPtr<gfxImageSurface> image = surf->GetAsImageSurface();
> > +    NS_ENSURE_TRUE(image, false);
> > +    mIndeterminateImage[idx] = image;
> 
> I don't understand why this won't leak by making mIndeterminateImage an
> nsRefPtr.  How does it know to free the 2nd and third?  I seen "this is a
> global object, so using an nsAutoTArray here will leak." but won't that free
> the first object?

The use of RefPtr keeps the images around. Freeing the resources happens in the xpcom shutdown event handler - 
 
+  if (strcmp(aTopic, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID) == 0) {
+    // avoids leak reporting
+    mIndeterminateImage[BITMAP_TOP] = mIndeterminateImage[BITMAP_MID] =
+      mIndeterminateImage[BITMAP_BOTTOM] = nsnull;
+  }

without this, all three (or a smaller subset) will leak on shutdown.

Comment 26

[Brian R. Bondy [:bbondy]]

Comment on attachment 602125 [details] [diff] [review]
fix v.3

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

oops ya didn't read it right before.  r+ with nits.

Comment 27

[Wayne Mery (:wsmwk)]

(In reply to Brian R. Bondy [:bbondy] from comment #26)
> Comment on attachment 602125 [details] [diff] [review]
> fix v.3
> 
> Review of attachment 602125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> oops ya didn't read it right before.  r+ with nits.

Jim, did this land?

Also, why do you have meta bug 726144 blocking this (and others)?  Don't bugs typically block the meta?

Comment 28

[Jim Mathies [:jimm]]

(In reply to Wayne Mery (:wsmwk) from comment #27)
> (In reply to Brian R. Bondy [:bbondy] from comment #26)
> > Comment on attachment 602125 [details] [diff] [review]
> > fix v.3
> > 
> > Review of attachment 602125 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > oops ya didn't read it right before.  r+ with nits.
> 
> Jim, did this land?

Not yet, all the patches are over in bug 658829. There are a couple of reviews to finish up and there's a bug with indeterminates with current mc.

> Also, why do you have meta bug 726144 blocking this (and others)?  Don't
> bugs typically block the meta?

I suppose those could be reversed.

Comment 29

[lmlb_lmlb]

From what I can tell, the reason why the aero styles png is too dark is because it's using premultiplied alpha.

Comment 30

[Jim Mathies [:jimm]]

Attachment: updated patch

Comment 31

[ebrahim]

Attachment: Windows 8

This is what I can see on Windows 8 and latest firefox Nightly. Is this really desired?

Comment 32

[Mounir Lamouri (:mounir)]

Ebrahim, could you open a bug for this? It might very likely be for Windows 8 only.

Comment 33

[Jim Mathies [:jimm]]

(In reply to Ebrahim Byagowi from comment #31)
> Created attachment 736474 [details]
> Windows 8
> 
> This is what I can see on Windows 8 and latest firefox Nightly. Is this
> really desired?

Yes, currently we're using a filled progress meter with the pulse. This bug is all about making it look more native.

Comment 34

[ebrahim]

OK. Bug 962636 filed.

Comment 36

[Stephen A Pohl [:spohl]]

We have switched to non-native theming of form controls in bug 1535761 and dependent bugs.