Closed
Bug 567742
Opened 15 years ago
Closed 14 years ago
Fullscreen mode makes the window badly manage Aero Glass
Categories
(Firefox :: Theme, defect)
Tracking
()
RESOLVED
FIXED
Firefox 4.0b1
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: mak, Assigned: dao)
References
Details
(Keywords: regression)
Attachments
(8 files, 5 obsolete files)
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•15 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.
Comment 2•15 years ago
|
||
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•15 years ago
|
||
Yes, I'm getting this problem too
Comment 5•15 years ago
|
||
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•15 years ago
|
||
If you wear a lwtheme this bug doesn't show up.
Comment 7•15 years ago
|
||
This is a partial fix - the remainder of the fix is in bug 567771.
Assignee | ||
Comment 8•15 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 10•15 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•15 years ago
|
||
The problem is that DwmExtendFrameIntoClientArea is being called with the pMargins parameter pointing to a MARGINS struct with a negative value.
Comment 12•15 years ago
|
||
(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•15 years ago
|
||
(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•15 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•15 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•15 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
Comment 18•15 years ago
|
||
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•15 years ago
|
Assignee: tellrob → nobody
Comment 19•15 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•15 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•15 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.
Comment 22•15 years ago
|
||
(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•15 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.)
Comment 25•15 years ago
|
||
(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•15 years ago
|
||
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•15 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•15 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•15 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.
Comment 32•15 years ago
|
||
(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•15 years ago
|
||
Have a look here , this bug is fun!
http://i47.tinypic.com/svjep1.png
Comment 37•15 years ago
|
||
Comment 38•15 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•15 years ago
|
||
Yes, it was confirmed long ago that this bug exists. No more screenshots or me too comments are needed.
Comment 40•15 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•15 years ago
|
blocking2.0: --- → ?
Comment 41•15 years ago
|
||
This should block 1.9.3 because it makes fullscreen mode unusable with the new glass theme.
Comment 42•15 years ago
|
||
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•15 years ago
|
||
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•15 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.
Comment 46•15 years ago
|
||
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•15 years ago
|
||
Actually, I am under the impression that AeroBuddy code was used to write the code now in trunk.
Comment 48•15 years ago
|
||
(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•15 years ago
|
||
Well, you might at least test Vista and AIOS. If AIOS has a problem, you might alert them.
Comment 50•15 years ago
|
||
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•15 years ago
|
||
Assignee: nobody → dao
Attachment #447185 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #451886 -
Flags: review?(gavin.sharp)
Comment 52•15 years ago
|
||
(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•15 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
Comment 54•14 years ago
|
||
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•14 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-
Comment 56•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
Attachment #452317 -
Attachment is obsolete: true
Comment 59•14 years ago
|
||
This "new" patch actually doesn't work, not for me at least
Comment 60•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
hope this diff file has correct structure
Attachment #452449 -
Flags: review?(dao)
Comment 63•14 years ago
|
||
Might you want :not(:-moz-lwtheme) on all the selectors?
Comment 64•14 years ago
|
||
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-
Comment 65•14 years ago
|
||
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 66•14 years ago
|
||
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•14 years ago
|
Attachment #452506 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #452355 -
Attachment is obsolete: true
Comment 67•14 years ago
|
||
(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•14 years ago
|
||
did a few more tests - seems like the bug can't be fixed by CSS.
Assignee | ||
Comment 69•14 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.
Updated•14 years ago
|
Attachment #451886 -
Flags: review?(gavin.sharp) → review+
Comment 71•14 years ago
|
||
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•14 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•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: relnote
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a6
Comment 74•14 years ago
|
||
this is not a proper bug fix :(
maybe you'd better land this fix to trunk but re-open this bug?
Comment 75•14 years ago
|
||
(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•14 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
Comment 77•14 years ago
|
||
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•14 years ago
|
||
please, someone, keep it on track, or the bug will stay unfixed for years...
Comment 79•14 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.
Comment 80•14 years ago
|
||
See also bug 577132 (which mentions http://www.withinwindows.com/2010/07/01/retrieving-aero-glass-base-color-for-opaque-surface-rendering/ )
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 81•13 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•13 years ago
|
||
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•13 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.
Description
•