Fullscreen mode makes the window badly manage Aero Glass

RESOLVED FIXED in Firefox 4.0b1

Status

()

Firefox
Theme
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: mak, Assigned: dao)

Tracking

({regression})

Trunk
Firefox 4.0b1
x86
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(8 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
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.
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

7 years ago
Created attachment 447128 [details]
Screenshot

Yes, I'm getting this problem too

Comment 4

7 years ago
Created attachment 447130 [details]
Comparison screenshot
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.
If you wear a lwtheme this bug doesn't show up.
Created attachment 447185 [details] [diff] [review]
Fullscreen windows should not use glass

This is a partial fix - the remainder of the fix is in bug 567771.
Assignee: nobody → tellrob
Status: NEW → ASSIGNED
Attachment #447185 - Flags: review?(dao)
Depends on: 567771
(Assignee)

Comment 8

7 years ago
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.)
Attachment #447185 - Flags: review?(dao) → review-

Comment 9

7 years ago
Created attachment 447389 [details]
Screen capture of Minefield 20100525

Comment 10

7 years ago
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

7 years ago
The problem is that DwmExtendFrameIntoClientArea is being called with the pMargins parameter pointing to a MARGINS struct with a negative value.
(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.
Duplicate of this bug: 568716

Comment 14

7 years ago
Created attachment 448013 [details]
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

7 years ago
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

7 years ago
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

7 years ago
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
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).
Status: ASSIGNED → NEW
(Assignee)

Updated

7 years ago
Assignee: tellrob → nobody

Comment 19

7 years ago
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.
(Assignee)

Comment 20

7 years ago
(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

7 years ago
Maybe I'm being stupid, but I couldn't find the source code for 3.7 anywhere in the links on that page.
(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).
Duplicate of this bug: 569062

Comment 24

7 years ago
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.)
(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.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 569156
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?
Duplicate of this bug: 569755

Comment 29

7 years ago
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

7 years ago
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

7 years ago
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.
(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.
Duplicate of this bug: 570418

Updated

7 years ago
Duplicate of this bug: 570446

Comment 35

7 years ago
Have a look here , this bug is fun!
http://i47.tinypic.com/svjep1.png
Duplicate of this bug: 570834

Comment 37

7 years ago
Created attachment 449980 [details]
 Printscreen from my dual-display setup demonstrating the bug.

Comment 38

7 years ago
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

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

Comment 40

7 years ago
(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...

Updated

7 years ago
blocking2.0: --- → ?
This should block 1.9.3 because it makes fullscreen mode unusable with the new glass theme.

Comment 42

7 years ago
Created attachment 450394 [details]
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)
Keywords: relnote
Duplicate of this bug: 571472
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

7 years ago
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.
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

7 years ago
Actually, I am under the impression that AeroBuddy code was used to write the code now in trunk.
(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

7 years ago
Well, you might at least test Vista and AIOS.  If AIOS has a problem, you might alert them.
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.
(Assignee)

Comment 51

7 years ago
Created attachment 451886 [details] [diff] [review]
use a custom background instead of glass in FS mode
Assignee: nobody → dao
Attachment #447185 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #451886 - Flags: review?(gavin.sharp)
(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

7 years ago
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
Created attachment 452317 [details] [diff] [review]
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.
Attachment #452317 - Flags: review?(dao)
(Assignee)

Comment 55

7 years ago
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.
Attachment #452317 - Flags: review?(dao) → review-
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
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)
Created attachment 452355 [details] [diff] [review]
patch based on comment 56
Attachment #452317 - Attachment is obsolete: true

Comment 59

7 years ago
This "new" patch actually doesn't work, not for me at least
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.
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.
Created attachment 452449 [details] [diff] [review]
based on comment #60

hope this diff file has correct structure
Attachment #452449 - Flags: review?(dao)
Might you want :not(:-moz-lwtheme) on all the selectors?
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.
Attachment #452449 - Flags: review?(dao) → review-
Created attachment 452506 [details] [diff] [review]
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.
Attachment #452449 - Attachment is obsolete: true
Attachment #452506 - Flags: review?(gavin.sharp)
Attachment #452506 - Flags: review?(dao)
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...
Attachment #452506 - Flags: review?(gavin.sharp)
Attachment #452506 - Flags: review?(dao)
Attachment #452506 - Flags: review-
(Assignee)

Updated

7 years ago
Attachment #452506 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #452355 - Attachment is obsolete: true
(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.
did a few more tests - seems like the bug can't be fixed by CSS.
(Assignee)

Comment 69

7 years ago
(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.
Duplicate of this bug: 573394
Attachment #451886 - Flags: review?(gavin.sharp) → review+
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?
(Assignee)

Comment 72

7 years ago
(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.
(Assignee)

Comment 73

7 years ago
http://hg.mozilla.org/mozilla-central/rev/e205cd7d28af
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: relnote
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a6
this is not a proper bug fix :(
maybe you'd better land this fix to trunk but re-open this bug?
(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

7 years ago
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
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.
please, someone, keep it on track, or the bug will stay unfixed for years...

Comment 79

7 years ago
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.
See also bug 577132 (which mentions http://www.withinwindows.com/2010/07/01/retrieving-aero-glass-base-color-for-opaque-surface-rendering/ )
blocking2.0: ? → betaN+

Comment 81

6 years ago
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

6 years ago
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).

Comment 83

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.