Last Comment Bug 431666 - Allow theme CSS to distinguish between themed and non-themed in Windows
: Allow theme CSS to distinguish between themed and non-themed in Windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Windows XP
-- enhancement (vote)
: ---
Assigned To: Rob Arnold [:robarnold]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 433017
  Show dependency treegraph
 
Reported: 2008-05-01 06:11 PDT by Kai Liu
Modified: 2008-08-19 05:38 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (474 bytes, text/html)
2008-05-01 06:11 PDT, Kai Liu
no flags Details
simple patch (3.24 KB, patch)
2008-05-01 06:13 PDT, Kai Liu
no flags Details | Diff | Splinter Review
v1.1 (2.73 KB, patch)
2008-07-25 16:36 PDT, Rob Arnold [:robarnold]
dbaron: review+
Details | Diff | Splinter Review
v1.2 (6.20 KB, patch)
2008-08-11 21:57 PDT, Rob Arnold [:robarnold]
no flags Details | Diff | Splinter Review
v1.2 (unbitrotted) (6.45 KB, patch)
2008-08-14 14:54 PDT, Rob Arnold [:robarnold]
vladimir: review+
Details | Diff | Splinter Review

Description User image Kai Liu 2008-05-01 06:11:05 PDT
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).
Comment 1 User image Kai Liu 2008-05-01 06:13:29 PDT
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.
Comment 2 User image David Baron :dbaron: ⌚️UTC-8 2008-07-07 17:31:50 PDT
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?
Comment 3 User image Kai Liu 2008-07-07 20:56:38 PDT
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.
Comment 4 User image Dão Gottwald [:dao] 2008-07-08 01:31:27 PDT
(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?
Comment 5 User image Kai Liu 2008-07-08 06:37:05 PDT
(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.
Comment 6 User image Dão Gottwald [:dao] 2008-07-08 06:59:50 PDT
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?
Comment 7 User image Alex Faaborg [:faaborg] (Firefox UX) 2008-07-16 17:31:44 PDT
>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.
Comment 8 User image Dão Gottwald [:dao] 2008-07-17 00:58:49 PDT
(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.
Comment 9 User image Kai Liu 2008-07-17 01:38:10 PDT
(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.
Comment 10 User image Alex Faaborg [:faaborg] (Firefox UX) 2008-07-22 11:52:41 PDT
>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).
Comment 11 User image Rob Arnold [:robarnold] 2008-07-25 16:36:53 PDT
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).
Comment 12 User image David Baron :dbaron: ⌚️UTC-8 2008-08-08 15:51:45 PDT
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...
Comment 13 User image Rob Arnold [:robarnold] 2008-08-11 21:57:37 PDT
Created attachment 333358 [details] [diff] [review]
v1.2
Comment 14 User image Rob Arnold [:robarnold] 2008-08-14 14:54:12 PDT
Created attachment 333827 [details] [diff] [review]
v1.2 (unbitrotted)
Comment 15 User image Vladimir Vukicevic [:vlad] [:vladv] 2008-08-14 15:10:56 PDT
Comment on attachment 333827 [details] [diff] [review]
v1.2 (unbitrotted)

Looks ok to me.
Comment 16 User image Rob Arnold [:robarnold] 2008-08-18 10:37:44 PDT
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/78aa15c87f52
Comment 17 User image Serge Gautherie (:sgautherie) 2008-08-18 16:32:58 PDT
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| ?
Comment 18 User image Rob Arnold [:robarnold] 2008-08-18 16:43:52 PDT
Yeah, probably. It's pretty minor though and there's no code that checks (or should check) for it explicitly.
Comment 19 User image neil@parkwaycc.co.uk 2008-08-19 05:36:05 PDT
Is there a bug on updating winstripe?
Comment 20 User image Dão Gottwald [:dao] 2008-08-19 05:38:06 PDT
Update which part of winstripe?

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