567742 - Fullscreen mode makes the window badly manage Aero Glass

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Marco Bonardo [:mak]]

After glass has been reenabled, in bug 546259 and after the fix for bug 555182, fullscreen mode (F11) is still messed up, everything is glassed, included content, and looks like old chrome area is even glassed more then remaining content part.

Rob thinks could be a theme problem, or at least something in the theme that is acting on the nsWindow.

Filing it here for now, could be moved to widgets:win32 in case.

Comment 1

[Dão Gottwald [:dao]]

There's very little CSS dealing with fullscreen mode. Even if some CSS turns out to be related to this bug, I bet that's not the real cause but merely triggers a widget bug.

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

Should be noted that with dw/d2d enabled full-screen does not fade out in the content area.  

With dw/d2d on, the menu bar etc are 'ugly'.. but that's another bug.
bug 554874

Comment 3

[Shawn Thompson]

Attachment: Screenshot

Yes, I'm getting this problem too

Comment 4

[u88484]

Attachment: Comparison screenshot

Comment 5

[Rob Arnold [:robarnold]]

So the real cause appears to be that we can't distinguish a fullscreen glass window from a glass popup. Eitherway, a fullscreen window shouldn't be glass IMO so we'll need some CSS to fix the problem. There is still an underlying bug however which I am currently investigating.

Comment 6

[[not reading bugmail]]

If you wear a lwtheme this bug doesn't show up.

Comment 7

[Rob Arnold [:robarnold]]

Attachment: Fullscreen windows should not use glass

This is a partial fix - the remainder of the fix is in bug 567771.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 447185 [details] [diff] [review]
Fullscreen windows should not use glass

this disables glass but leaves all the adjustments we make for glass (toolbar appearance, text color, text shadow etc.)

Comment 9

[pepemosca]

Attachment: Screen capture of Minefield 20100525

Comment 10

[pepemosca]

Comment on attachment 447389 [details]
Screen capture of Minefield 20100525

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100525 Minefield/3.7a5pre (.NET CLR 3.5.30729)

Comment 11

[Paul Buonopane]

The problem is that DwmExtendFrameIntoClientArea is being called with the pMargins parameter pointing to a MARGINS struct with a negative value.

Comment 12

[Rob Arnold [:robarnold]]

