Closed Bug 1164952 Opened 5 years ago Closed 5 years ago

DevEdition theme has transparent window background in Windows 7 classic mode

Categories

(Firefox :: Theme, defect)

40 Branch
Unspecified
All
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 + fixed
firefox41 --- verified

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Keywords: regression, Whiteboard: [bugday-20150724])

Attachments

(2 files, 3 obsolete files)

+++ 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.
Attached image screenshot.png
No longer depends on: 1164178
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).
(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. ?
(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
See Also: → 1165212
See Also: → 1165284
Attached patch windows-transparency.patch (obsolete) — Splinter Review
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)
Blocks: 1165383
Attached patch windows-transparent.patch (obsolete) — Splinter Review
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
(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)
Attached patch windows-transparent.patch (obsolete) — Splinter Review
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)
(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)
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.
Blocks: 1165284, 1165212
See Also: 1165212, 1165284
[Tracking Requested - why for this release]: This is a major visual regression in Windows XP and Windows 7 Classic themes
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+
remote:   https://hg.mozilla.org/integration/fx-team/rev/7ac79cd0a688
Whiteboard: [fixed-in-fx-team]
Version that landed on fx-team
Attachment #8606555 - Attachment is obsolete: true
Attachment #8606585 - Flags: review+
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?
https://hg.mozilla.org/mozilla-central/rev/7ac79cd0a688
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Duplicate of this bug: 1165573
Duplicate of this bug: 1165694
Duplicate of this bug: 1165779
Duplicate of this bug: 1165822
Duplicate of this bug: 1165909
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)
@Russ: need to tweak about:config

follow this http://forums.mozillazine.org/viewtopic.php?p=14158799#p14158799
(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
It is time to back Bug 1158872 out from Aurora40.0a1
Flags: needinfo?(gijskruitbosch+bugs)
(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)
(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
Attachment #8606585 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [good first verify]
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]
It works fine. Tested on Firefox Aurora 41. Windows 8.1 [budday-20150729
Seems to be OK on Firefox Aurora 41. Windows 7. 64bit.
[Testday-20150731]
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.