Closed Bug 633282 Opened 13 years ago Closed 13 years ago

Aero glass glaze effect jumps down when status info overlay appears

Categories

(Core :: Layout, defect)

x86
Windows Vista
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla6
Tracking Status
blocking2.0 --- -

People

(Reporter: ehoogeveen, Assigned: jimm)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(10 files, 21 obsolete files)

5.80 KB, patch
roc
: review+
Details | Diff | Splinter Review
52.96 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.27 KB, patch
sdwilsh
: review+
Details | Diff | Splinter Review
3.42 KB, patch
Details | Diff | Splinter Review
701 bytes, patch
dao
: review+
Details | Diff | Splinter Review
563 bytes, patch
dao
: review+
Details | Diff | Splinter Review
4.62 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.36 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
20.54 KB, image/png
Details
User-Agent:       Mozilla/5.0 (Windows NT 6.0; WOW64; rv:2.0b12pre) Gecko/20110210 Firefox/4.0b12pre
Build Identifier: Mozilla/5.0 (Windows NT 6.0; WOW64; rv:2.0b12pre) Gecko/20110210 Firefox/4.0b12pre

Every time the status info overlay activates during page load, the white glaze gradient jumps down by about 20 pixels. This creates the impression that something is going on in the background and can be quite distracting. With the Add-on bar enabled there is still a small shift, but it is small enough to be nearly unnoticeable.

Reproducible: Always

Steps to Reproduce:
1. Disable the Add-on bar if enabled
2. Load or reload a page
Actual Results:  
White glaze gradient jumps down during page load, then up again as the status info overlay goes away.

Expected Results:  
Glaze gradient stays put.

See bug 623092 for a description of the glaze effect, with a screenshot.
Bug 628654 is where the status info overlay was introduced, though the bug also shows up to a lesser extent with the full Add-on bar.
Version: unspecified → Trunk
blocking2.0: --- → ?
Odd, I wonder why that panel on the bottom changes the top margin. Tn, any ideas?

download panel hidden:
0[2965a8]: glass margins: left:3 top:84 right:3 bottom:3

