Pull out correct toolbar colors in Vista themes

VERIFIED FIXED in mozilla1.9

Status

()

Core
Widget: Win32
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

({access, dev-doc-complete})

Trunk
mozilla1.9
x86
Windows Vista
access, dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

9 years ago
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
(Assignee)

Comment 1

9 years ago
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

9 years ago
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.
(Assignee)

Updated

9 years ago
Attachment #313620 - Attachment is patch: true
Attachment #313620 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 3

9 years ago
Thanks. I wondered why it was being served up funny.
Nominating for blocking.  This could help Vista theme work for Firefox 3.
Blocks: 405605
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
Component: OS Integration → Widget: Win32
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: os.integration → win32
Flags: blocking1.9?

Updated

9 years ago
Blocks: 403137
Attachment #313620 - Flags: review?(vladimir)
(Assignee)

Comment 5

9 years ago
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 :)
Flags: blocking1.9? → blocking1.9-

Updated

9 years ago
Blocks: 419383
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.
Flags: blocking1.9- → blocking1.9?
Keywords: access
Version: unspecified → Trunk
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.
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-
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.
(Assignee)

Comment 11

9 years ago
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.
(Assignee)

Comment 13

9 years ago
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?
Attachment #313620 - Attachment is obsolete: true
Attachment #314772 - Flags: review?
Attachment #313620 - Flags: review?(vladimir)
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.
(Assignee)

Comment 15

9 years ago
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).
Attachment #314772 - Attachment is obsolete: true
Attachment #314855 - Flags: review?
Attachment #314772 - Flags: review?
(Assignee)

Comment 16

9 years ago
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.

Comment 18

9 years ago
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.
(Assignee)

Comment 19

9 years ago
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.
Attachment #314855 - Attachment is obsolete: true
Attachment #314985 - Flags: review?
Attachment #314855 - Flags: review?

Updated

9 years ago
Attachment #314985 - Flags: review? → review?(vladimir)

Updated

9 years ago
Attachment #314985 - Flags: superreview?(roc)
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.
(Assignee)

Comment 21

9 years ago
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?
(Assignee)

Comment 22

9 years ago
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.
Attachment #314985 - Attachment is obsolete: true
Attachment #317035 - Flags: superreview?
Attachment #317035 - Flags: review?
Attachment #314985 - Flags: superreview?(roc)
Attachment #314985 - Flags: review?(vladimir)
(Assignee)

Updated

9 years ago
Attachment #317035 - Flags: superreview?(roc)
Attachment #317035 - Flags: superreview?
Attachment #317035 - Flags: review?(vladimir)
Attachment #317035 - Flags: review?
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.
(Assignee)

Comment 24

9 years ago
Created attachment 317551 [details] [diff] [review]
Patch V6

Removed offensive bit.
Attachment #317035 - Attachment is obsolete: true
Attachment #317551 - Flags: superreview?
Attachment #317551 - Flags: review?
Attachment #317035 - Flags: superreview?(roc)
Attachment #317035 - Flags: review?(vladimir)
(Assignee)

Updated

9 years ago
Attachment #317551 - Flags: superreview?(roc)
Attachment #317551 - Flags: superreview?
Attachment #317551 - Flags: review?(vladimir)
Attachment #317551 - Flags: review?
Attachment #317551 - Flags: superreview?(roc) → superreview+
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..
Attachment #317551 - Flags: review?(vladimir) → review+

Updated

9 years ago
Attachment #317551 - Flags: approval1.9?
Comment on attachment 317551 [details] [diff] [review]
Patch V6

a1.9=beltzner
Attachment #317551 - Flags: approval1.9? → approval1.9+

Updated

9 years ago
Keywords: checkin-needed

Updated

9 years ago
Blocks: 403147
Assignee: nobody → wesley-johnston
(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 
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9

Comment 31

9 years ago
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

9 years ago
Wesley, I think you accidentally forgot to include nsCSSProps.cpp when diffing for the v6 patch.
(Assignee)

Comment 33

9 years ago
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?

Updated

9 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I landed the forgotten nsCSSProps.cpp change:

mozilla/layout/style/nsCSSProps.cpp 	3.168
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 35

9 years ago
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
Status: RESOLVED → VERIFIED

Updated

9 years ago
Keywords: dev-doc-needed

Updated

9 years ago
Keywords: dev-doc-needed → dev-doc-complete
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.