The default bug view has changed. See this FAQ.

Allow theme CSS to distinguish between themed and non-themed in Windows

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
--
enhancement
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Kai Liu, Assigned: robarnold)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 318796 [details]
testcase

(Please don't kill me for proposing this so late in the cycle!)

The :-moz-system-metric(windows-default-theme) metric introduced in bug 426660 allows us to distinguish between a "default theme" (any one of the four official Microsoft-signed styles for XP and Vista).  However, this metric has been mis-used as a proxy to distinguish between the classic Windows appearance and the new uxtheme-driven styled appearance.  While this produces the correct result in most cases, to use windows-default-theme as a proxy for IsAppStyled() is an incorrect approach and can produce undesirable results when third-party uxtheme styles are in use.

Because third-party uxtheme styles can vary wildly in their look and feel, there are many situations where we still need to use the windows-default-theme metric, such as when we have significant doubts that the results will look good if the user isn't used one of the signed Microsoft themes that we have tested and targeted (the styling of the "Larry" popup in Vista Aero is one such example).  At the same time, there are also cases where a different CSS rule is needed because of how native uxtheme styling differs from the traditional CSS styling (e.g., for some widgets, the native styling code draws the border within the content box, whereas the traditional CSS styling draws it outside of the content box, thus creating mismatches that have to be compensated for), and in cases like that, it does not make sense to use windows-default-theme as a proxy for IsAppThemed() because it's not a problem that magically goes away if the style in use is not one of the four Microsoft-signed ones.  But that is what we are doing right now, and it's incorrect and a bit too kludgy.

So I'm proposing that we add another metric, windows-is-themed to supplement the existing windows-default-theme metric (we really need both, I think).
(Reporter)

Comment 1

9 years ago
Created attachment 318797 [details] [diff] [review]
simple patch

Fortunately, the patch for this is very simple and low-risk.

I did a build overnight using this patch, and tested it this morning in 2K, XP, and Vista, and it works as expected.
(Reporter)

Updated

9 years ago
Attachment #318797 - Flags: superreview?(roc)
Attachment #318797 - Flags: review?(dbaron)
Attachment #318797 - Flags: superreview?(roc) → superreview?(dbaron)
(Reporter)

Updated

9 years ago
Component: Themes → Style System (CSS)
QA Contact: themes → style-system
(Reporter)

Updated

9 years ago
Blocks: 433017
(Reporter)

Updated

9 years ago
Blocks: 216266
(Reporter)

Updated

9 years ago
No longer blocks: 216266
Comment on attachment 318797 [details] [diff] [review]
simple patch

>+     * A Boolean value to determine whether the Windows is themed (Classic vs.
>+     * uxtheme)

Could you explain this more precisely?

>+     * This is Windows-specific and is not implemented on other platforms
>+     * (will return the default of NS_ERROR_FAILURE).

Seems odd to be inconsistent with the WindowsDefaultTheme metric, where we have code on other platforms to return NS_ERROR_NOT_IMPLEMENTED.


I'd like to see a little more evidence that this is needed before taking it.  Do other front-end developers agree that this is an important platform addition?
(Reporter)

Comment 3

9 years ago
Comment on attachment 318797 [details] [diff] [review]
simple patch

Ah, I had forgotten about this bug...

> Seems odd to be inconsistent with the WindowsDefaultTheme metric, where we have
> code on other platforms to return NS_ERROR_NOT_IMPLEMENTED.
> 
At the time, I was going for a touch-as-few-files-as-possible approach, so I omitted those.  The default case also returns an NS_ERROR, and the end result would have been the same (IIRC, there were other metrics that did something similar).  Which would you prefer, letting the default case handle it, or doing something more explicit?

> I'd like to see a little more evidence that this is needed before taking it. 
> Do other front-end developers agree that this is an important platform
> addition?
> 
There are some problems that can be solved by relying more on nsNativeThemeWin and less on CSS rules, but I think there is still a case for this, because I think that the biggest break in consistency on Windows happens at the themed-vs-nonthemed boundary.  I recently played around a bit with Windows Blinds and found that it changes the theme appearance without changing the theme file name that Windows reports, and of the population of users who use third-party themes, it seems like a significant a number of them use Windows Blinds (it's not free, but that's balanced by it not requiring the user to patch a system DLL and bypass the operating system's file protection scheme).  Which means that there really isn't a way to reliably detect whether or not we are using one of the "default" themes (and some of the Aero-specific color-hardcoding in the Library did not look very good with some of the WB themes).  So I'm thinking that it may be a good idea to just replace that metric with this one.
Attachment #318797 - Attachment is obsolete: true
Attachment #318797 - Flags: superreview?(dbaron)
Attachment #318797 - Flags: review?(dbaron)
(In reply to comment #3)
> I recently played around a bit with Windows
> Blinds and found that it changes the theme appearance without changing the
> theme file name that Windows reports [...] 
> Which means that there really isn't a way to reliably detect whether or not we
> are using one of the "default" themes (and some of the Aero-specific
> color-hardcoding in the Library did not look very good with some of the WB
> themes).  So I'm thinking that it may be a good idea to just replace that
> metric with this one.

So windows-default-theme has false positive results. I don't see how a metric that's even less restrictive improves this. Hardcoding colors for a wider range of themes would actually make things worse, right?
(Reporter)

Comment 5

9 years ago
(In reply to comment #4)
> So windows-default-theme has false positive results. I don't see how a metric
> that's even less restrictive improves this. Hardcoding colors for a wider range
> of themes would actually make things worse, right?
> 

What I mean is that because we are getting false positives, we should consider not coding stuff that's specific to a particular theme.
I still don't see what that example means for this bug. How exactly would Windows Blinds themes benefit from windows-is-themed instead of windows-default-theme? How would other third-party themes benefit? If we just don't hardcode stuff anymore, why do we need a metric?
>If we just don't hardcode stuff anymore, why do we need a metric?

Right, but the point is that UX team really wants to hard code stuff (in a way that degrades gracefully) so are not limited to only using the extremely limited available palette.

Can we inspect the set of system colors and from that infer what OS theme the user has.  We will still get some false positives, but this might work better than the current approach if windows blinds is registering as a default theme.
(In reply to comment #7)
> >If we just don't hardcode stuff anymore, why do we need a metric?
> 
> Right, but the point is that UX team really wants to hard code stuff (in a way
> that degrades gracefully) so are not limited to only using the extremely
> limited available palette.

I don't think that was Kai's point. It seems that windows-is-themed would be far less suited for hardcoded colors than windows-default-theme is.
(Reporter)

Comment 9

9 years ago
(In reply to comment #8)
> It seems that windows-is-themed would be
> far less suited for hardcoded colors than windows-default-theme is.
> 

Indeed.  It would not be suitable for hardcoded colors at all, nor was it ever intended for such a purpose.

The way I see it, it's better to have a metric that indicates "uxtheme or no uxtheme" under all circumstances than one that indicates "uxtheme or no uxtheme" under some circumstances and "Aero/Luna/etc." under others.  windows-is-themed would not be suitable for color hardcoding (and I never intended it to be), but I personally think that we should avoid hard coding colors if it is not possible to do it reliably.
>The way I see it, it's better to have a metric that indicates "uxtheme or no
>uxtheme" under all circumstances than one that indicates "uxtheme or no
>uxtheme" under some circumstances and "Aero/Luna/etc."

Perhaps I'm not parsing that correctly, but wouldn't the ideal metric be the ability to guarantee that the user has one of the three Luna themes, or Aero?  We want to use this to do things like style the library window on Vista, or perhaps in the future leverage the Luna color palette when using a Luna theme.  It would be even more ideal if we could figure out which of the three Luna themes the user has, since then we could actually modify some icons appropriately (like do a color match on the start button).
(Assignee)

Comment 11

9 years ago
Created attachment 331370 [details] [diff] [review]
v1.1

I took Kai's patch and unbitrotted it and reversed the semantics to indicate that the current 'theme' is Windows Classic (i.e. not uxtheme).
Assignee: nobody → tellrob
Status: NEW → ASSIGNED
Attachment #331370 - Flags: review?(vladimir)
Attachment #331370 - Flags: review?(dbaron)
Comment on attachment 331370 [details] [diff] [review]
v1.1

>From: Kai Liu <kliu@mozilla.kailiu.com>
>
>Allow theme CSS to distinguish between themed and non-themed in Windows
>
>diff --git a/widget/public/nsILookAndFeel.h b/widget/public/nsILookAndFeel.h
>--- a/widget/public/nsILookAndFeel.h
>+++ b/widget/public/nsILookAndFeel.h
>@@ -225,16 +225,25 @@ public:
>      * being used.
>      *
>      * The value of this metric is not used on other platforms. These platforms
>      * should return NS_ERROR_NOT_IMPLEMENTED when queried for this metric.
>      */
>     eMetric_WindowsDefaultTheme,
> 
>     /*
>+     * A Boolean value to determine whether the Windows is themed (Classic vs.
>+     * uxtheme)

"the Windows"?

Could you update the description to what this actually does?


Also, could you update all the other platforms nsILookAndFeel implementations the same way that they handle the WindowsDefaultTheme metric?

With that, r=dbaron, if this is actually needed...
Attachment #331370 - Flags: review?(dbaron) → review+
(Assignee)

Comment 13

9 years ago
Created attachment 333358 [details] [diff] [review]
v1.2
Attachment #331370 - Attachment is obsolete: true
Attachment #333358 - Flags: review?(vladimir)
Attachment #331370 - Flags: review?(vladimir)
(Assignee)

Comment 14

9 years ago
Created attachment 333827 [details] [diff] [review]
v1.2 (unbitrotted)
Attachment #333358 - Attachment is obsolete: true
Attachment #333827 - Flags: review?(vladimir)
Attachment #333358 - Flags: review?(vladimir)
Comment on attachment 333827 [details] [diff] [review]
v1.2 (unbitrotted)

Looks ok to me.
Attachment #333827 - Flags: review?(vladimir) → review+
(Assignee)

Comment 16

9 years ago
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/78aa15c87f52
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Comment on attachment 333827 [details] [diff] [review]
v1.2 (unbitrotted)

>diff --git a/widget/public/nsILookAndFeel.h b/widget/public/nsILookAndFeel.h
>+     * This is Windows-specific and is not implemented on other platforms
>+     * (will return the default of NS_ERROR_FAILURE).

Shouldn't this be updated to |NS_ERROR_NOT_IMPLEMENTED| ?
(Assignee)

Comment 18

9 years ago
Yeah, probably. It's pretty minor though and there's no code that checks (or should check) for it explicitly.
Is there a bug on updating winstripe?
Update which part of winstripe?
You need to log in before you can comment on or make changes to this bug.