Closed
Bug 1164952
Opened 9 years ago
Closed 9 years ago
DevEdition theme has transparent window background in Windows 7 classic mode
Categories
(Firefox :: Theme, defect)
Tracking
()
VERIFIED
FIXED
Firefox 41
People
(Reporter: bgrins, Assigned: bgrins)
References
Details
(Keywords: regression, Whiteboard: [bugday-20150724])
Attachments
(2 files, 3 obsolete files)
658.98 KB,
image/png
|
Details | |
2.49 KB,
patch
|
bgrins
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1164178 +++ And Toolbox/Toolbar items are transparent and title bar is also transparent on Windows7 Classic Visual style.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Tracked this down to the following CSS: https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#132-141 @media not all and (-moz-windows-compositor) { #main-window[tabsintitlebar] #titlebar:-moz-lwtheme { visibility: hidden; } #main-window[tabsintitlebar] #titlebar-content:-moz-lwtheme { -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox"); visibility: visible; } } We need to override this for dev edition (since we just want the normal titlebar to show up).
Comment 3•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #2) > Tracked this down to the following CSS: > https://dxr.mozilla.org/mozilla-central/source/browser/themes/windows/ > browser.css#132-141 > > @media not all and (-moz-windows-compositor) { > #main-window[tabsintitlebar] #titlebar:-moz-lwtheme { > visibility: hidden; > } > > #main-window[tabsintitlebar] #titlebar-content:-moz-lwtheme { > -moz-binding: > url("chrome://global/content/bindings/general.xml#windowdragbox"); > visibility: visible; > } > } > > We need to override this for dev edition (since we just want the normal > titlebar to show up). Do we? Doesn't that push tabs out of the titlebar etc. ?
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #3) > Do we? Doesn't that push tabs out of the titlebar etc. ? Since it's just setting visibility I doubt this will change anything with the layout (I could very well be wrong). The main issue is that the #titlebar has that beautiful classic blue gradient due to the -moz-window-titlebar appearance, and right now that's being hidden for lw-themes. In the DE case we actually want that visible since there is no background image to show. Let me know if you think there is a better approach
Assignee | ||
Comment 5•9 years ago
|
||
Gijs, this patch fixes the problem for me in Win7 classic theme mode. Basically here is what is happening to the best that I can tell: * Lightweight theme is applied, with a transparent accentcolor and transparent bg image * The #titlebar element is hidden with visibility:hidden (see Comment 2) * If you click on the transparent region the click goes through to the OS and blurs the window. * If I re-show the #titlebar element, the click doesn't go through to the OS but dragging it still doesn't work * I also need to add a background color to the #main-window to get original functionality working This feels pretty hacky, but I'm getting lost in all of the different issues coming up here. Does this seem like a reasonable fix? If not, any advice?
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8606305 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 6•9 years ago
|
||
Here's a try push for testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad8b9daf94b8
Assignee | ||
Comment 7•9 years ago
|
||
Based on further research and comments in Bug 1165383, I think maybe backing out the accentcolor change and instead handling that in CSS only when the compositor is available is going to be the least error-prone fix https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aa4dff36873
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #7) > Created attachment 8606477 [details] [diff] [review] > windows-transparent.patch > > Based on further research and comments in Bug 1165383, I think maybe backing > out the accentcolor change and instead handling that in CSS only when the > compositor is available is going to be the least error-prone fix > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aa4dff36873 Can you please confirm this fixes the problems you've been seeing with basic / classic mode? This build can be simulated as Dev Edition if you select the Dev Edition theme from the Addons Manager and then restart the browser. There's another build that's based on Aurora but it's not yet done: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af8bc1da4cef
Flags: needinfo?(alice0775)
Assignee | ||
Comment 9•9 years ago
|
||
I wish the lightweight theme system better supported transparent accentcolors, but it feels like the safest thing to do for 40 at this point is to back that part out and: 1) Land a separate CSS fix for the white titlebar issue on Win7 / 8 (the original report in Bug 1158872). 2) Land a fix to make the titlebar show up properly in the DE theme case when there is no compositor available (Win 7 basic/classic and WinXP) So that's what this patch does.
Attachment #8606305 -
Attachment is obsolete: true
Attachment #8606477 -
Attachment is obsolete: true
Attachment #8606305 -
Flags: feedback?(gijskruitbosch+bugs)
Attachment #8606555 -
Flags: review?(MattN+bmo)
Comment 10•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #8) > (In reply to Brian Grinstead [:bgrins] from comment #7) > > Created attachment 8606477 [details] [diff] [review] > > windows-transparent.patch > > > > Based on further research and comments in Bug 1165383, I think maybe backing > > out the accentcolor change and instead handling that in CSS only when the > > compositor is available is going to be the least error-prone fix > > > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aa4dff36873 > > Can you please confirm this fixes the problems you've been seeing with basic > / classic mode? This build can be simulated as Dev Edition if you select > the Dev Edition theme from the Addons Manager and then restart the browser. > > There's another build that's based on Aurora but it's not yet done: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=af8bc1da4cef See Bug 1165383 Comment #10, #11 for Windows7 Clasic Bug 1165383 Comment #12 for Windows7 Basic
Flags: needinfo?(alice0775)
Assignee | ||
Comment 11•9 years ago
|
||
This issue seems to be also be causing Bug 1165284, Bug 1165212, and Bug 1165383. Seems like there are some very bad side effects from having a transparent background color on #main-window in Windows without -moz-windows-compositor.
Assignee | ||
Comment 12•9 years ago
|
||
[Tracking Requested - why for this release]: This is a major visual regression in Windows XP and Windows 7 Classic themes
tracking-firefox40:
--- → ?
Comment 13•9 years ago
|
||
Comment on attachment 8606555 [details] [diff] [review] windows-transparent.patch Review of attachment 8606555 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/windows/devedition.css @@ +16,5 @@ > + So instead just show the normal titlebar in that case, and override the > + window color as transparent when the compositor is available. */ > +@media not all and (-moz-windows-compositor) { > + #main-window[tabsintitlebar] #titlebar:-moz-lwtheme { > + visibility: visible; As discussed in-person, I think we should also be setting the window background to var(--chrome-background-color) so it doesn't show as white in some cases e.g. fullscreen.
Attachment #8606555 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 14•9 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/7ac79cd0a688
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 15•9 years ago
|
||
Version that landed on fx-team
Attachment #8606555 -
Attachment is obsolete: true
Attachment #8606585 -
Flags: review+
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8606585 [details] [diff] [review] windows-transparent.patch Approval Request Comment [Feature/regressing bug #]: 1158872 [User impact if declined]: Users running Dev Edition with Windows XP or Win 7 classic or basic theme will see a transparent browser window and that is broken in various ways (can't drag the window, flash video doesn't play) [Describe test coverage new/current, TreeHerder]: CSS change only without tests [Risks and why]: There is a risk that this CSS change could cause some other problems, although it's limited to Dev Edition theme and Windows XP or 7 with the basic / classic theme. There shouldn't be a change for Win 7+ (normal theme). We've also tested with mozscreenshots to make sure we aren't missing anything obvious in XP / 7 / 8. [String/UUID change made/needed]:
Attachment #8606585 -
Flags: approval-mozilla-aurora?
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7ac79cd0a688
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Comment 23•9 years ago
|
||
This bug just hit me today - figured I'd workaround it by changing my theme but even that has issues: whichever theme I choose, after restart, I'm back with the dev.ed theme (i.e. the change wont stick)
Comment 24•9 years ago
|
||
@Russ: need to tweak about:config follow this http://forums.mozillazine.org/viewtopic.php?p=14158799#p14158799
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Russ from comment #23) > This bug just hit me today - figured I'd workaround it by changing my theme > but even that has issues: whichever theme I choose, after restart, I'm back > with the dev.ed theme (i.e. the change wont stick) This has been fixed in Bug 1164178, but is also awaiting uplift
Comment 26•9 years ago
|
||
It is time to back Bug 1158872 out from Aurora40.0a1
Flags: needinfo?(gijskruitbosch+bugs)
Comment 27•9 years ago
|
||
(In reply to Alice0775 White from comment #26) > It is time to back Bug 1158872 out from Aurora40.0a1 When this patch has landed and is going to be uplifted today or tomorrow? Doesn't make sense to me.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 28•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #27) > (In reply to Alice0775 White from comment #26) > > It is time to back Bug 1158872 out from Aurora40.0a1 > > When this patch has landed and is going to be uplifted today or tomorrow? > Doesn't make sense to me. I've pinged a release manager about this who is going to look at the request today
Updated•9 years ago
|
Attachment #8606585 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
status-firefox40:
--- → affected
Updated•9 years ago
|
QA Whiteboard: [good first verify]
Comment 30•9 years ago
|
||
Reproduced on Firefox Developer version 40.0a2 : Build ID 20150514004002 User Agent Mozilla/5.0 (Windows NT 6.1; rv:40.0) Gecko/20100101 Firefox/40.0 And this is totally fine on latest Developer version 41.0a2 : Build ID 20150724004006 User Agent Mozilla/5.0 (Windows NT 6.1; rv:41.0) Gecko/20100101 Firefox/41.0
QA Whiteboard: [good first verify] → [good first verify][bugday-20150724]
Whiteboard: [bugday-20150724]
Comment 31•9 years ago
|
||
Thanks!
Comment hidden (spam) |
Comment 33•9 years ago
|
||
It works fine. Tested on Firefox Aurora 41. Windows 8.1 [budday-20150729
Comment 34•9 years ago
|
||
Seems to be OK on Firefox Aurora 41. Windows 7. 64bit. [Testday-20150731]
Comment 35•9 years ago
|
||
Seems to be OK on Firefox 41.0b7. Windows 7 x64. Tried all variations with/without: titlebar, menu bar shown, bookmark toolbar, light/dark theme. [Testday-20150904]
Assignee | ||
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•