(In reply to comment #11)
> The problem is that DwmExtendFrameIntoClientArea is being called with the
> pMargins parameter pointing to a MARGINS struct with a negative value.

I did not notice this - do you still see this after bug 567771 was fixed? It should be resetting the margins to be all zeros.

Comment 14

[pal-moz]

Attachment: screenshot

(In reply to comment #12)
> I did not notice this - do you still see this after bug 567771 was fixed? It
> should be resetting the margins to be all zeros.

not fixed.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100528 Minefield/3.7a5pre ID:20100528040337

Comment 15

[Paul Buonopane]

One of the values of the MARGINS struct is negative.  Due to a bug in the DWM API outlined at http://www.earth2me.com/development/dwm, either all values should be negative or none should be negative.

What baffles me is why there's glass in the first place when it's fullscreen.  In a borderless window, there's no non-client area to extend in the first place.  Using DwmExtendFrameIntoClientArea on a borderless window does not add glass.

There's another DWM function to add glass, DwmEnableBlurBehindWindow, but rather than working from the border in, it works from the center out to the border.  I don't see any reason for it to be used anywhere in the application, unless we start to allow windows with no tabs, but maybe it's getting called?

Comment 16

[Paul Buonopane]

Oh, I see... the glass remains because the the top need to be glass-ified when the mouse goes to the top of the screen.  That could be a problem.  Maybe we need to rethink how that dropdown works?  It's always seemed rather clunky to me, especially in how it displaces the content beneath it.

Comment 17

[Paul Buonopane]

Here's a Spy++ report of the style info while in fullscreen mode.

Class: DummyWindowless
WS_VISIBLE
WS_CLIPSIBLINGS
WS_OVERLAPPED
WS_MINIMIZEBOX
WS_MAXIMIZEBOX
WS_EX_LEFT
WS_EX_LTRREADING
WS_EX_RIGHTSCROLLBAR

Class: MozillaWindowClass (Parent: DummyWindowless)
WS_CHILDWINDOW
WS_VISIBLE
WS_CLIPSIBLINGS
WS_CLIPCHILDREN
WS_EX_LEFT
WS_EX_LTRREADING
WS_EX_RIGHTSCROLLBAR

Comment 18

[Rob Arnold [:robarnold]]

We just need to not use glass on the window in fullscreen mode. I don't see any easy way to do this with the CSS but I admit I don't know all the latest tricks. Unassigning from myself so that someone with more experience here can take the bug.

We can also wait for the fullscreen UI mockups to be done (no bug filed yet).

Comment 19

[Paul Buonopane]

I can probably help fix this, but I don't believe I have access to the source code.  As far as I can tell, the nightly source code isn't available to the public.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> I can probably help fix this, but I don't believe I have access to the source
> code.  As far as I can tell, the nightly source code isn't available to the
> public.

https://developer.mozilla.org/En/Developer_Guide/Source_Code

Comment 21

[Paul Buonopane]

Maybe I'm being stupid, but I couldn't find the source code for 3.7 anywhere in the links on that page.

Comment 22

[Rob Arnold [:robarnold]]

(In reply to comment #21)
> Maybe I'm being stupid, but I couldn't find the source code for 3.7 anywhere in
> the links on that page.

https://developer.mozilla.org/En/Developer_Guide/Source_Code/Mercurial has instructions for obtaining the source to 3.7 (mozilla-central).

Comment 24

[Paul Buonopane]

Found the problem.  Rather than disabling glass when dwStyle does not have the WS_CAPTION flag, it unconditionally extends the glass.  Why it was set to do this is beyond me, considering that should almost never be the case.  Working on a patch.  (Written, just need to test it.)

Comment 25

[Rob Arnold [:robarnold]]

(In reply to comment #24)
> Found the problem.  Rather than disabling glass when dwStyle does not have the
> WS_CAPTION flag, it unconditionally extends the glass.  Why it was set to do
> this is beyond me, considering that should almost never be the case.

Actually it was the primary use case when Aero Glass first landed. If you set browser.ctrlTab.previews to true in about:config and press ctrl-tab or ctrl-shift-tab, you'll see a glass panel popup and it doesn't have a caption (just like the alt-tab dialog doesn't have a caption). That is what the check is for because for those panels, we want the entire window to be glass. This behavior should not be changed because future UI work may depend on having full glass panels.

Comment 27

[[not reading bugmail]]

So is there no way to tell it when to restrict and when to extend depending on the desired panel use or would that everything more complex with two rendering options?

Comment 29

[pepemosca]

Still broken:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100603 Minefield/3.7a5pre (.NET CLR 3.5.30729)

:(

Comment 30

[Paul Buonopane]

Shouldn't we be setting glass on a window-by-window basis?  Clearly the "glass if no WS_CAPTION" method does not work.  I see no reason for something as simple as a preview pane to be following the same "path" as the main window.  Isn't there something about glass that can be set in the CSS for the theme?  I thought I saw it somewhere.  Would that not be a better place to override the default glass behavior?  In fact, wouldn't that be the best place to define when we use glass at all--in other words, default to no glass?  That certainly seems more backward-compatible in theory.

Comment 31

[Paul Buonopane]

Patch by Rob Arnold (when manually applied to an existing installation by editing the theme JAR) seems to do the trick.  As a side note, browser.ctrlTab.previews=true has no effect; I doubt this was caused by the patch, though.

Comment 32

[Rob Arnold [:robarnold]]

(In reply to comment #30)
> Shouldn't we be setting glass on a window-by-window basis?

nsWindow::mTransparencyMode will tell you if glass should be considered for the window. By default, it has the value of eTransparencyOpaque.

> Clearly the "glass
> if no WS_CAPTION" method does not work.

This is not clear to me. A fullscreen window with glass over part of it is not in any UI mockup and I cannot think of a valid use case.

> I see no reason for something as
> simple as a preview pane to be following the same "path" as the main window.

It's all the same as this level of the code. They're both toplevel native windows.
 
> Isn't there something about glass that can be set in the CSS for the theme?  I
> thought I saw it somewhere.  Would that not be a better place to override the
> default glass behavior? 

Glass is set on the window via the "-moz-appearance: -moz-win-glass" rule, mTransparencyMode is set to eTransparencyGlass. Your suggested approach is indeed the one I wanted to take in my original patch. I did not see an elegant way to fix it in the CSS though. My original patch turned off glass on the main window but did not undo the other glass-assuming changes.

> In fact, wouldn't that be the best place to define
> when we use glass at all--in other words, default to no glass?  That certainly
> seems more backward-compatible in theory.

The aforementioned code that checks for WS_CAPTION is only run on windows where mTransparencyMode == eTransparencyGlass so if we simply make it opaque by not applying that style in the CSS, we'll get an opaque window. This is the default behavior - we actually explicitly request glass for the main window in browser-aero.css.

Comment 35

[bogas04]

Have a look here , this bug is fun!
http://i47.tinypic.com/svjep1.png

Comment 37

[Kristofer]

Attachment: Printscreen from my dual-display setup demonstrating the bug.

Comment 38

[Kristofer]

Comment on attachment 449980 [details]
 Printscreen from my dual-display setup demonstrating the bug.

As you can see there is a window behind minefield (latest firefox beta). I run minefield using the default theme coming with it. Only the bright parts of the background seems to shine through, while the rest of the image seems to keep it's original color.

Comment 39

[u88484]

Yes, it was confirmed long ago that this bug exists.  No more screenshots or me too comments are needed.

Comment 40

[Kristofer]

(In reply to comment #39)
> Yes, it was confirmed long ago that this bug exists.  No more screenshots or me
> too comments are needed.

Okay, I'm so sorry...

Comment 41

[Rob Arnold [:robarnold]]

This should block 1.9.3 because it makes fullscreen mode unusable with the new glass theme.

Comment 42

[Filip Zakrzewski]

Attachment: After flash movie crashing aero

After some time watching flash movie at full screen (flash full screen) normal mode is crashed too (screen attached - look at File, Edit, and address bar)

Comment 44

[[not reading bugmail]]

So can't we use the 1px black line currently separating the content area and toolbars in regular mode also in fullscreen to distinguish aero from content?  

I see it disappears from fullscreen view.

Comment 45

[Will Pittenger]

I see this with Vista SP2 64-bit in 3.6.3 with AeroBuddy.  I have Qute as my theme.  AIOS also has problems with Aero that should be fixed too.

Comment 46

[Tyler Downer [He/Him]]

Will, you can't be seeing this bug on 3.6.3, because glass is not enabled. If this is an extension or theme you are using, then report to the extension authors

Comment 47

[Will Pittenger]

Actually, I am under the impression that AeroBuddy code was used to write the code now in trunk.

Comment 48

[Rob Arnold [:robarnold]]

(In reply to comment #47)
> Actually, I am under the impression that AeroBuddy code was used to write the
> code now in trunk.

Seeing as I can't remember having ever heard of AeroBuddy much less seen its source, I don't think that's the case. Glass is not well supported on 3.6.3, certainly not for the main window (it was not intended for the main window at the time).

Comment 49

[Will Pittenger]

Well, you might at least test Vista and AIOS.  If AIOS has a problem, you might alert them.

Comment 50

[Tyler Downer [He/Him]]

It isn't really our responsibility to tell extension developers that their extensions are having problems, most the time. if you are enabling aero somehow in 3.6.3, and then getting issues with the all in one sidebar, then we aren't really to blame. And the all in one sidebar is not compatible with trunk yet even, so there is no place where there is a supported configuration of firefox with aero and a supported AIOS.

Comment 51

[Dão Gottwald [:dao]]

Attachment: use a custom background instead of glass in FS mode

Comment 52

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #51)
> Created an attachment (id=451886) [details]
> use a custom background instead of glass in FS mode

Dão Gottwald, you patch actually doesn't fix the problem: you just added a new rule to override the bug for the most cases. But: 1. you make code more complicated, by adding more lines, instead of editing the existing ones.
2. in some cases the bug will still show up (like when you'll try to hide Fx's window titlebar, for example by an extension like <a href="https://addons.mozilla.org/ru/firefox/addon/71425/">foxiframe basic</a>).
3. with your patch the glass effect for top panels in F11 full-screen mode will be lost, the background will be colored instead.

My patch probably completely fixes the bug, it doesn't add more code, it just edits 1 line of code. And the top toolbars in F11 full-screen mode will stay glassed.

p.s.: yet, I don't know how to add the patch to the comment and I'm not sure if the comment will be editable after I commit it.

Comment 53

[Filip Zakrzewski]

https://bugzilla.mozilla.org/attachment.cgi?bugid=567742&action=enter
https://developer.mozilla.org/en/Getting_your_patch_in_the_tree

There is info how to add patch

Add it as fast as possible to be reviewed

Comment 54

[Please Ignore This Troll (Account Disabled)]

Attachment: Pure fix of the bug, not by coloring any toolbars, not by complicating code much

It took me long to create a diff file, but now I'm able to submit patches. Try this one, it should work better, than the one proposed by Dao. Please, review it asap.

Comment 55

[Dão Gottwald [:dao]]

Comment on attachment 452317 [details] [diff] [review]
Pure fix of the bug, not by coloring any toolbars, not by complicating code much

This patch doesn't do anything, as neither #main-window:not(:-moz-lwtheme) > #nav-bar nor #main-window:not(:-moz-lwtheme) > #TabsToolbar match anything.

Comment 56

[Please Ignore This Troll (Account Disabled)]

a typo, meant this:
+  #main-window:not(:-moz-lwtheme), #main-window:not(:-moz-lwtheme) > #navigator-toolbox > #nav-bar, #main-window:not(:-moz-lwtheme) > #navigator-toolbox > #TabsToolbar {


change it that way and it will work

Comment 57

[Please Ignore This Troll (Account Disabled)]

please, someone make a diff file, I was emailed by Kurt - he said there is something wrong with local file structure, so the patch won't fly anyway - in the previous post I've already said what should be edited and where - please, someone do it asap in order to fasten the process of fixing this particular bug (some of the devs owed to mark it as blocking 4.0 beta)

Comment 58

[Matt Brubeck (:mbrubeck)]

Attachment: patch based on comment 56

Comment 59

[BuTaMuH]

This "new" patch actually doesn't work, not for me at least

Comment 60

[Please Ignore This Troll (Account Disabled)]

yeah, for some reason it doesn't work for me either, though I've asked 3-4 times different people if the CSS syntax is correct. Seems like there is some deeper bug in Fx that doesn't let working with such complicated code (when there is a pseudo-element like :not() is being used for another pseudo-element like :-moz-lwtheme and when the global rule is being applied to that double pseudo-element's child(ren))

changing that line to
#main-window:not(:-moz-lwtheme), #main-window >
#navigator-toolbox > #nav-bar, #main-window >
#navigator-toolbox > #TabsToolbar {

works for me, but it needs to be tested with personas/themes. Something tells me those 2 bars will always have a glassed background, though if personas/theme code has !important in it's code - there won't be a problem, but I'm not sure if there will be one for a theme/persona that is not using !important in it's rules.
Someone please re-make the patch using this code and test it with different personas.

Comment 61

[Please Ignore This Troll (Account Disabled)]

I've run a few tens of experiments with personas/themes and they all work good.
Seems like my thought, that any theme/persona's code has more priority than Fx's one, because it is being applied after the Fx's code.
So probably this time it should work and work good. And shouldn't brake anything, though you still can test it.

Comment 62

[Please Ignore This Troll (Account Disabled)]

Attachment: based on comment #60

hope this diff file has correct structure

Comment 63

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Might you want :not(:-moz-lwtheme) on all the selectors?

Comment 64

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 452449 [details] [diff] [review]
based on comment #60

>diff -r 6b648d903230 browser/themes/winstripe/browser/browser-aero.css

>-  #main-window:not(:-moz-lwtheme) {
>+  #main-window:not(:-moz-lwtheme),
>+  #main-window > #navigator-toolbox > #nav-bar,
>+  #main-window > #navigator-toolbox > #TabsToolbar {

The "#main-window" part of these selectors is redundant (#navigator-toolbox never has any other parent). I also don't understand why this change would have any impact on the bug as summarized.

Comment 65

[Please Ignore This Troll (Account Disabled)]

Attachment: finally the proper diff

oh my god, I'm so stupid: I diffed the wrong file, that one was for tests
I was wondering why my patches work good for me, though manually changing as it is described in the diff - doesn't lead to the same results.

Comment 66

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 452506 [details] [diff] [review]
finally the proper diff

This just makes that rule not match anything - it's equivalent to removing the rule, which I don't think we want...

Comment 67

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #66)
> (From update of attachment 452506 [details] [diff] [review])
> This just makes that rule not match anything - it's equivalent to removing the
> rule, which I don't think we want...

no, that makes match anything except the #browser area, which is content area + sidebar, since I don't use sidebar at all - I totally forgot about it, but anyways I don't think anyone wants it to be glassed (otherwise #browser is better be changed to #appcontent or even just #content).

but saying that this patch is equal deleting the rule - is not true: I have this patch applied and I have menubar and tabbar glassed, just as needed. So the rule is actually matching all the needed elements.

Comment 68

[Please Ignore This Troll (Account Disabled)]

did a few more tests - seems like the bug can't be fixed by CSS.

Comment 69

[Dão Gottwald [:dao]]

(In reply to comment #67)
> no, that makes match anything except the #browser area

#main-window:not(:-moz-lwtheme) and #main-window:not(:-moz-lwtheme):not(#browser) are equivalent, your patch is a no-op.

Comment 71

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 451886 [details] [diff] [review]
use a custom background instead of glass in FS mode

The grey seems a bit off - is there a system color we could use here?

Comment 72

[Dão Gottwald [:dao]]

(In reply to comment #71)
> The grey seems a bit off - is there a system color we could use here?

I don't think so, at least none that we could use at the moment.

Comment 73

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e205cd7d28af

Comment 74

[Please Ignore This Troll (Account Disabled)]

this is not a proper bug fix :(
maybe you'd better land this fix to trunk but re-open this bug?

Comment 75

[WulfTheSaxon [:Wulf]]

(In reply to comment #71)
> (From update of attachment 451886 [details] [diff] [review])
> The grey seems a bit off - is there a system color we could use here?

There's Control Panel --> Appearance and Personalization --> Change window glass colors. The swatch colors are stored in HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Control Panel\Glass Colorization (the default, Sky, is 6b74b8fc in ARGB). Can't seem to find where the active color is stored, though.

Also, just ran across this: http://stackoverflow.com/questions/1487919/how-does-windows-change-aero-glass-color (the color is DwmGetColorizationColor, or there's DwmEnableComposition which at first glance seems like a better solution)

Comment 76

[Filip Zakrzewski]

Probably DwmGetColorizationColor is the best way to fix this bug.
More info at http://msdn.microsoft.com/en-us/library/aa969513%28VS.85%29.aspx
It should be a lot better than hardcoded color and it will be compatible with Windows 7 glass colors

Comment 77

[Rob Arnold [:robarnold]]

Yeah, we should add a -moz-win-glass color (the function call is the right way to implement it). There is or should be a bug filed on adding it. If that's the ideal color for the fullscreen background, then a bug should be filed on making that so. I should also note that there are fullscreen ui mockups coming so this is likely not the final design.

Comment 78

[Please Ignore This Troll (Account Disabled)]

please, someone, keep it on track, or the bug will stay unfixed for years...

Comment 79

[Daniel Desira]

The bug has only been partially fixed, since a small area at the top is still affected. I'll post a screen shot if requested.

Comment 80

[WulfTheSaxon [:Wulf]]

See also bug 577132 (which mentions http://www.withinwindows.com/2010/07/01/retrieving-aero-glass-base-color-for-opaque-surface-rendering/ )

Comment 81

[Will Pittenger]

I'm not convinced this is completely fixed.  I am running Firefox 5.0 on Vista 64-bit SP2.  My theme is Winstripe Toolbar Icons 2.5.9.  See the attached screen shots.

Comment 82

[Will Pittenger]

Attachment: What I see in full screen mode

As noted, this is Firefox 5.0 on Vista 64-bit SP2.  The toolbar shouldn't be that dark color.  If this is glass, it should be my wall paper (the default Vista paper).

Comment 83

[Daniel Desira]

(In reply to comment #82)
> Created attachment 541650 [details]
> What I see in full screen mode
> 
> As noted, this is Firefox 5.0 on Vista 64-bit SP2.  The toolbar shouldn't be
> that dark color.  If this is glass, it should be my wall paper (the default
> Vista paper).

I observed that this is also the case with the default skin on Nightly (Firefox 7) 64-bit on Windows 7 Ultimate 64-bit SP1. This means that it is a bug with full screen's integration with Aero.