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

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

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.

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

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

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.

Minusing based on comment #5 :)

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.

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.

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

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.

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?

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.

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?

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.

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

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 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.

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.

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.

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.

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?

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

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.

Removed offensive bit.

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 on attachment 317551 [details] [diff] [review] Patch V6 a1.9=beltzner

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

I'd like that.

I filed bug 431309.

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

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

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

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?

I landed the forgotten nsCSSProps.cpp change: mozilla/layout/style/nsCSSProps.cpp 3.168

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