Last Comment Bug 427045 - Pull out correct toolbar colors in Vista themes
: Pull out correct toolbar colors in Vista themes
Status: VERIFIED FIXED
: access, dev-doc-complete
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal with 2 votes (vote)
: mozilla1.9
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
Depends on:
Blocks: 403137 403147 405605 419383
  Show dependency treegraph
 
Reported: 2008-04-04 08:48 PDT by Wesley Johnston (:wesj)
Modified: 2008-10-05 19:56 PDT (History)
11 users (show)
dsicore: blocking1.9-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
My first patch (8.67 KB, patch)
2008-04-04 08:50 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Second try (13.91 KB, patch)
2008-04-09 21:48 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
v3 (18.91 KB, patch)
2008-04-10 08:22 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Version 4 (14.99 KB, patch)
2008-04-10 16:21 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Version 5 - Try try again (12.62 KB, patch)
2008-04-22 09:42 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch V6 (10.56 KB, patch)
2008-04-24 08:11 PDT, Wesley Johnston (:wesj)
vladimir: review+
roc: superreview+
mbeltzner: approval1.9+
Details | Diff | Splinter Review
testcase (458 bytes, text/html)
2008-05-01 09:33 PDT, Kai Liu
no flags Details

Description Wesley Johnston (:wesj) 2008-04-04 08:48:16 PDT
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) 2008-04-04 08:50:23 PDT
Created attachment 313620 [details] [diff] [review]
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. 2008-04-04 09:09:55 PDT
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) 2008-04-04 09:23:58 PDT
Thanks. I wondered why it was being served up funny.
Comment 4 :Ehsan Akhgari 2008-04-06 16:42:49 PDT
Nominating for blocking.  This could help Vista theme work for Firefox 3.
Comment 5 Wesley Johnston (:wesj) 2008-04-07 07:16:15 PDT
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] 2008-04-07 18:21:56 PDT
Minusing based on comment #5 :)
Comment 7 Dão Gottwald [:dao] 2008-04-08 04:45:49 PDT
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) 2008-04-08 14:33:42 PDT
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] 2008-04-08 14:40:18 PDT
wanted1.9.0.x doesn't help unless we want to change the theme in a point release.
Comment 10 Dão Gottwald [:dao] 2008-04-09 00:43:13 PDT
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) 2008-04-09 07:42:01 PDT
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] 2008-04-09 11:33:15 PDT
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) 2008-04-09 21:48:18 PDT
Created attachment 314772 [details] [diff] [review]
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] 2008-04-09 22:39:54 PDT
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) 2008-04-10 08:22:41 PDT
Created attachment 314855 [details] [diff] [review]
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) 2008-04-10 08:24:59 PDT
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] 2008-04-10 08:36:36 PDT
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. 2008-04-10 08:54:34 PDT
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) 2008-04-10 16:21:54 PDT
Created attachment 314985 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2008-04-22 02:14:59 PDT
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) 2008-04-22 06:03:38 PDT
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) 2008-04-22 09:42:30 PDT
Created attachment 317035 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2008-04-22 11:39:16 PDT
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) 2008-04-24 08:11:49 PDT
Created attachment 317551 [details] [diff] [review]
Patch V6

Removed offensive bit.
Comment 25 Vladimir Vukicevic [:vlad] [:vladv] 2008-04-28 10:31:00 PDT
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] 2008-04-28 10:38:15 PDT
Comment on attachment 317551 [details] [diff] [review]
Patch V6

a1.9=beltzner
Comment 27 Dão Gottwald [:dao] 2008-04-29 02:49:18 PDT
(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) (Exited; email my personal email if necessary) 2008-04-29 02:51:33 PDT
I'd like that.
Comment 29 Dão Gottwald [:dao] 2008-04-29 02:59:31 PDT
I filed bug 431309.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-04-29 12:27:46 PDT
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 2008-05-01 09:33:24 PDT
Created attachment 318835 [details]
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 2008-05-01 10:50:12 PDT
Wesley, I think you accidentally forgot to include nsCSSProps.cpp when diffing for the v6 patch.
Comment 33 Wesley Johnston (:wesj) 2008-05-01 11:00:22 PDT
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: gavin@gavinsharp.com] 2008-05-01 20:50:40 PDT
I landed the forgotten nsCSSProps.cpp change:

mozilla/layout/style/nsCSSProps.cpp 	3.168
Comment 35 Kai Liu 2008-05-02 09:37:19 PDT
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

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