462441 - Windows 7 default OS theme not recognized by windows-default-theme metric

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Gábor Stefanik]

Attachment: Screenshot of the bug

When running Firefox 3.1 beta 1 on the recently handed out Windows 7 PDC test build, the Site Identity button looks oddly squared off. I didn't test whether this happens on 3.1b1+Vista or 3.0.3+Windows 7 as well.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

--> Firefox::Theme

I am gonna go out on a limb and guess that our theme detection stuff might be broken here. That looks like the Windows Classic form of the button on a Windows Aero frame.

Blocking for investigation, we might take this off the radar if it turns out to be a Windows 7 bug.

Comment 2

[Gábor Stefanik]

Bug is also present in the latest 3.0.5pre nightly, even when using a non-stock theme like XP on Vista, Vista on XP, Energy, Strata Reloaded, etc. The NASA Night Launch theme looks completely broken, with background images and textbox borders missing.

Also, with the default theme, the border of the Location bar is wrong - it looks like a native Windows Vista/Windows 7 textbox, and not like Firefox 3's proper Location bar. Same goes for the Search bar.

Comment 3

[Gábor Stefanik]

The "JetFox Aqua" theme does render correctly, though.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

Perhaps this is related to our only considering the cases of Aero and Classic?  Ideally we would have very rich OS theme detection, although in this case there is no way for us predict themes from the future, so some polish work should be expected for each new version of Windows.

Comment 5

[Gábor Stefanik]

I think the problem is that before checking whether the theme is Aero, Firefox checks if the OS is Vista (Windows NT 6.0). It should check for "Vista or later", or perhaps "Windows NT 6.x" (Windows 7 is Windows NT 6.1, not 7.0 as the name would suggest).

By the way, Bugzilla erroneously detects Windows 7 as Windows NT. Windows Vista would be a much more correct selection. (The UA of Firefox 3.1b1 on 7 is "Mozilla/5.0 (Windows; U; Windows NT 6.1; hu; rv:1.9.1b1) Gecko/20081007 Firefox/3.1b1".)

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

>It should check for "Vista or later"

We can't really predict what design decisions Microsoft will make in later releases of windows, so there isn't much use in trying to future proof our detection.

Comment 7

[Gábor Stefanik]

But defaulting to Aero for future versions of Windows is much more likely to be correct than defaulting to Classic. Microsoft definitely won't switch back to Windows 2000.

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

My point is that neither is likely to be correct, for instance if we start hard coding colors and images based on the very cold Aero color palette and widget appearances, these are inevitably going to contrast if Microsoft switches to a warmer color palette.  My overall goal is for us to try to integrate visually with the current OS theme, which means we should have no expectation of future proofing our design.

For instance our OS X theme looks great, in large part due to the number of images and hard coded colors that are being used throughout.  When OS 11, or 10.x comes out, there is really no chance that the theme is going to look right, but that is the cost of getting it right for Leopard.  On Windows we haven't previously taken a similar approach because we are limiting ourselves to extractable colors and backgrounds in an attempt to be more robust when the OS changes, but it is costing us in terms of the quality of the current appearance.

Comment 9

[Gábor Stefanik]

I agree that if Microsoft completely redesigns the Windows UI, then the current design will look out-of-place and non-native - but it is definitely more correct than displaying a mix of Aero and Classic. A non-native appearance is significantly better than a broken one.

Comment 10

[Dão Gottwald [:dao]]

What do you mean by "Aero"? Where do you see it?

Comment 11

[Gábor Stefanik]

I mean a mix of (the version of Strata that is shown when Firefox is running on Windows Vista with either the Windows Aero, Windows Vista Standard or Windows Vista Basic theme) and (the version of Strata that is shown when Firefox is running on Windows Vista with the Windows Classic theme). But these are painstakingly long names for a theme.

Comment 12

[Gábor Stefanik]

BTW this appears to be not a Windows 7 bug - setting compatibility mode to "Windows Vista RTM" fixes all theme glitches. (Setting "Windows XP SP2" results in the XP Strata appearing, again without rendering errors.) Again, I say that if we find an unknown OS with a version number that is too high, default to the theme targeted at the newest known OS. Definitely better than displaying a mix of theme elements from different OS-specific themes.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

My mistake; Firefox 3.1 won't be built to target Windows 7, so this doesn't block.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

>BTW this appears to be not a Windows 7 bug - setting compatibility mode to
>"Windows Vista RTM" fixes all theme glitches.

It's not a bug, it's a feature :)  What is specifically going on is that our theme is falling back to the classic appearance because we don't detect that the user has the Aero OS theme (since they aren't even running Vista anymore).

https://developer.mozilla.org/En/CSS::-moz-system-metric%28windows-default-theme%29