download panel visible:
0[2965a8]: glass margins: left:3 top:113 right:3 bottom:3
Status: UNCONFIRMED → NEW
Ever confirmed: true
The glass margins are computed from the visible region, right? What is the visible region in both cases?
The pixel-difference depends on the number of open tabs. See screens in 
https://bugzilla.mozilla.org/show_bug.cgi?id=631236
Blocks: 633719
(In reply to comment #2)
> The glass margins are computed from the visible region, right? What is the
> visible region in both cases?

I'll debug this week. I understand how we calculate this, what I found odd was that the status panel, which obscures content, changes the results. Especially weird is that it changes to top bounds of visible content.
Yeah, that is odd.
This is very odd. It's as if the status panel is being computed as extra glass chrome above the content area. Could it be that the positioning of the status panel defaults to the top before it's positioned by css and it is slow enough that the window firsts sees it as at the top or something like that? This is kind of distracting for anyone that uses restored windows regularly.
Annoying, and kind of distracting, but not disruptive enough to hold back Firefox 4 over, blocking-. Would take a safe fix on the late train if it's ready.
blocking2.0: ? → -
I find this position changing very irregular. As I'm reading different bugs about it, sometimes the glaze jumps around when hovering a link, sometimes it doesn't move at all. I haven't found a system with the presence of the add-on bar, the number of open tabs, the switching between tabs, the opening of the add-ons view or restarting Firefox. The behaviour seems to change all the time.
I did a little debugging on this end of last week. The status panel does effect the top bounds of the visible region handed down to widget's UpdateTransparentRegion. I need to dig into layout a bit further to try and understand why.

I wonder if moving the status panel inward a pixel or two so that it doesn't intersect the edge of content would help.
Severity: trivial → major
Component: Theme → Layout
Product: Firefox → Core
QA Contact: theme → layout
As we're at it, I've often wondered why every other window has a 1px white (or at least brighter) border inside the transparent rectangle, but Firefox does not. You can easily see the difference if you place Firefox and another window next to each other. Is this a UI design thing, does it have to be like that for some reason?
(In reply to comment #14)
> As we're at it, I've often wondered why every other window has a 1px white (or
> at least brighter) border inside the transparent rectangle, but Firefox does
> not. You can easily see the difference if you place Firefox and another window
> next to each other. Is this a UI design thing, does it have to be like that for
> some reason?

That's bug 618353.
No longer blocks: 633719
(In reply to comment #14)
> As we're at it, I've often wondered why every other window has a 1px white (or
> at least brighter) border inside the transparent rectangle, but Firefox does
> not. You can easily see the difference if you place Firefox and another window
> next to each other. Is this a UI design thing, does it have to be like that for
> some reason?

I actually hope we don't add the white border. It gives the app an inset appearance which cannot be well integrated with toolbars with rounded corners (our navigation bar) and just doesn't make any sense at all with tabs. Chrome also looks like Firefox and the appearance matches our design mock-ups. 

The border is too wide, as noted in 618353, but the answer is not to add that ugly inset border pixel.
What appears to be happening here is we're hitting the maximum complexity check in SubtractFromVisibleRegion by adding more content to the view. There's a maximum number of rects allowed for the visible region equal to 15, if we grow beyond that subtract operations are skipped.

Adding the status panel trigger this. It can be triggered with other ui elements (like the add-ons bar) as well. Once we hit that limit anything added or subtracted from the view causes the glass margins to jump around because elements such as toolbars start getting skipped from the subtract operation.

For common ui layouts (nav bar, favorites bar, content) upping the limit addresses the problem. But for users that add additional ui elements (toolbars or bringing up the side bar) you hit the limit early enough to trigger this bug again.

The problem in general here is this limit on complexity in the visible region. For calculating glass we really need accurate information.

We could try tracking two different visible regions, a real one and the optimized one, and use the real region for glass margins. Or we could up that complexity check to say 30 rects or so to avoid the most common situations.

Roc, tn, any ideas here?
cc'ing roc!
There's no point in tracking both, the purpose of the complexity limit is performance.

Another option would be to track the largest single opaque display item (in device pixels) and use that to derive the glass margins. That wouldn't normally be affected by chrome changes. You'd have to account for wrapped display items though, so it's not trivial. The "best" way to do it would be to make yet another change to the signature of ComputeVisibility.
(Maybe it should take a state object)
(In reply to comment #20)
> There's no point in tracking both, the purpose of the complexity limit is
> performance.
> 
> Another option would be to track the largest single opaque display item (in
> device pixels) and use that to derive the glass margins. That wouldn't normally
> be affected by chrome changes. You'd have to account for wrapped display items
> though, so it's not trivial. The "best" way to do it would be to make yet
> another change to the signature of ComputeVisibility.

Ok, looks like there is no easy fix. I'll work on this for 5.0.
Assignee: nobody → jmathies
That would always make our aero glaze effect be too low as bug 623092 is about, if we are okay with that.
(In reply to comment #23)
> That would always make our aero glaze effect be too low as bug 623092 is about,
> if we are okay with that.

Not sure I follow you on that. It's currently too low as it is. For example, with the favorites and nav bar visible, glass is inset down to the top of the favorites bar (~84px). If we inset glass down to the bottom of the nav bar outer curve (~48px) the haze rises, and would more closely match other windows with similar glass headers. That's what (I think!) roc was suggesting, and what I'd like to try and get working.

(The status panel issue makes the current situation worse by lowering the inset down to content, pushing the haze farther down.)
First rev. This works well. The overhead here is small since opaqueRegion never grows beyond one rect. (I think I could just use a nsRect there, but I don't believe the overhead is any different.) This ignores any display list item that isn't 100% opaque, which made this easy to calculate based on the IsComplex() test.

Roc, tn, thoughts?
Fixes a bug in widget when plugins are scrolled out of view (the incoming region still needed to be clipped by the client bounds) and rev. the widget interface.
Attachment #516664 - Attachment is obsolete: true
Attachment #516679 - Flags: feedback?(roc)
+  bool res = ComputeVisibilityForSublist(aBuilder,

PRBool

@@ -1648,16 +1670,17 @@ PRBool nsDisplayOpacity::ComputeVisibili
   visibleUnderChildren.And(*aVisibleRegion, bounds);
   // do not pass up the aContainsRootContentDocBG value because anything under
   // us is not opaque
   PRBool notUsed;
   nsRect allowExpansion;
   allowExpansion.IntersectRect(bounds, aAllowVisibleRegionExpansion);
   return
     nsDisplayWrapList::ComputeVisibility(aBuilder, &visibleUnderChildren,
+                                         aOpaqueRegion,
                                          allowExpansion, notUsed);

You're letting stuff inside a translucent group add to the opaque region, surely you don't want to do that?

 PRBool nsDisplayClip::ComputeVisibility(nsDisplayListBuilder* aBuilder,
                                         nsRegion* aVisibleRegion,
+                                        nsRegion* aOpaqueRegion,
                                         const nsRect& aAllowVisibleRegionExpansion,
                                         PRBool& aContainsRootContentDocBG) {
   nsRegion clipped;
   clipped.And(*aVisibleRegion, mClip);
 
   nsRegion finalClipped(clipped);
   nsRect allowExpansion;
   allowExpansion.IntersectRect(mClip, aAllowVisibleRegionExpansion);
   PRBool anyVisible =
     nsDisplayWrapList::ComputeVisibility(aBuilder, &finalClipped,
+                                         aOpaqueRegion,

Similar issue here. And for nsDisplayClipRoundedRect. nsDisplayZoom and nsDisplayTransform also need to do something. And nsDisplaySVGEffects.

I was thinking we wouldn't have a region here but just a rect, and keep picking the largest rect or something like that. If we allow all opaque stuff to accumulate in a region then it's just like the inverse of the visible region, with all the complexity problems that an unrestricted visible region would have.
(In reply to comment #27)
> +  bool res = ComputeVisibilityForSublist(aBuilder,
> 
> PRBool
> 
> @@ -1648,16 +1670,17 @@ PRBool nsDisplayOpacity::ComputeVisibili
>    visibleUnderChildren.And(*aVisibleRegion, bounds);
>    // do not pass up the aContainsRootContentDocBG value because anything under
>    // us is not opaque
>    PRBool notUsed;
>    nsRect allowExpansion;
>    allowExpansion.IntersectRect(bounds, aAllowVisibleRegionExpansion);
>    return
>      nsDisplayWrapList::ComputeVisibility(aBuilder, &visibleUnderChildren,
> +                                         aOpaqueRegion,
>                                           allowExpansion, notUsed);
> 
> You're letting stuff inside a translucent group add to the opaque region,
> surely you don't want to do that?

Hmm, well, this would call mList.ComputeVisibilityForSublist(..) which would run across the items and call TreatAsOpaque on each, which calls aItem->GetOpaqueRegion() to get the opaque data. I think that would result in no additions. (I'll step through it to see. I was wondering about the behavior of some of these item types.)

> 
> I was thinking we wouldn't have a region here but just a rect, and keep picking
> the largest rect or something like that. If we allow all opaque stuff to
> accumulate in a region then it's just like the inverse of the visible region,
> with all the complexity problems that an unrestricted visible region would
> have.

I started with a region, thought about making it rect but didn't go that far. I'll go ahead and make that change. From debugging though the number of rects here never builds beyond one or two for Fx's main window since we ignore complex opaque regions. What you get is a set of rects that either stack or set next to each other. I think the overhead of either method is about equal. (The only time I ended up with more than one rect was with a plugin interestingly enough.)
(In reply to comment #28)
> Hmm, well, this would call mList.ComputeVisibilityForSublist(..) which would
> run across the items and call TreatAsOpaque on each, which calls
> aItem->GetOpaqueRegion() to get the opaque data. I think that would result in
> no additions. (I'll step through it to see. I was wondering about the behavior
> of some of these item types.)

aItem->GetOpaqueRegion in an nsDisplayOpacity container can definitely return something non-empty.

> I started with a region, thought about making it rect but didn't go that far.
> I'll go ahead and make that change. From debugging though the number of rects
> here never builds beyond one or two for Fx's main window since we ignore
> complex opaque regions. What you get is a set of rects that either stack or set
> next to each other. I think the overhead of either method is about equal. (The
> only time I ended up with more than one rect was with a plugin interestingly
> enough.)

Hmm OK.
(In reply to comment #29)
> (In reply to comment #28)
> > Hmm, well, this would call mList.ComputeVisibilityForSublist(..) which would
> > run across the items and call TreatAsOpaque on each, which calls
> > aItem->GetOpaqueRegion() to get the opaque data. I think that would result in
> > no additions. (I'll step through it to see. I was wondering about the behavior
> > of some of these item types.)
> 
> aItem->GetOpaqueRegion in an nsDisplayOpacity container can definitely return
> something non-empty.

Ok I see the issue. In the current patch, ComputeVisibility is passing the aOpaqueRect down.

I went through each of these noting the behavior for ComputeVisibility and GetOpaqueRegion. Here's the results:



nsDisplaySolidColor
 ComputeVisibility - aOpaqueRect not altered
 GetOpaqueRegion - empty region

nsDisplayBackground
 ComputeVisibility - aOpaqueRect not altered
 GetOpaqueRegion - returns valid region for TreatAsOpaque

nsDisplayOutline
 ComputeVisibility - aOpaqueRect not altered
 GetOpaqueRegion - empty region
 (should modify aOpaqueRect? probably not needed.)

nsDisplayCaret
 ComputeVisibility - aOpaqueRect not altered
 GetOpaqueRegion - empty region
 (not opaque)

nsDisplayBorder
 ComputeVisibility - aOpaqueRect not altered
 GetOpaqueRegion - empty region
 (should modify aOpaqueRect? probably not needed.)

nsDisplayBoxShadowOuter
 ComputeVisibility - aOpaqueRect not altered
 GetOpaqueRegion - empty region
 (not opaque)

nsDisplayBoxShadowInner
 ComputeVisibility - aOpaqueRect not altered
 GetOpaqueRegion - empty region
 (not opaque)

nsDisplayWrapList
 ComputeVisibility - calls mList.ComputeVisibilityForSublist, pass in aOpaqueRect
 GetOpaqueRegion - if mList.IsOpaque(), returns bounds

nsDisplayOpacity
 ComputeVisibility - pass dummy rect to ComputeVisibilityForSublist, aOpaqueRect should not be altered
 GetOpaqueRegion - empty region

nsDisplayOwnLayer
 ComputeVisibility - calls mList.ComputeVisibilityForSublist, pass in aOpaqueRect
 GetOpaqueRegion - if mList.IsOpaque(), returns bounds

nsDisplayScrollLayer
 ComputeVisibility - calls mList.ComputeVisibilityForSublist, pass in aOpaqueRect
 GetOpaqueRegion - if mList.IsOpaque(), returns bounds

nsDisplayClip
 ComputeVisibility - calls ComputeVisibilityForSublist, clip result to mClip and add to aOpaqueRect
 GetOpaqueRegion - if mList.IsOpaque(), returns bounds

nsDisplayClipRoundedRect
 ComputeVisibility - calls ComputeVisibilityForSublist, clip result to mClip and add to aOpaqueRect
 GetOpaqueRegion - if mList.IsOpaque(), returns bounds

nsDisplayZoom
 ComputeVisibility - calls ComputeVisibilityForSublist with temp rect, add temp to aOpaqueRect w/conversion
 GetOpaqueRegion - if mList.IsOpaque(), returns bounds

nsDisplayTransform
 ComputeVisibility - aOpaqueRect should not altered
 GetOpaqueRegion - returns valid region

nsDisplaySVGEffects
 ComputeVisibility - calls mList.ComputeVisibilityForSublist, pass in aOpaqueRect
 GetOpaqueRegion - empty region

I'll post an updated patch based on this. Please let me know if you spot any errors above.
Attached file notes attached as text file (obsolete) —
New rev based on the list I posted.
Attachment #516679 - Attachment is obsolete: true
Attachment #516679 - Flags: feedback?(roc)
Attached file notes attached as text file (updated) (obsolete) —
Attachment #517005 - Attachment is obsolete: true
One of the types I'm not sure on is nsDisplaySVGEffects. It has a list it calls ComputeVisibilityForSublist on, but I don't understand yet what type of items would be in that list, so I'm not sure if aOpaqueRect would be set correctly. I'll do some additional debugging this weekend.
SVG effects can do pretty much anything, so I think you just want to ignore the opaque rect that its sublist returns, because the SVG effects item could make them totally transparent.
(In reply to comment #35)
> SVG effects can do pretty much anything, so I think you just want to ignore the
> opaque rect that its sublist returns, because the SVG effects item could make
> them totally transparent.

Updated with those changes and removed some left over debugging code. This also passed a try server run.
Attachment #517028 - Attachment is obsolete: true
Comment on attachment 517148 [details] [diff] [review]
set opaque vs visible region on widgets v.3

Haven't looked at the whole patch, just cherry picking things here.

 PRBool nsDisplayZoom::ComputeVisibility(nsDisplayListBuilder *aBuilder,
+  // Convert opaque rect units from mAPD to mParentAPD and add to
+  // aOpaqueRect.
+  aOpaqueRect->UnionRect(*aOpaqueRect,
+    opaqueRect.ConvertAppUnitsRoundOut(mAPD, mParentAPD));

UnionRect creates a rect that encompasses both passed in rects, which means the resulting rect could contain parts that aren't opaque.
(In reply to comment #37)
> Comment on attachment 517148 [details] [diff] [review]
> set opaque vs visible region on widgets v.3
> 
> Haven't looked at the whole patch, just cherry picking things here.
> 
>  PRBool nsDisplayZoom::ComputeVisibility(nsDisplayListBuilder *aBuilder,
> +  // Convert opaque rect units from mAPD to mParentAPD and add to
> +  // aOpaqueRect.
> +  aOpaqueRect->UnionRect(*aOpaqueRect,
> +    opaqueRect.ConvertAppUnitsRoundOut(mAPD, mParentAPD));
> 
> UnionRect creates a rect that encompasses both passed in rects, which means the
> resulting rect could contain parts that aren't opaque.

Would that ever really happen the way we walk through all the layout items here? I was under the impression we walked the tree from the smallest, inner element outward. In which case you would always be adding to the existing rect or adding sections adjacent to it.

If this isn't the case, I think I would have to go back to using a region and use the final bounding rect. But roc didn't seem to like that idea. I suppose I could do something different, check for intersection first, but if not found, I wouldn't be able to combine them. (Back to the region again.)

FWIW, the way it is now works well with Fx windows.
Maybe instead of trying to do things automagically using the visible region calculations, we should define a new XUL attribute that sets the glass margins to the edges of the given element.
(In reply to comment #37)
> Comment on attachment 517148 [details] [diff] [review]
> set opaque vs visible region on widgets v.3
>
> UnionRect creates a rect that encompasses both passed in rects, which means the
> resulting rect could contain parts that aren't opaque.

I guess I can see this breaking for content floating in glass. Oddly enough though the current fix didn't break the downloads window. I'm curious why that worked.

(In reply to comment #39)
> Maybe instead of trying to do things automagically using the visible region
> calculations, we should define a new XUL attribute that sets the glass margins
> to the edges of the given element.

I'll take a look. I think the biggest concern here is the maintenance of keeping the values up to date for different UI layouts we have in the main window.
Looking at browser.xul as an example, we could add an opaque xul attrib (or potentially a -moz css property?) to elements like

<toolbar id="nav-bar">
<hbox flex="1" id="browser">
<vbox id="browser-bottombox" layer="true">

then track their area and set the glass values based on that.
I think we could just add the attribute to the #browser hbox and be done with it for the browser window.

If someone forgets to add the attribute, they just get the full sheet of glass. That isn't the most performant, but it's fine for most things.
If we went with something like that it means we could back out http://hg.mozilla.org/mozilla-central/rev/c165b3127975 which would simplify things a bit.
Not beeing an expert here, but is this issue really triggered by the maximum complexity check in SubtractFromVisibleRegion?

With two tabs open in Win7/Aero, the effect is marginal - with tree tabs it hits the eye. Do (nonvisible) tabs add to the complexity of the visible region?
Yes, we are talking about the actual tab in the tab bar that is visible.
(In reply to comment #45)
> Yes, we are talking about the actual tab in the tab bar that is visible.

That's why I was wondering about the effect of nonvisible tabs...;)....

BTW, confirming comment #9: On Win7x64/Aero you have the same bug just when
hovering over a link (which, of course, makes the status bar visible). The bug
doesn't depend on whether the toolbar is visible or not. The strength just
depends on the number of tabs (>2=considerable).
Did this get fixed? I don't see it happening again.

Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110305 Firefox/4.0b13pre

I have the same build as yesterday so I wouldn't see why.
I have installed several Window Updates in the last day.

I have also been able to notice the situation where one window would have the problem and not a new one.

I have also tried to see if it was related to pages with horizontal scroll bar and/or vertical scroll bar without any discoveries.
Armen, as Martin Conrad mentioned this bug seems to be triggered by having a few tabs open (which presumably increases the complexity somehow). With one tab open the glaze does shift a bit, but only by a few pixels. Once complexity gets high enough the effect is distracting.
I just did some testing concerning the visibility (complexity?) of bars and found out that the bug is actually linked to the distinct visibility of bars/tabs.
Here is a list of combinations I've tested:

navigation bar alone => tab count < 3 = slightly buggy, >= 3 = buggy
navigation bar + any other bar(s) => always buggy
menu bar alone => tab count < 5 = slightly buggy, >5 = OK!
menu bar + bookmark bar => tab count < 3 = slightly buggy, =< 3 = buggy, > 5 = OK!

Does somebody see a pattern here? Not yours truly!
(In reply to comment #48)
> Armen, as Martin Conrad mentioned this bug seems to be triggered by having a
> few tabs open (which presumably increases the complexity somehow). With one tab
> open the glaze does shift a bit, but only by a few pixels. Once complexity gets
> high enough the effect is distracting.

I have read it. I can't reproduce anymore even with all the different combinations of bars.
(In reply to comment #42)
> I think we could just add the attribute to the #browser hbox and be done with
> it for the browser window.
> 
> If someone forgets to add the attribute, they just get the full sheet of glass.
> That isn't the most performant, but it's fine for most things.

Roc, where is the appropriate place in the code to collect this data and set it on the widget? Would we do this during the display list processing or someplace else?
Armen, I just installed the latest nightly (see below) and the bug is still there (albeit the combination matrix has changed slightly).

If you were interested, I could post some screenshots.

Here's what I'm running: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.0b13pre) Gecko/20110309 Firefox/4.0b13pre (latest graphics driver and OS patches installed).
Blocks: 640226
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/cd52999e29c7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre ID:20110209161954
Fails;
http://hg.mozilla.org/mozilla-central/rev/fc119d5000e5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre ID:20110209175810
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cd52999e29c7&tochange=fc119d5000e5
In local build,
build from f2a249b46e43 : fails
build from cd52999e29c7 : works
Triggered by:
f2a249b46e43	Dão Gottwald — Bug 631698 - remove bogus 1px border below active app tabs (w/ tab overflow, Aero Glass, no DirectWrite). r=fryn a=dolske
Blocks: 631698
Keywords: regression
Keywords: regression
No longer blocks: 640226
FWIW, I didn't mean comment 37 to be fatal to that patch. Just pointing out a potential problem.
I ended up on this page because I noticed this same issue and found it already reported. For me, it varies with number of tabs, and varies with "add-on bar" showing or not.

But I also noticed another affect. When scrolling on my iGoogle page, the white glaze over will disappear completely for a portion of the page and reappear as I continue to scroll. If I stop while it has disappeared, I can move to a second tab and it is still gone. After opening and closing various numbers of tabs, I couldn't notice a definitive pattern. However, sometimes new tabs would bring the white overlay back, but returning to the iGoogle page would make it disappear again for that tab. Also, while a page is loading or hovering a link can make it disappear and reappear.

Also, if you grab the top of the window to resize, it isn't very graceful in shrinking. When the window is something like 150 px tall, it disappears, and reappears briefly and disappears again as you continue to reduce the size of the window.

It seems this glaze affect should be based on the total height of the whole window, not based on the internal elements of the window, which seems to be why it might disappear and reappear as the window is resized down.
For my all systems, it varies with the dimensions of browser-chrome (Opening/Closing a tab also changes it when they are on top). It also varies by a few pixels on hovering at a url. According to the Windows Aero UI guideline, the position of glaze effect should be based on the height of the window.
I also agree. This issue is annoying and distracting. However, on an additional note, I find it looks cheap, childish and broken. Is there a way this can be turned off totally?
Is there any theme or persona we can use that will at least hide this behavior? If not, I'm going to test reverting my profile back to FF3.X to make sure nothing goes crazy and report back.
Is it a coincidence that this bug disappears if you turn off the add-on bar and the height of the "jumping" is exactly the same height as the add-on bar?
It's as if chrome believes that showing the add-on bar adds vertical height to the window, but only when displaying the status info overlay. Somewhere, something is adding the height of the add-on bar to some calculation where it shouldn't be added.
We understand what's going on here, and we intend to get this fixed. Please try to keep the bug chatter to a minimum. Thank you!
Whiteboard: [see comment 71]
(In reply to comment #71)
> We understand what's going on here, and we intend to get this fixed. Please try
> to keep the bug chatter to a minimum. Thank you!

Sorry. The last exchanges other than "me too" still seemed like people weren't even sure how to reproduce it or what caused it.
Jim, do you think you'll be able to do comment #39 for FF5?
(In reply to comment #73)
> Jim, do you think you'll be able to do comment #39 for FF5?

Yes, I'll be working on this next week.
On a side note, I can't reproduce this bug after installing this extension which customizes the default theme (https://addons.mozilla.org/en-US/firefox/addon/flow/).
I'm not going to be able to get to this this week as I'm sidetracked on other 5.0 work. If someone would like to pick this up feel free to do so, otherwise I'll try to get to it during the work week.
I experience this when I have three or more tabs opened in the window:

http://i.imgur.com/TuJFl.png (1. Start Firefox, open two more tabs and go back to
                              the first <about:home> page;)
http://i.imgur.com/tffY6.png (2. Hover over the "About Mozilla" link.)

When I close one of the additional tabs the effect is either not visible or negligible.
Attached patch backout of c165b3127975 (obsolete) — Splinter Review
The merge on this was a complete failure due to async plugin painting changes, so I backed it manually.
Attachment #517033 - Attachment is obsolete: true
Attachment #517148 - Attachment is obsolete: true
Attached patch layout changes v.1 (obsolete) — Splinter Review
Attached patch widget changes v.1 (obsolete) — Splinter Review
Attached patch browser xul changes v.1 (obsolete) — Splinter Review
I ended up creating a new nsDisplayOpaqueFrame display item for this, hope that was the appropriate way to go. I filter this out in ComputeVisibility when we are walking over the list. So far I've only had to add one of these in nsBoxFrame and a couple attribs on some box's in the browser.xul. From some initial testing it looks to have fixed all our haze problems. I'll do some more testing and a try run before seeking reviews.
This looks reasonable. Having to support multiple isopaque items is a bit of a pain, but I guess it's the only way without restructure browser.xul.
(In reply to comment #86)
> This looks reasonable. Having to support multiple isopaque items is a bit of a
> pain, but I guess it's the only way without restructure browser.xul.

I should be able to wrap both of those up in a single vertical box so we only need one element with the attribute. I'll test and see.
That might break extensions and stuff. It's needless churn in a possibly-sensitive area at least. It's probably better to do what you've done here.
Comment on attachment 527099 [details] [diff] [review]
browser xul changes v.1

>-  <hbox flex="1" id="browser">
>+  <hbox flex="1" id="browser" isopaque="true">

The attribute name seems a bit confusing since this thing is in fact not opaque.
(In reply to comment #89)
> Comment on attachment 527099 [details] [diff] [review]
> browser xul changes v.1
> 
> >-  <hbox flex="1" id="browser">
> >+  <hbox flex="1" id="browser" isopaque="true">
> 
> The attribute name seems a bit confusing since this thing is in fact not
> opaque.

How about using something specific to glass like glass='false'?
(In reply to comment #90)
> (In reply to comment #89)
> > Comment on attachment 527099 [details] [diff] [review]
> > browser xul changes v.1
> > 
> > >-  <hbox flex="1" id="browser">
> > >+  <hbox flex="1" id="browser" isopaque="true">
> > 
> > The attribute name seems a bit confusing since this thing is in fact not
> > opaque.
> 
> How about using something specific to glass like glass='false'?

Not to argue against something specific to glass, but just 'glass' still seems ambiguous if not confusing, since you get and actually see glass on the transparent parts.
(In reply to comment #91)
> (In reply to comment #90)
> > (In reply to comment #89)
> > > Comment on attachment 527099 [details] [diff] [review]
> > > browser xul changes v.1
> > > 
> > > >-  <hbox flex="1" id="browser">
> > > >+  <hbox flex="1" id="browser" isopaque="true">
> > > 
> > > The attribute name seems a bit confusing since this thing is in fact not
> > > opaque.
> > 
> > How about using something specific to glass like glass='false'?
> 
> Not to argue against something specific to glass, but just 'glass' still seems
> ambiguous if not confusing, since you get and actually see glass on the
> transparent parts.

I was thinking the same thing, how about a verb: limitglass="true". I like that one.
Attached patch layout changes v.2 (obsolete) — Splinter Review
Switches out 'isopaque' for 'limitglass', and fixes a leak in display list processing caused by a missing AppendToBottom(item) call when we detect a TYPE_OPAQUE_FRAME.
Attachment #527097 - Attachment is obsolete: true
Attached patch browser xul changes v.2 (obsolete) — Splinter Review
I honestly have a hard time making sense of 'limitglass' even after following this bug. I'll make it concrete: browser-bottombox has rounded corners, so you see glass there. Is there a way to end "The bottom box limits glass ..." in a meaningful way? It surely covers the glass behind it in a large part, but then, so do the toolbars at the top.
the original was out of date when I posted it. fixes a compile error.
Attachment #527098 - Attachment is obsolete: true
(In reply to comment #95)
> I honestly have a hard time making sense of 'limitglass' even after following
> this bug. I'll make it concrete: browser-bottombox has rounded corners, so you
> see glass there. Is there a way to end "The bottom box limits glass ..." in a
> meaningful way? It surely covers the glass behind it in a large part, but then,
> so do the toolbars at the top.

Oh, hmm, odd since the glass looked just fine, we do some adjusting down in widget that may have been covering for the rounded corners. The simple solution there is to remove the attribute from mark browser-bottombox. Glass margins will then be set to the top of that element when it's visible. Moving the boundaries around really isn't an issue, we just need them to be set statically vs. calculating them on the fly.
Attached patch browser xul changes v.3 (obsolete) — Splinter Review
Removed the bottom box attribute. This causes the haze to move when you enable/disable it from the view, but I'd guess users either have this on or off, so I don't think this is an issue.
Attachment #527099 - Attachment is obsolete: true
Attachment #527286 - Attachment is obsolete: true
Attached patch downloads changes v.1 (obsolete) — Splinter Review
We use borderless glass in the downloads window so I've added this to the download window's list view. Without it the haze disappears.
Attachment #527096 - Flags: review?(tnikkel)
Attachment #527284 - Flags: review?(roc)
Comment on attachment 527287 [details] [diff] [review]
widget changes v.2

sorry roc, one more but it's pretty simple.
Attachment #527287 - Flags: review?(roc)
Comment on attachment 527300 [details] [diff] [review]
browser xul changes v.3

Dao, we can change up the attribute name again but please make suggestions if you prefer something different.
Attachment #527300 - Flags: review?(dao)
Comment on attachment 527302 [details] [diff] [review]
downloads changes v.1

downloads.xul -> Shawn
Attachment #527302 - Flags: review?(sdwilsh)
(In reply to comment #99)
> Created attachment 527302 [details] [diff] [review]
> downloads changes v.1
> 
> We use borderless glass in the downloads window

We actually don't, it's -moz-win-glass. Is this going to break without the attribute?
(In reply to comment #103)
> (In reply to comment #99)
> > Created attachment 527302 [details] [diff] [review]
> > downloads changes v.1
> > 
> > We use borderless glass in the downloads window
> 
> We actually don't, it's -moz-win-glass. Is this going to break without the
> attribute?

Ah, same situation though. Nothing breaks. Without the attribute, the opaque region for the window is empty, which results in widget setting negative glass margins rendering the entire window as glass. When this is set Windows removes the glass haze from the sides of the window. Adding the attribute to the list sets the margin bounds to the edge of the list view, bringing back the haze.
"rendering the entire window as glass" sounds like the inner glass border is going to be missing, which seems like significant breakage. The attribute shouldn't become a requirement, since content and theme code isn't necessarily authored by the same people.
(In reply to comment #105)
> "rendering the entire window as glass" sounds like the inner glass border is
> going to be missing, which seems like significant breakage. The attribute
> shouldn't become a requirement, since content and theme code isn't necessarily
> authored by the same people.

Yes it does unless you set the attribute properly. If we want a default fallback we can pass both the visible region and the opaque region down to widget and have it use the opaque region only when it's not empty.

(In which case we might want to keep c165b3127975? I think tn really wanted to back that out though so maybe we could live without it.)
I think 'excludeglass' would be a better name than 'limitglass'.

(In reply to comment #105)
> "rendering the entire window as glass" sounds like the inner glass border is
> going to be missing, which seems like significant breakage. The attribute
> shouldn't become a requirement, since content and theme code isn't necessarily
> authored by the same people.

I think it really has to be a requirement. The content author has to use the attribute so we can get the haze in a consistent place, i.e., fix this bug. Going with a fallback approach just means this bug will reappear.

The attribute is harmless for themes that don't use glass. If authors don't use it, they'll get the "full sheet of glass" which is also OK in most situations (some performance issues for large windows on slow machines).

If you're really worried about the content/theme separation, we could make it a CSS property I guess. Possibly even a new -moz-appearance value.
(In reply to comment #107)
> If you're really worried about the content/theme separation, we could make it a
> CSS property I guess. Possibly even a new -moz-appearance value.

+1

We need to start telling people to do this whenever they use -moz-win(-borderless)-glass. Third-party themes have no sane way to set the attribute in random Firefox/Thunderbird/Songbird/add-on/... windows.
Seems to me it would make just as much sense for the content authors to set the attribute, and for theme authors to only use -glass styles on windows which have the attribute. That would mean theme authors could only theme arbitrary "unfriendly" windows with the "full sheet of glass", but is that really a big deal?
I don't think it's sane to expect everyone creating a xul window to take this into account. Also, it's not clear that content can cater for different themes: we want glass behind the add-on bar, but native apps exclude the status bar; we want glass behind toolbars, IE9 doesn't.
(In reply to comment #110)
> I don't think it's sane to expect everyone creating a xul window to take this
> into account.

They wouldn't have to. Themers would simply have to choose between giving such a window a full sheet of glass, or no glass at all.

> Also, it's not clear that content can cater for different themes:
> we want glass behind the add-on bar, but native apps exclude the status bar; we
> want glass behind toolbars, IE9 doesn't.

I can see that we might possibly want that flexibility, but I wonder if it's worth introducing a new CSS property to get it.
(In reply to comment #111)
> (In reply to comment #110)
> > I don't think it's sane to expect everyone creating a xul window to take this
> > into account.
> 
> They wouldn't have to. Themers would simply have to choose between giving such
> a window a full sheet of glass, or no glass at all.

Neither of these choices are good if you're aiming for a native appearance.
Comment on attachment 527302 [details] [diff] [review]
downloads changes v.1

r=sdwilsh (holds try for whatever you end up needing to do for this per discussion in the bug)
Attachment #527302 - Flags: review?(sdwilsh) → review+
Comment on attachment 527096 [details] [diff] [review]
backout of c165b3127975

Can you also remove the ROOT_CONTENT_DOC_BG flag from AddCanvasBackgroundColorItem in nsIPresShell.h? It should be unused now. When nsSubDocumentFrame calls AddCanvasBackgroundColorItem it doesn't need to pass this flag anymore.

Otherwise looks good, thanks.
Blocks: 647555
(In reply to comment #110)
> I don't think it's sane to expect everyone creating a xul window to take this
> into account. Also, it's not clear that content can cater for different themes:
> we want glass behind the add-on bar, but native apps exclude the status bar; we
> want glass behind toolbars, IE9 doesn't.

Well everyone wouldn't, just the people using -moz-win(-borderless)-glass css values to get glass areas in their client ui would need it. 

We've spent an immense amount of energy trying to get the current way we do this working and still don't have a perfect solution. The layout visibility data doesn't lend itself to these calculations. This fix solves the problem cleanly. 

(In reply to comment #108)
> (In reply to comment #107)
> > If you're really worried about the content/theme separation, we could make it a
> > CSS property I guess. Possibly even a new -moz-appearance value.
> 
> +1

I'll look into this to ease the pain for theme authors. Hopfully I have access to this info down in nsBoxFrame where we set this up.
(In reply to comment #115)
> (In reply to comment #110)
> > I don't think it's sane to expect everyone creating a xul window to take this
> > into account. Also, it's not clear that content can cater for different themes:
> > we want glass behind the add-on bar, but native apps exclude the status bar; we
> > want glass behind toolbars, IE9 doesn't.
> 
> Well everyone wouldn't, just the people using -moz-win(-borderless)-glass css
> values to get glass areas in their client ui would need it. 

These are different sets of people. Somebody adding a XUL window to Firefox will have a hard time predicting and catering for different theme approaches.
This is undoubtedly the single most glaring UI issue in Firefox on Aero Glass systems. Is it possible to fix Firefox and worry about third parties at some point in the future? 

Also, what is the fall-out for third parties not getting this right? If it's just that they suffer from the kinds of problems we're suffering from today, I can't see how that could possibly warrant not band-aiding Firefox today and working for a better solution for them at some point in the future.
(In reply to comment #117)
> This is undoubtedly the single most glaring UI issue in Firefox on Aero Glass
> systems. Is it possible to fix Firefox and worry about third parties at some
> point in the future? 
I agree; we shouldn't let perfect be the enemy of good here.  The jumpiness is *really* annoying on Windows.  Fixing this with a CSS property seems like a good candidate for a follow-up.
The regression this would cause isn't "good". And no, it's not that they'd just suffer from the kinds of problems we're suffering from today.
(In reply to comment #119)
> The regression this would cause isn't "good". And no, it's not that they'd just
> suffer from the kinds of problems we're suffering from today.

Aside from losing the default Windows 2px black/grey client area border, what are the big risks here? The only serious regression this could cause would be the case where a 3rd party was creating a xul window that hosted a windowed plugin. That seems pretty edgy to me, and obviously there would be simple fix.
(In reply to comment #118)
> (In reply to comment #117)
> > This is undoubtedly the single most glaring UI issue in Firefox on Aero Glass
> > systems. Is it possible to fix Firefox and worry about third parties at some
> > point in the future? 
> I agree; we shouldn't let perfect be the enemy of good here.  The jumpiness is
> *really* annoying on Windows.  Fixing this with a CSS property seems like a
> good candidate for a follow-up.

Just guessing but it looks like I can access moz-appearance data from the nsStyleContext in CreateViewForFrame, so this might not be too hard of a change to make. I'm looking at this now.
(In reply to comment #121)
> (In reply to comment #118)
> > (In reply to comment #117)
> > > This is undoubtedly the single most glaring UI issue in Firefox on Aero Glass
> > > systems. Is it possible to fix Firefox and worry about third parties at some
> > > point in the future? 
> > I agree; we shouldn't let perfect be the enemy of good here.  The jumpiness is
> > *really* annoying on Windows.  Fixing this with a CSS property seems like a
> > good candidate for a follow-up.
> 
> Just guessing but it looks like I can access moz-appearance data from the
> nsStyleContext in CreateViewForFrame, so this might not be too hard of a change
> to make. I'm looking at this now.

Looks like it's passed to the ctor too. Even better.
- removed ROOT_CONTENT_DOC_BG
Attachment #527096 - Attachment is obsolete: true
Attachment #528170 - Flags: review?(tnikkel)
Attachment #527096 - Flags: review?(tnikkel)
Attached patch layout changes v.3 (obsolete) — Splinter Review
Swapped the attrib for the moz-appearance style with prop '-moz-exclude-glass'
Attachment #527284 - Attachment is obsolete: true
Attachment #528172 - Flags: review?(roc)
Attachment #527284 - Flags: review?(roc)
Attached patch browser css v.1 (obsolete) — Splinter Review
Attachment #527300 - Attachment is obsolete: true
Attachment #528173 - Flags: review?(dao)
Attachment #527300 - Flags: review?(dao)
last but not least, downloads css changes.
Attachment #527302 - Attachment is obsolete: true
Attachment #528175 - Flags: review?(sdwilsh)
(In reply to comment #126)
> Created attachment 528175 [details] [diff] [review]
> downloads css v.1
> 
> last but not least, downloads css changes.

oops. I've fixed that white space change in the other file locally.
Comment on attachment 528175 [details] [diff] [review]
downloads css v.1

>+++ b/toolkit/mozapps/downloads/content/downloads.css
>@@ -38,9 +38,8 @@ richlistitem[type="download"][state="8"]
> richlistitem[type="download"][state="9"] {
>   -moz-binding: url('chrome://mozapps/content/downloads/download.xml#download-blocked-policy');
> }
> 
> /* Only focus buttons in the selected item*/
> richlistitem[type="download"]:not([selected="true"]) button {
>   -moz-user-focus: none;
> }
>-
I suspect this is a spurious change...

>+++ b/toolkit/themes/winstripe/mozapps/downloads/downloads-aero.css
>   #downloadView {
>+    /* clamp glass bounds to the rich list so our glass haze stays constant. */
nit: "Clamp" instead of "clamp" please

r=sdwilsh
Attachment #528175 - Flags: review?(sdwilsh) → review+
I think we want -moz-win for consistency.
Besides, -moz-appearance values should probably be nouns / noun phrases... but I don't have a good suggestion.
Comment on attachment 528175 [details] [diff] [review]
downloads css v.1

>   #downloadView {
>+    /* clamp glass bounds to the rich list so our glass haze stays constant. */

More to the point, there would be no glass haze (nor an inset border) without this.

>+    -moz-appearance: -moz-exclude-glass;
toolkit/themes/winstripe/mozapps/extensions/about.css uses -moz-win-glass as well.
(In reply to comment #129)
> I think we want -moz-win for consistency.
> Besides, -moz-appearance values should probably be nouns / noun phrases... but
> I don't have a good suggestion.

Yep, that seems to fit with the other values. (I assume that's "win" == "Windows" and not "window".) I've updated this locally.

(In reply to comment #130)
> Comment on attachment 528175 [details] [diff] [review]
> downloads css v.1
> 
> >   #downloadView {
> >+    /* clamp glass bounds to the rich list so our glass haze stays constant. */
> 
> More to the point, there would be no glass haze (nor an inset border) without
> this.
> 
> >+    -moz-appearance: -moz-exclude-glass;

True, but it's not a severe regression unless there's plugin content in the window. Plus, this will be going out with a major release, so it's not like we're springing this on anyone without time to update.

(In reply to comment #131)
> toolkit/themes/winstripe/mozapps/extensions/about.css uses -moz-win-glass as
> well.

I'll take a look. This doesn't look like css that Fx currently makes use of. Maybe it's thunderbird? I'll try to track this down so I can test before posting a patch here or in a follow up bug.
Attachment #528170 - Flags: review?(tnikkel) → review+
> > >   #downloadView {
> > >+    /* clamp glass bounds to the rich list so our glass haze stays constant. */
> > 
> > More to the point, there would be no glass haze (nor an inset border) without
> > this.
> > 
> > >+    -moz-appearance: -moz-exclude-glass;
> 
> True, but it's not a severe regression unless there's plugin content in the
> window. Plus, this will be going out with a major release, so it's not like
> we're springing this on anyone without time to update.

All I'm saying is, the comment in your patch is misleading.

> > toolkit/themes/winstripe/mozapps/extensions/about.css uses -moz-win-glass as
> > well.
> 
> I'll take a look. This doesn't look like css that Fx currently makes use of.
> Maybe it's thunderbird? I'll try to track this down so I can test before
> posting a patch here or in a follow up bug.

You need to right click on an add-on to open the about dialog.
(In reply to comment #133)
> > True, but it's not a severe regression unless there's plugin content in the
> > window. Plus, this will be going out with a major release, so it's not like
> > we're springing this on anyone without time to update.
> 
> All I'm saying is, the comment in your patch is misleading.

I think we can address the specifics of what this 'thing' does in dev docs. If the comment is really misleading for people I'd say we just take it out.

> 
> You need to right click on an add-on to open the about dialog.

Ah, thanks. I've updated locally, will post a patch and seek a review here in a bit.
Blocks: 622770, 623092, 637018
Comment on attachment 528172 [details] [diff] [review]
layout changes v.3

Review of attachment 528172 [details] [diff] [review]:

I don't think aOpaqueRegion is the right name here. It's not just opaque (we know about other opaque regions), it's really limit-glass.

I still wish we could get away without modifying ComputeVisibility for all display items. What if we just accumulated the limit-glass region for the root display list only? Seems like that would work for the cases we care about.

::: layout/base/nsDisplayList.h
@@ +1599,5 @@
   NS_DISPLAY_DECL_NAME("Outline", TYPE_OUTLINE)
 };
 
 /**
+ * A display item used in tracking frames marked as opaque via a xul

We're actually not using a xul attribute anymore; please fix comment.
Attached patch layout changes v.4 (obsolete) — Splinter Review
Modified to use moz-win-exclude-glass.
Attachment #528172 - Attachment is obsolete: true
Attachment #528211 - Flags: review?(roc)
Attachment #528172 - Flags: review?(roc)
Attached patch browser css v.2 (obsolete) — Splinter Review
Attachment #528173 - Attachment is obsolete: true
Attachment #528212 - Flags: review?(dao)
Attachment #528173 - Flags: review?(dao)
Attached patch ext. about css v.1 (obsolete) — Splinter Review
Attachment #528213 - Flags: review?(dtownsend)
(In reply to comment #136)
> Comment on Review of attachment 528172 [details] [diff] [review]:
> layout changes v.3
> 
> attachment 528172 [details] [diff] [review]
> 
> I don't think aOpaqueRegion is the right name here. It's not just opaque (we
> know about other opaque regions), it's really limit-glass.

Now that we've settled on a name I suppose we can be more specific:

nsDisplayOpaqueFrame
aExcludeGlassRegion

if that works for you.

> I still wish we could get away without modifying ComputeVisibility for all
> display items. What if we just accumulated the limit-glass region for the root
> display list only? Seems like that would work for the cases we care about.

I'll test to see if this picks up our current usage. I've never fully understood the ordering of the list - it's always seemed somewhat random.
Would checking the root pick up any css props set across all content? Seems like we would end up with blind spots where it just didn't work, and devs wouldn't understand why.

> 
> ::: layout/base/nsDisplayList.h
> @@ +1599,5 @@
>    NS_DISPLAY_DECL_NAME("Outline", TYPE_OUTLINE)
>  };
> 
>  /**
> + * A display item used in tracking frames marked as opaque via a xul
> 
> We're actually not using a xul attribute anymore; please fix comment.

will do.
(In reply to comment #140)
> (In reply to comment #136)
> > I still wish we could get away without modifying ComputeVisibility for all
> > display items. What if we just accumulated the limit-glass region for the root
> > display list only? Seems like that would work for the cases we care about.
> 
> I'll test to see if this picks up our current usage. I've never fully
> understood the ordering of the list - it's always seemed somewhat random.
> Would checking the root pick up any css props set across all content? Seems
> like we would end up with blind spots where it just didn't work, and devs
> wouldn't understand why.

The implications for devs are that -moz-appearance:moz-win-exclude-glass would not work if the element has an ancestor with opacity < 1, or a CSS transform, or an SVG filter, mask or clip-path, or is clipped. But then, it *shouldn't* work in most of those cases, except for transforms.
Attachment #528213 - Flags: review?(dtownsend) → review+
(In reply to comment #141)
> (In reply to comment #140)
> > (In reply to comment #136)
> > > I still wish we could get away without modifying ComputeVisibility for all
> > > display items. What if we just accumulated the limit-glass region for the root
> > > display list only? Seems like that would work for the cases we care about.
> > 
> > I'll test to see if this picks up our current usage. I've never fully
> > understood the ordering of the list - it's always seemed somewhat random.
> > Would checking the root pick up any css props set across all content? Seems
> > like we would end up with blind spots where it just didn't work, and devs
> > wouldn't understand why.
> 
> The implications for devs are that -moz-appearance:moz-win-exclude-glass would
> not work if the element has an ancestor with opacity < 1, or a CSS transform,
> or an SVG filter, mask or clip-path, or is clipped. But then, it *shouldn't*
> work in most of those cases, except for transforms.

Ok, so it sounds like most opaque content will always be in the root. This might be a side case but what if you have a transparent div that contains a windowed plugin?
(In reply to comment #142)
> Ok, so it sounds like most opaque content will always be in the root. This
> might be a side case but what if you have a transparent div that contains a
> windowed plugin?

Actually, not a div but an hbox say. I'm wondering about 3rd party toolbars that embed flash and silverlight.
Those plugin areas won't be in the exclude-glass region, but we'll fix that in nsWindow::SetOpaqueRegion anyway, won't we?
(In reply to comment #144)
> Those plugin areas won't be in the exclude-glass region, but we'll fix that in
> nsWindow::SetOpaqueRegion anyway, won't we?

Yep, good point. Total disconnect there. I'll test limiting things to root tomorrow and post new patches if it works out.
Comment on attachment 528212 [details] [diff] [review]
browser css v.2

> @media all and (-moz-windows-compositor) {
>+  #browser {
>+    -moz-appearance: -moz-win-exclude-glass;
>+  }

This breaks the transparency of #browser-border-start and #browser-border-end, both of which are children of #browser. I think you can use #appcontent instead of #browser, but be aware that this would exclude #sidebar-box and #sidebar-splitter:

<hbox id="browser">
  <vbox id="browser-border-start"/>
  <vbox id="sidebar-box">
    ...
  </vbox>
  <splitter id="sidebar-splitter"/>
  <vbox id="appcontent">
    ...
  </vbox>
  <vbox id="browser-border-end"/>
</hbox>
Attachment #528212 - Flags: review?(dao) → review-
Comment on attachment 528213 [details] [diff] [review]
ext. about css v.1

>--- a/toolkit/themes/winstripe/mozapps/extensions/about.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/about.css

>+  #extensionDetailsBox {
>+    -moz-appearance: -moz-win-exclude-glass;
>+  }

This seems to make no difference. Apparently it's the wrong node, I think you want #clientBox.
Attachment #528213 - Flags: review-
Attachment #528211 - Attachment is obsolete: true
Attachment #528212 - Attachment is obsolete: true
Attachment #528213 - Attachment is obsolete: true
Attachment #528211 - Flags: review?(roc)
Attached patch new layout approach v.1 (obsolete) — Splinter Review
Attached patch browser cssSplinter Review
Attached patch ext. about cssSplinter Review
Attached patch new layout approach v.2 (obsolete) — Splinter Review
updated to use nsIFrame properties for storage.
- switched builder for storage and added clipping to the visible region.
Attachment #529215 - Attachment is obsolete: true
Attachment #529705 - Flags: review?(roc)
No longer blocks: 647555
Comment on attachment 529705 [details] [diff] [review]
new layout approach v.3

Review of attachment 529705 [details] [diff] [review]:

::: layout/base/nsLayoutUtils.cpp
@@ +1640,5 @@
     nsIWidget *widget = aFrame->GetNearestWidget();
     if (widget) {
+      nsRegion excludedRegion = builder.GetExcludedGlassRegion();
+      excludedRegion.Sub(excludedRegion, visibleRegion);
+      nsIntRegion windowRegion(excludedRegion.ToOutsidePixels(presContext->AppUnitsPerDevPixel()));

Should be ToInsidePixels to ensure that we don't exclude glass from partially transparent pixels.
Attachment #529705 - Flags: review?(roc) → review+
Attachment #530077 - Flags: review?(roc)
All of this overhead can go away now. We expect designers to set the right margins. The only thing we want to continue to do is check for plugins just to be safe.

Note the MAX_HORIZONTAL_GLASS_MARGIN must come out since that check introduces a new bug where a visible side bar eliminates the glass haze on the window.
Attachment #530081 - Flags: review?(roc)
Comment on attachment 530077 [details] [diff] [review]
add ToInsidePixels to nsRegion

Review of attachment 530077 [details] [diff] [review]:

This isn't a great definition of ToInsidePixels since if the region contains two rectangles that are adjacent but the edge isn't on a pixel boundary, we'll end up with a region with a one-pixel gap in it. But if it works, it's good enough.

::: gfx/src/nsRegion.cpp
@@ +1372,5 @@
+    nsIntRect deviceRect;
+    if (aOutsidePixels)
+      deviceRect = currentRect->ToOutsidePixels(aAppUnitsPerPixel);
+    else
+      deviceRect = currentRect->ToInsidePixels(aAppUnitsPerPixel);

{}
Comment on attachment 530081 [details] [diff] [review]
updates to UpdateOpaqueRegion part 2

Review of attachment 530081 [details] [diff] [review]:
Attachment #530081 - Flags: review?(roc) → review+
(In reply to comment #157)
> Comment on attachment 530077 [details] [diff] [review] [review]
> add ToInsidePixels to nsRegion
> 
> Review of attachment 530077 [details] [diff] [review] [review]:
> 
> This isn't a great definition of ToInsidePixels since if the region contains
> two rectangles that are adjacent but the edge isn't on a pixel boundary, we'll
> end up with a region with a one-pixel gap in it. But if it works, it's good
> enough.
> 
> ::: gfx/src/nsRegion.cpp
> @@ +1372,5 @@
> +    nsIntRect deviceRect;
> +    if (aOutsidePixels)
> +      deviceRect = currentRect->ToOutsidePixels(aAppUnitsPerPixel);
> +    else
> +      deviceRect = currentRect->ToInsidePixels(aAppUnitsPerPixel);
> 
> {}

I can work up something better, but it'd be great if I can do it in a follow up. In this specific case GetLargestRectangle wallpapers over the problem.
Comment on attachment 530077 [details] [diff] [review]
add ToInsidePixels to nsRegion

Actually I meant to r+ the patch.

I don't think GetLargestRectangle helps you with that problem. The gap will prevent GetLargestRectangle from finding the right rectangle. Hopefully this doesn't matter in practice.
Attachment #530077 - Flags: review?(roc) → review+
Blocks: 654954
Attachment #529105 - Flags: review?(dao)
Attachment #529103 - Attachment is obsolete: true
Comment on attachment 529105 [details] [diff] [review]
browser css

> @media all and (-moz-windows-compositor) {
>+  #appcontent {
>+    -moz-appearance: -moz-win-exclude-glass;
>+  }

Can you move this below the rule where we set -moz-appearance: -moz-win-borderless-glass;? (Or move that rule to the top.)
Attachment #529105 - Flags: review?(dao) → review+
Comment on attachment 529107 [details] [diff] [review]
ext. about css

The right inset border is cut off in these windows, you might want to look into why that's the case. It doesn't seem to be an issue without your patches.
Attachment #529107 - Flags: review+
(In reply to comment #163)
> Comment on attachment 529107 [details] [diff] [review] [review]
> ext. about css
> 
> The right inset border is cut off in these windows, you might want to look
> into why that's the case. It doesn't seem to be an issue without your
> patches.

This was caused by switching to ToInsidePixels().

Roc, seems like maybe we want to use ToOutsidePixels, since the inside method is rounding down in some cases pushing the glass etched border in by one pixel.
How about ToNearestPixels?

I don't like the rendering of the window contents depending on the exact region we get, but I guess we're stuck with it :-(.
(In reply to comment #165)
> How about ToNearestPixels?
> 
> I don't like the rendering of the window contents depending on the exact
> region we get, but I guess we're stuck with it :-(.

Nearest works too, so I'll use that.

I'm going to do one last push to try to check talos results. Assuming all goes well I'll land this tomorrow.
Target Milestone: --- → mozilla6
Depends on: 658785
I can verify that the glaze no longer moves (testing with 5 tabs open). Thanks for the hard work!
Status: RESOLVED → VERIFIED
This problem is still present in Firefox 4.0.1 under Windows 7 64-bit when navigating Google results in a browser window. 

Specifically: hovering over hyperlinks in a Google results list (such as a website name, 'cached', 'similar', and the 'previous' and 'next' Google navigation links at the bottom of the page, causes the Aero 'glaze' in the Firefox window border to move upwards. Moving the cursor away from a hyperlink makes the glaze go down again to its usual position.
(In reply to comment #169)
> This problem is still present in Firefox 4.0.1 under Windows 7 64-bit when
> navigating Google results in a browser window.
The bug was fixed in Firefox 6 and above. You won't see the change in Firefox 4.
I'm not sure if Josh T. is just trolling, but I would like to know when this bug will be fixed. The change is REALLY annoying. Any estimates?
(In reply to comment #171)
> I'm not sure if Josh T. is just trolling, but I would like to know when this
> bug will be fixed. The change is REALLY annoying. Any estimates?

Sorry if I appear to be trolling. The bug is marked as VERIFIED FIXED, so it has already been fixed. You can see the fix right now in the Aurora builds. You will see the fix in Firefox in six weeks after Firefox 5 is released later this month.

If you're using Aurora and you're still experiencing this bug, file a follow up bug.

Sorry for the bugspam.
Depends on: 677095
This bug has not been fixed. Firefox 6 in Windows 7 Ultimate 64bit has the same behavior.
(In reply to haridimos26 from comment #174)
> This bug has not been fixed. Firefox 6 in Windows 7 Ultimate 64bit has the
> same behavior.

I'm not able to reproduce this in 6.0 by hovering over bookmarks in the bookmarks toolbar. Can you provide some steps to reproduce what you are seeing?
I'm sorry, my mistake. This has been resolved in 6.0. Ignore previous comment.
For sheppy:

In past versions Firefox calculated the location of glass boundaries (the point at which transparent window margins end) automatically. In version 6.0 this was replaced with an opt-out style where XUL elements are marked explicitly as opaque via css. Currently various UI elements have this new style including the main content area of the browser, downloads window content, and extension’s about dialog.

The css property we use to identify opaque regions is through a new value for the moz-appearance style:

-moz-appearance: -moz-win-exclude-glass;

Themes which modify browser css must mimic what the default theme is doing as far as content is concerned. If content is not excluded, various anomalies can show up, most notably in windowed plugins which display improperly when placed on transparent window surfaces.

For reference:
http://mxr.mozilla.org/mozilla-central/search?string=moz-win-exclude-glass
Depends on: 672058
You need to log in before you can comment on or make changes to this bug.