427045 - Pull out correct toolbar colors in Vista themes

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Wesley Johnston (:wesj)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008040305 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008040305 Minefield/3.0pre

I'm pretty sure its possible to pull out the correct color for toolbars using Media::Rebar or Communications::Rebar styling. They're dark backgrounds, and need light text on them. This should fall back to something that shows up when the Vista Classic theme is being used. Not being able to seems to be creating headaches and new strange CSS properties like Bug 426660.

I've done some digging for this, and the text color can be pulled out from the Media::Toolbar and Communications::Toolbar themes. It falls back to black when the themes aren't available. I have a patch which I'm sure isn't strictly correct, but does this by implementing -moz-mediatext and -moz-communicationstext color properties. I'm not sure if that's whats wanted, or if there's a better way to do this or what, but I'll post it here for reference and if someone wants to direct me to a better way to do it, I can try to implement it. I'm new to this though, and it will probably take time.

I also noticed that nsLookAndFeel isn't ever closing the theme pointers that it loads. I'm not sure if thats a bug or by design.

Reproducible: Always

Comment 1

[Wesley Johnston (:wesj)]

Attachment: My first patch

I know that this isn't ready for primetime, but just wanted something as a proof of concept.

Comment 2

[Ryan A. C.]

Remember to set the Patch flag when attaching a patch, this allows Bugzilla to set the correct MIME type on the patch, and enables the diff ability/patch highlighting. This can be done retroactively in the Details link on the attachment.

Comment 3

[Wesley Johnston (:wesj)]

Thanks. I wondered why it was being served up funny.

Comment 4

[(no longer active)]

Nominating for blocking.  This could help Vista theme work for Firefox 3.

Comment 5

[Wesley Johnston (:wesj)]

Oh crap. Please don't review that. Its a bunch of copy and paste work to check if those color values really did exist. I really don't have time to fix it, but someone is welcome to take it as a base for a better fix.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Minusing based on comment #5 :)

Comment 7

[Dão Gottwald [:dao]]

Re-nominating for blocking. This is needed to use the toolbar styles from bug 419383 correctly -- they are new in 1.9 and that bug is blocking, too.

Comment 8

[Damon Sicore (:damons)]

Will take the patch, but still, don't think we'd hold the release this late in the game.  Again, feel free to argue.  Maybe convince beltzner.

Comment 9

[Dão Gottwald [:dao]]

wanted1.9.0.x doesn't help unless we want to change the theme in a point release.

Comment 10

[Dão Gottwald [:dao]]

Vlad, can you review the patch nonetheless? There's a small chance that Wesley finds time to finish it. Your feedback would probably help a lot.

Comment 11

[Wesley Johnston (:wesj)]

Yeah, Vlad if you don't mind, can you review it? I know I probably need to insert some code for other platforms to have a fallback. That should be easy. If I have some comments I can fix the other stuff too I think. Most of the code is just a copy paste from the menu theme code. I'm not sure if we'd want a special color property for hover/active text either.

Also, I wanted to note again that the theme handles here aren't being closed properly. Is that worth a different bug?

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Overall the patch approach is fine; that's essentially how you would add new -moz- colors that come from lookandfeel.  Calling getThemeColor in a loop could be bad in case of some breakage (e.g. if it kept returning E_HANDLE), so that should probably be handled in a different way; also, the fallthrough in those case blocks aren't correct.

Also, I would add the new css enums to the end of the enum, instead of in the middle, to avoid changing the numeric values of the previous ones.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Second try

OK, a second quick crack at this. Not even trying to fall through anymore because I'm a bit lazy, and not sure how to do it right. Added fallbacks for other platforms. I'm not sure what you meant by "loops" though Vlad. Should I check that hr in the "else if" and provide a different fall back if the theme just can't be found?

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Sorry, not loops, I dunno what I was thinking up there.  What I meant to say, and did so badly, was that you should probably structure things something like instead of...

>
>+    case eColor__moz_mediatext:
>+#ifndef WINCE
>+      if (isAppThemed && isAppThemed() && GetWindowsVersion() >= VISTA_VERSION) {
>+        COLORREF color;
>+        HRESULT hr;
>+        hr = getThemeColor(gMediaToolbarTheme, TP_BUTTON, TS_NORMAL, TMT_TEXTCOLOR, &color);
>+        if (hr == S_OK)
>+        {
>+          aColor = COLOREF_2_NSRGB(color);
>+          return NS_OK;
>+        }
>+        // Since we don't get theme changed messages, check if we lost the handle
>+        else if (hr == E_HANDLE)
>+        {
>+          closeTheme(gMediaToolbarTheme);
>+          gMediaToolbarTheme = openTheme(NULL, L"Media::Toolbar");
>+          getThemeColor(gMediaToolbarTheme, TP_BUTTON, TS_NORMAL, TMT_TEXTCOLOR, &color);
>+          aColor = COLOREF_2_NSRGB(color);
>+          return NS_OK;
>+        }
>+      }
>+      // if we've gotten here just return -moz-dialogtext instead
>+      idx = COLOR_WINDOWTEXT;
>+      break;
>+#endif