We need to get this fixed, but it might involve some theme re-factoring, and changes to how we deal with detecting the OS theme on Windows, since now we have 3 default OS themes to deal with.  A quick fix in the meantime would be to make sure that -moz-system-metric(windows-default-theme) is resolving correctly with the Windows 7 default theme, and overall this wouldn't be too bad since the new theme is (sort of) Aero-ish.

Comment 16

[Reed Loden [:reed]]

Attachment: possible patch

Dunno if this works or not.

Comment 17

[Gábor Stefanik]

(In reply to comment #15)
> It's not a bug, it's a feature :)  What is specifically going on is that our
> theme is falling back to the classic appearance because we don't detect that
> the user has the Aero OS theme (since they aren't even running Vista anymore).

IMO it is correct to fall back to classic if the OS version is less than what we support - however, falling back to classic for too high OS versions is much less likely to look good than falling back to the theme of the highest supported OS version.

Comment 18

[Dão Gottwald [:dao]]

We're not falling back to classic. We're falling back to the natively styled textbox for unknown themes.

Comment 19

[Rob Arnold [:robarnold]]

Comment on attachment 356289 [details] [diff] [review]
possible patch

Don't have 7 yet, but this looks like it should fix it.

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: screenshot with the patch applied

The patch does fix the issue.

Attaching screenshot from test build:
https://build.mozilla.org/tryserver-builds/2009-01-12_12:[e-mail]-1231793606/

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why don't we just take this check out instead of having to update it every time a version of Windows is released?

Comment 22

[Alex Faaborg [:faaborg] (Firefox UX)]

>Why don't we just take this check out instead of having to update it every time
>a version of Windows is released?

We currently rely on the check in order to provide images and colors that are consistent with the appearance of the surrounding operating system, things that we can't automatically extract.  Ideally we would be able to query the name of the OS theme being used, as opposed to just finding out if it happens to be the default.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean in particular this test "GetWindowsVersion() <= VISTA_VERSION" (which covered all Windows versions) which we're changing to "GetWindowsVersion() <= WIN7_VERSION" (which will now cover all Windows versions). What is the point of this check if we're going to keep changing it to cover all Windows versions?

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #23)
> I mean in particular this test "GetWindowsVersion() <= VISTA_VERSION" (which
> covered all Windows versions) which we're changing to "GetWindowsVersion() <=
> WIN7_VERSION" (which will now cover all Windows versions). What is the point of
> this check if we're going to keep changing it to cover all Windows versions?

Microsoft could ship a different flavour of the aero theme, which would make the hardcoded stuff wrong. The only reason we can simply update the version now is that Vista's aero and Windows7's aero are very close, although there are already subtle differences, I think.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We already look wrong in each new version of Windows, so how is this check helping us?

Comment 26

[Dão Gottwald [:dao]]

Microsoft won't alter the aero theme within the already-released Vista. Or maybe I don't understand what you mean.

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #25)
> We already look wrong in each new version of Windows

Ah, I get it. Yes, it doesn't look ideal, because we can't pretend to know the theme and hardcode stuff according to it. We're using native theming instead, so it's not wrong technically.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 356289 [details] [diff] [review]
possible patch

This seems pointless to me, but it's not worth worrying about.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean, we know that when Windows 8 or whatever comes out, we're going to change our code to look good with it. At that point we can tweak this code as necessary. Having this check is really pretending that our current code could be acceptable for Windows 8, which just isn't true.

Comment 30

[Dão Gottwald [:dao]]

(In reply to comment #29)
> I mean, we know that when Windows 8 or whatever comes out, we're going to
> change our code to look good with it. At that point we can tweak this code as
> necessary. Having this check is really pretending that our current code could
> be acceptable for Windows 8, which just isn't true.

I assume you're referring to the whole code base. Certainly there's always something to tweak or something new to integrate with. But since Microsoft cares a bit about backwards-compatibility, running Firefox 3.1 on Windows 7, even though it won't be an official target, doesn't seem unacceptable for instance. As for the theme in particular, well, the idea of native theming is that it will be acceptable.

Comment 31

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/9ec0ab99182e

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 356289 [details] [diff] [review]
possible patch

a191=beltzner

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 356289 [details] [diff] [review]
possible patch

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 34

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b9d032bfae76

Checking in widget/src/windows/nsLookAndFeel.cpp;
/cvsroot/mozilla/widget/src/windows/nsLookAndFeel.cpp,v  <--  nsLookAndFeel.cpp
new revision: 1.75; previous revision: 1.74
done
Checking in widget/src/windows/nsWindow.h;
/cvsroot/mozilla/widget/src/windows/nsWindow.h,v  <--  nsWindow.h
new revision: 3.259; previous revision: 3.258
done