5.79 KB, image/png
1.93 KB, patch
|Details | Diff | Splinter Review|
21.69 KB, image/png
Created attachment 345594 [details] 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.
--> 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.
Component: Toolbars → Theme
QA Contact: toolbars → theme
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.
The "JetFox Aqua" theme does render correctly, though.
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.
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".)
>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.
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.
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.
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.
What do you mean by "Aero"? Where do you see it?
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.
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.
My mistake; Firefox 3.1 won't be built to target Windows 7, so this doesn't block.
Flags: blocking-firefox3.1+ → blocking-firefox3.1-
>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.
(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.
We're not falling back to classic. We're falling back to the natively styled textbox for unknown themes.
Component: Theme → Widget: Win32
Product: Firefox → Core
QA Contact: theme → win32
Summary: Site identity button looks weird in Firefox 3.1b1 on the Windows 7 PDC test build → Windows 7 default OS theme not recognized by windows-default-theme metric
Comment on attachment 356289 [details] [diff] [review] possible patch Don't have 7 yet, but this looks like it should fix it.
Attachment #356289 - Flags: review?(tellrob) → review+
Assignee: nobody → reed
Status: NEW → ASSIGNED
Created attachment 356595 [details] 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:firstname.lastname@example.org/
Why don't we just take this check out instead of having to update it every time a version of Windows is released?
>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.
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?
(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.
We already look wrong in each new version of Windows, so how is this check helping us?
Microsoft won't alter the aero theme within the already-released Vista. Or maybe I don't understand what you mean.
(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.
Attachment #356289 - Flags: superreview?(roc) → superreview+
Comment on attachment 356289 [details] [diff] [review] possible patch This seems pointless to me, but it's not worth worrying about.
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.
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Whiteboard: [needs 1.9.1 approval, baking]
Attachment #356289 - Flags: approval1.9.1? → approval1.9.1+
Attachment #356289 - Flags: approval220.127.116.11? → approval18.104.22.168+
Comment on attachment 356289 [details] [diff] [review] possible patch Approved for 22.214.171.124, a=dveditz for release-drivers.
Whiteboard: [needs 1.9.1 approval, baking]
Flags: blocking1.9.1? → blocking1.9.1+
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
Keywords: fixed126.96.36.199, fixed1.9.1
You need to log in before you can comment on or make changes to this bug.