to something like this:

hr = getThemeColor(gMed...);
if (hr == E_HANDLE) {
    reopen();
    // try again
    hr = getThemeColor(...);
}

if (hr == S_OK) {
    aColor = COLORREF_2_NSRGB();
    return NS_OK;}
}

// if we got here..
idx = ...;
break;

Also, you probably want the #endif to be above the "if we got here" comment, and to use idx = COLOR_WINDOWTEXT on WINCE.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: v3

I knew this was gonna take too long. Fixed the WIN_CE problems, and rearranged the hresult handling.

Sorry to bug you about this Vlad, but CVS is giving me strange results when I diff now. Is this just some dumb thing where I need to update my tree with a special command? I tried asking on Mozilla's IRC for help, but couldn't find a channel that really fit this (probably because its a CVS and not a Mozilla problem).

Comment 16

[Wesley Johnston (:wesj)]

Oops. There's some unrelated cruft at the end of that patch, but the ! sections make me think its going to have to be redone anyway, so I'll wait till then.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 314855 [details] [diff] [review]
v3

>diff -u -8 -p -n -r3.105 nsCSSKeywordList.h

You probably meant -N (although it's not needed here) rather than -n.

Comment 18

[Ryan A. C.]

Note the comment at the top of the list on nsILookAndFeel.h
// When modifying this list, also modify nsXPLookAndFeel::sColorPrefs
// in widget/src/xpwidgts/nsXPLookAndFeel.cpp.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Version 4

OK. Added stuff in xpwidgets (I saw that comment yesterday and then it slipped my mind). I should also probably say that I can't get tests to compile here, so I haven't run them. I'm not sure if that's killer or not.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Should these be -moz-win-... instead of just -moz? I think so given the comment in nsILookAndFeel. If we do that I think we don't have to implement them in all platforms.

+        // Since we don't get theme changed messages, check if we lost the handle
+        if (hr == E_HANDLE)

This looks scary. Is this protocol for recovering from a theme change documented anywhere?

Share the code for getting the colors from the theme into a helper function that takes the address of the theme handle and the name of the theme.

Comment 21

[Wesley Johnston (:wesj)]

Thanks roc! Sorry to have to ask a lot of questions. The original code I was using was just stripped from the code for menuhover colors (which is handling the theme change the same way). Is there any reason I shouldn't update it to use these functions/patterns?

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Version 5 - Try try again

I have a strong feeling that this is (once again) not going to be "right", but I wanted to try again.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Just about there.

+      case eColor__moz_win_mediatext:
+      case eColor__moz_win_communicationstext:
       case eColor__moz_dialogtext:
         aColor = NS_RGB(0x66, 0x00, 0x66);

I don't think this should be here. We don't handle -moz-mac colors here. I don't think we need to handle -moz-win on other platforms at all.

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch V6

Removed offensive bit.

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 317551 [details] [diff] [review]
Patch V6

Looks ok I guess, though I'd almost rather just hardcode white for 1.9 and take this afterwards..

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 317551 [details] [diff] [review]
Patch V6

a1.9=beltzner

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #20)
> Should these be -moz-win-... instead of just -moz? I think so given the comment
> in nsILookAndFeel. If we do that I think we don't have to implement them in all
> platforms.

Should a new bug be filed for renaming media-toolbox et al to -moz-win-media-toolbox?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd like that.

Comment 29

[Dão Gottwald [:dao]]

I filed bug 431309.

Comment 30

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

mozilla/layout/style/nsCSSKeywordList.h 	3.107
mozilla/widget/public/nsILookAndFeel.h 	1.67
mozilla/widget/src/windows/nsLookAndFeel.cpp 	1.73
mozilla/widget/src/windows/nsLookAndFeel.h 	1.17
mozilla/widget/src/xpwidgets/nsXPLookAndFeel.cpp 	1.59 

Comment 31

[Kai Liu]

Attachment: testcase

Hmm, using the latest 20080501 nightly in Vista, this does not seem to be working with the attached test case.

Comment 32

[Kai Liu]

Wesley, I think you accidentally forgot to include nsCSSProps.cpp when diffing for the v6 patch.

Comment 33

[Wesley Johnston (:wesj)]

Ahh... sorry about that. I have a lot of meetings and stuff to do today, but I can get it in tomorrow morning maybe, or if someone else wants to fix it (or just rip that part out of the V5 patch) go ahead.

I hope this works for people besides me. I kinda guessed at how to do it, and it worked (for me at least). Has someone tested one of the working, rejected patches to make sure I'm not just crazy?

Comment 34

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

I landed the forgotten nsCSSProps.cpp change:

mozilla/layout/style/nsCSSProps.cpp 	3.168

Comment 35

[Kai Liu]

Verified using the testcase in comment 32 on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050206 Minefield/3.0pre