Last Comment Bug 633282 - Aero glass glaze effect jumps down when status info overlay appears
: Aero glass glaze effect jumps down when status info overlay appears
Status: VERIFIED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows Vista
: -- major with 12 votes (vote)
: mozilla6
Assigned To: Jim Mathies [:jimm]
:
:
Mentors:
: 631236 633719 637055 637063 637222 637982 641058 641156 643133 643202 643828 644108 644208 644879 646448 646755 648794 654901 664952 (view as bug list)
Depends on: 658785 672058 677095
Blocks: 654954 622770 623092 631698 637018
  Show dependency treegraph
 
Reported: 2011-02-10 11:34 PST by Emanuel Hoogeveen [:ehoogeveen]
Modified: 2012-03-24 10:52 PDT (History)
48 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
set opaque vs visible region on widgets v.1 (53.26 KB, patch)
2011-03-03 11:56 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
set opaque vs visible region on widgets v.2 (54.60 KB, patch)
2011-03-03 12:55 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
notes attached as text file (2.30 KB, text/plain)
2011-03-04 14:24 PST, Jim Mathies [:jimm]
no flags Details
set opaque vs visible region on widgets v.3 (55.38 KB, patch)
2011-03-04 15:45 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
notes attached as text file (updated) (2.40 KB, text/plain)
2011-03-04 15:52 PST, Jim Mathies [:jimm]
no flags Details
set opaque vs visible region on widgets v.3 (55.56 KB, patch)
2011-03-05 10:22 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
backout of c165b3127975 (49.94 KB, patch)
2011-04-19 14:09 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
layout changes v.1 (45.03 KB, patch)
2011-04-19 14:11 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
widget changes v.1 (4.30 KB, patch)
2011-04-19 14:12 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
browser xul changes v.1 (2.04 KB, patch)
2011-04-19 14:13 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
layout changes v.2 (45.08 KB, patch)
2011-04-20 08:49 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
browser xul changes v.2 (2.04 KB, patch)
2011-04-20 08:50 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
widget changes v.2 (5.80 KB, patch)
2011-04-20 08:51 PDT, Jim Mathies [:jimm]
roc: review+
Details | Diff | Splinter Review
browser xul changes v.3 (1.15 KB, patch)
2011-04-20 09:40 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
downloads changes v.1 (1.04 KB, patch)
2011-04-20 09:44 PDT, Jim Mathies [:jimm]
sdwilsh: review+
Details | Diff | Splinter Review
backout of c165b3127975 v.2 (52.96 KB, patch)
2011-04-25 14:11 PDT, Jim Mathies [:jimm]
tnikkel: review+
Details | Diff | Splinter Review
layout changes v.3 (47.36 KB, patch)
2011-04-25 14:14 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
browser css v.1 (694 bytes, patch)
2011-04-25 14:15 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
downloads css v.1 (1.27 KB, patch)
2011-04-25 14:17 PDT, Jim Mathies [:jimm]
sdwilsh: review+
Details | Diff | Splinter Review
layout changes v.4 (47.37 KB, patch)
2011-04-25 16:21 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
browser css v.2 (698 bytes, patch)
2011-04-25 16:22 PDT, Jim Mathies [:jimm]
dao+bmo: review-
Details | Diff | Splinter Review
ext. about css v.1 (573 bytes, patch)
2011-04-25 16:24 PDT, Jim Mathies [:jimm]
dtownsend: review+
dao+bmo: review-
Details | Diff | Splinter Review
add moz-win-exclude-glass style (3.42 KB, patch)
2011-04-29 08:11 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
new layout approach v.1 (4.23 KB, patch)
2011-04-29 08:13 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
browser css (701 bytes, patch)
2011-04-29 08:14 PDT, Jim Mathies [:jimm]
dao+bmo: review+
Details | Diff | Splinter Review
ext. about css (563 bytes, patch)
2011-04-29 08:15 PDT, Jim Mathies [:jimm]
dao+bmo: review+
Details | Diff | Splinter Review
new layout approach v.2 (4.35 KB, patch)
2011-04-29 15:10 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
new layout approach v.3 (4.62 KB, patch)
2011-05-03 06:50 PDT, Jim Mathies [:jimm]
roc: review+
Details | Diff | Splinter Review
add ToInsidePixels to nsRegion (3.36 KB, patch)
2011-05-04 10:37 PDT, Jim Mathies [:jimm]
roc: review+
Details | Diff | Splinter Review
updates to UpdateOpaqueRegion part 2 (4.50 KB, patch)
2011-05-04 10:43 PDT, Jim Mathies [:jimm]
roc: review+
Details | Diff | Splinter Review
border on about extension (20.54 KB, image/png)
2011-05-11 06:51 PDT, Jim Mathies [:jimm]
no flags Details

Description Emanuel Hoogeveen [:ehoogeveen] 2011-02-10 11:34:50 PST
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.
Comment 1 Jim Mathies [:jimm] 2011-02-10 13:54:38 PST
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
Comment 2 Timothy Nikkel (:tnikkel) 2011-02-10 14:12:18 PST
The glass margins are computed from the visible region, right? What is the visible region in both cases?
Comment 3 Lawrence Newton 2011-02-12 09:27:08 PST
The pixel-difference depends on the number of open tabs. See screens in 
https://bugzilla.mozilla.org/show_bug.cgi?id=631236
Comment 4 Jim Mathies [:jimm] 2011-02-12 11:46:43 PST
(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.
Comment 5 Timothy Nikkel (:tnikkel) 2011-02-12 11:49:05 PST
Yeah, that is odd.
Comment 6 James Socol [:jsocol, :james] 2011-02-12 21:35:35 PST
*** Bug 631236 has been marked as a duplicate of this bug. ***
Comment 7 Asa Dotzler [:asa] 2011-02-13 11:55:21 PST
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.
Comment 8 Dietrich Ayala (:dietrich) 2011-02-14 02:35:51 PST
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.
Comment 9 Yves Goergen 2011-02-28 04:35:04 PST
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.
Comment 10 Jim Mathies [:jimm] 2011-02-28 06:46:53 PST
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.
Comment 11 Jim Mathies [:jimm] 2011-02-28 06:58:27 PST
*** Bug 637222 has been marked as a duplicate of this bug. ***
Comment 12 Jim Mathies [:jimm] 2011-02-28 06:59:04 PST
*** Bug 633719 has been marked as a duplicate of this bug. ***
Comment 13 Jim Mathies [:jimm] 2011-02-28 07:01:31 PST
*** Bug 637063 has been marked as a duplicate of this bug. ***
Comment 14 Yves Goergen 2011-02-28 07:19:14 PST
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?
Comment 15 Jim Mathies [:jimm] 2011-02-28 07:32:33 PST
(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.
Comment 16 Asa Dotzler [:asa] 2011-02-28 07:42:03 PST
(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.
Comment 17 Dão Gottwald [:dao] 2011-03-02 00:26:10 PST
*** Bug 637982 has been marked as a duplicate of this bug. ***
Comment 18 Jim Mathies [:jimm] 2011-03-02 10:13:16 PST
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?
Comment 19 Jim Mathies [:jimm] 2011-03-02 10:13:48 PST
cc'ing roc!
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-02 11:25:42 PST
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.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-02 11:25:56 PST
(Maybe it should take a state object)
Comment 22 Jim Mathies [:jimm] 2011-03-02 14:37:52 PST
(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.
Comment 23 Timothy Nikkel (:tnikkel) 2011-03-02 16:51:04 PST
That would always make our aero glaze effect be too low as bug 623092 is about, if we are okay with that.
Comment 24 Jim Mathies [:jimm] 2011-03-02 17:50:20 PST
(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.)
Comment 25 Jim Mathies [:jimm] 2011-03-03 11:56:24 PST
Created attachment 516664 [details] [diff] [review]
set opaque vs visible region on widgets v.1

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?
Comment 26 Jim Mathies [:jimm] 2011-03-03 12:55:17 PST
Created attachment 516679 [details] [diff] [review]
set opaque vs visible region on widgets v.2

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.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-03 15:46:39 PST
+  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.
Comment 28 Jim Mathies [:jimm] 2011-03-03 16:27:11 PST
(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.)
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-03 19:08:11 PST
(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.
Comment 30 Jim Mathies [:jimm] 2011-03-04 14:20:37 PST
(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.
Comment 31 Jim Mathies [:jimm] 2011-03-04 14:24:51 PST
Created attachment 517005 [details]
notes attached as text file
Comment 32 Jim Mathies [:jimm] 2011-03-04 15:45:13 PST
Created attachment 517028 [details] [diff] [review]
set opaque vs visible region on widgets v.3

New rev based on the list I posted.
Comment 33 Jim Mathies [:jimm] 2011-03-04 15:52:11 PST
Created attachment 517033 [details]
notes attached as text file (updated)
Comment 34 Jim Mathies [:jimm] 2011-03-04 16:30:16 PST
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.
Comment 35 Timothy Nikkel (:tnikkel) 2011-03-04 16:35:38 PST
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.
Comment 36 Jim Mathies [:jimm] 2011-03-05 10:22:23 PST
Created attachment 517148 [details] [diff] [review]
set opaque vs visible region on widgets v.3

(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.
Comment 37 Timothy Nikkel (:tnikkel) 2011-03-06 15:55:44 PST
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.
Comment 38 Jim Mathies [:jimm] 2011-03-06 17:29:55 PST
(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.
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-06 17:36:21 PST
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.
Comment 40 Jim Mathies [:jimm] 2011-03-07 05:31:54 PST
(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.
Comment 41 Jim Mathies [:jimm] 2011-03-07 07:49:11 PST
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.
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-07 16:12:54 PST
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.
Comment 43 Timothy Nikkel (:tnikkel) 2011-03-07 16:25:48 PST
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.
Comment 44 Martin Conrad 2011-03-09 07:22:51 PST
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?
Comment 45 Timothy Nikkel (:tnikkel) 2011-03-09 07:28:32 PST
Yes, we are talking about the actual tab in the tab bar that is visible.
Comment 46 Martin Conrad 2011-03-09 07:41:12 PST
(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).
Comment 47 Armen Zambrano [:armenzg] (EDT/UTC-4) 2011-03-09 08:46:01 PST
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.
Comment 48 Emanuel Hoogeveen [:ehoogeveen] 2011-03-09 08:53:03 PST
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.
Comment 49 Martin Conrad 2011-03-09 10:26:50 PST
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!
Comment 50 Armen Zambrano [:armenzg] (EDT/UTC-4) 2011-03-09 11:37:31 PST
(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.
Comment 51 Jim Mathies [:jimm] 2011-03-09 14:29:58 PST
(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?
Comment 52 Martin Conrad 2011-03-09 15:25:15 PST
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).
Comment 53 Alice0775 White 2011-03-11 16:16:29 PST
*** Bug 641058 has been marked as a duplicate of this bug. ***
Comment 54 Alice0775 White 2011-03-11 18:58:39 PST
*** Bug 641156 has been marked as a duplicate of this bug. ***
Comment 55 Alice0775 White 2011-03-14 10:48:30 PDT
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
Comment 56 Alice0775 White 2011-03-14 16:37:07 PDT
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
Comment 57 Alice0775 White 2011-03-19 08:01:21 PDT
*** Bug 643133 has been marked as a duplicate of this bug. ***
Comment 58 Dão Gottwald [:dao] 2011-03-19 18:52:05 PDT
*** Bug 643202 has been marked as a duplicate of this bug. ***
Comment 59 Timothy Nikkel (:tnikkel) 2011-03-20 02:07:06 PDT
FWIW, I didn't mean comment 37 to be fatal to that patch. Just pointing out a potential problem.
Comment 60 Alice0775 White 2011-03-22 11:37:04 PDT
*** Bug 643828 has been marked as a duplicate of this bug. ***
Comment 61 Kevin Brosnan [:kbrosnan] 2011-03-22 12:45:55 PDT
*** Bug 637055 has been marked as a duplicate of this bug. ***
Comment 62 Alice0775 White 2011-03-23 06:43:37 PDT
*** Bug 644108 has been marked as a duplicate of this bug. ***
Comment 63 Jim Mathies [:jimm] 2011-03-23 13:13:31 PDT
*** Bug 644208 has been marked as a duplicate of this bug. ***
Comment 64 Jason 2011-03-23 17:12:16 PDT
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.
Comment 65 Abhinav Kumar 2011-03-23 17:24:01 PDT
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.
Comment 66 Daniel 2011-03-25 01:38:37 PDT
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?
Comment 67 Dão Gottwald [:dao] 2011-03-25 04:14:30 PDT
*** Bug 644879 has been marked as a duplicate of this bug. ***
Comment 68 Jerry Baker 2011-03-25 18:03:23 PDT
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.
Comment 69 Jerry Baker 2011-03-25 18:13:31 PDT
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?
Comment 70 Jerry Baker 2011-03-25 18:17:08 PDT
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.
Comment 71 Jim Mathies [:jimm] 2011-03-25 18:32:16 PDT
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!
Comment 72 Jerry Baker 2011-03-25 18:53:32 PDT
(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.
Comment 73 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-26 02:24:28 PDT
Jim, do you think you'll be able to do comment #39 for FF5?
Comment 74 Jim Mathies [:jimm] 2011-03-26 09:40:20 PDT
(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.
Comment 75 Abhinav Kumar 2011-03-27 01:53:22 PDT
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/).
Comment 76 Alice0775 White 2011-03-30 06:51:50 PDT
*** Bug 646448 has been marked as a duplicate of this bug. ***
Comment 77 Dão Gottwald [:dao] 2011-03-31 00:45:22 PDT
*** Bug 646755 has been marked as a duplicate of this bug. ***
Comment 78 Jim Mathies [:jimm] 2011-03-31 05:40:24 PDT
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.
Comment 79 Alice0775 White 2011-04-09 21:52:20 PDT
*** Bug 648794 has been marked as a duplicate of this bug. ***
Comment 80 Stanimir Stamenkov 2011-04-16 11:13:33 PDT
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.
Comment 81 Jim Mathies [:jimm] 2011-04-19 14:09:09 PDT
Created attachment 527096 [details] [diff] [review]
backout of c165b3127975

The merge on this was a complete failure due to async plugin painting changes, so I backed it manually.
Comment 82 Jim Mathies [:jimm] 2011-04-19 14:11:52 PDT
Created attachment 527097 [details] [diff] [review]
layout changes v.1
Comment 83 Jim Mathies [:jimm] 2011-04-19 14:12:40 PDT
Created attachment 527098 [details] [diff] [review]
widget changes v.1
Comment 84 Jim Mathies [:jimm] 2011-04-19 14:13:11 PDT
Created attachment 527099 [details] [diff] [review]
browser xul changes v.1
Comment 85 Jim Mathies [:jimm] 2011-04-19 14:20:54 PDT
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.
Comment 86 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-19 15:42:29 PDT
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.
Comment 87 Jim Mathies [:jimm] 2011-04-19 15:56:05 PDT
(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.
Comment 88 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-19 16:20:44 PDT
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 89 Dão Gottwald [:dao] 2011-04-19 22:17:29 PDT
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.
Comment 90 Jim Mathies [:jimm] 2011-04-20 06:28:39 PDT
(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'?
Comment 91 Dão Gottwald [:dao] 2011-04-20 07:46:58 PDT
(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.
Comment 92 Jim Mathies [:jimm] 2011-04-20 07:53:08 PDT
(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.
Comment 93 Jim Mathies [:jimm] 2011-04-20 08:49:20 PDT
Created attachment 527284 [details] [diff] [review]
layout changes v.2

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.
Comment 94 Jim Mathies [:jimm] 2011-04-20 08:50:33 PDT
Created attachment 527286 [details] [diff] [review]
browser xul changes v.2
Comment 95 Dão Gottwald [:dao] 2011-04-20 08:50:42 PDT
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.
Comment 96 Jim Mathies [:jimm] 2011-04-20 08:51:34 PDT
Created attachment 527287 [details] [diff] [review]
widget changes v.2

the original was out of date when I posted it. fixes a compile error.
Comment 97 Jim Mathies [:jimm] 2011-04-20 08:57:13 PDT
(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.
Comment 98 Jim Mathies [:jimm] 2011-04-20 09:40:54 PDT
Created attachment 527300 [details] [diff] [review]
browser xul changes v.3

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.
Comment 99 Jim Mathies [:jimm] 2011-04-20 09:44:28 PDT
Created attachment 527302 [details] [diff] [review]
downloads changes v.1

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.
Comment 100 Jim Mathies [:jimm] 2011-04-20 09:46:33 PDT
Comment on attachment 527287 [details] [diff] [review]
widget changes v.2

sorry roc, one more but it's pretty simple.
Comment 101 Jim Mathies [:jimm] 2011-04-20 09:48:58 PDT
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.
Comment 102 Jim Mathies [:jimm] 2011-04-20 09:50:44 PDT
Comment on attachment 527302 [details] [diff] [review]
downloads changes v.1

downloads.xul -> Shawn
Comment 103 Dão Gottwald [:dao] 2011-04-20 10:31:29 PDT
(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?
Comment 104 Jim Mathies [:jimm] 2011-04-20 10:49:32 PDT
(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.
Comment 105 Dão Gottwald [:dao] 2011-04-20 10:57:11 PDT
"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.
Comment 106 Jim Mathies [:jimm] 2011-04-20 11:34:15 PDT
(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.)
Comment 107 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-20 15:58:26 PDT
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.
Comment 108 Dão Gottwald [:dao] 2011-04-20 22:36:38 PDT
(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.
Comment 109 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-20 22:48:29 PDT
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?
Comment 110 Dão Gottwald [:dao] 2011-04-20 23:06:47 PDT
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.
Comment 111 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-21 03:17:11 PDT
(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.
Comment 112 Dão Gottwald [:dao] 2011-04-21 07:32:36 PDT
(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 113 Shawn Wilsher :sdwilsh 2011-04-21 11:56:07 PDT
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)
Comment 114 Timothy Nikkel (:tnikkel) 2011-04-21 17:43:02 PDT
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.
Comment 115 Jim Mathies [:jimm] 2011-04-25 10:12:57 PDT
(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.
Comment 116 Dão Gottwald [:dao] 2011-04-25 11:10:56 PDT
(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.
Comment 117 Asa Dotzler [:asa] 2011-04-25 11:25:51 PDT
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.
Comment 118 Shawn Wilsher :sdwilsh 2011-04-25 11:28:51 PDT
(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.
Comment 119 Dão Gottwald [:dao] 2011-04-25 11:30:35 PDT
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.
Comment 120 Jim Mathies [:jimm] 2011-04-25 11:45:03 PDT
(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.
Comment 121 Jim Mathies [:jimm] 2011-04-25 11:56:57 PDT
(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.
Comment 122 Jim Mathies [:jimm] 2011-04-25 11:58:15 PDT
(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.
Comment 123 Jim Mathies [:jimm] 2011-04-25 14:11:02 PDT
Created attachment 528170 [details] [diff] [review]
backout of c165b3127975 v.2

- removed ROOT_CONTENT_DOC_BG
Comment 124 Jim Mathies [:jimm] 2011-04-25 14:14:07 PDT
Created attachment 528172 [details] [diff] [review]
layout changes v.3

Swapped the attrib for the moz-appearance style with prop '-moz-exclude-glass'
Comment 125 Jim Mathies [:jimm] 2011-04-25 14:15:57 PDT
Created attachment 528173 [details] [diff] [review]
browser css v.1
Comment 126 Jim Mathies [:jimm] 2011-04-25 14:17:16 PDT
Created attachment 528175 [details] [diff] [review]
downloads css v.1

last but not least, downloads css changes.
Comment 127 Jim Mathies [:jimm] 2011-04-25 14:20:32 PDT
(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 128 Shawn Wilsher :sdwilsh 2011-04-25 14:22:09 PDT
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
Comment 129 Dão Gottwald [:dao] 2011-04-25 14:30:05 PDT
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 130 Dão Gottwald [:dao] 2011-04-25 14:44:10 PDT
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;
Comment 131 Dão Gottwald [:dao] 2011-04-25 14:44:48 PDT
toolkit/themes/winstripe/mozapps/extensions/about.css uses -moz-win-glass as well.
Comment 132 Jim Mathies [:jimm] 2011-04-25 15:01:22 PDT
(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.
Comment 133 Dão Gottwald [:dao] 2011-04-25 15:11:51 PDT
> > >   #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.
Comment 134 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-25 15:33:41 PDT
Comment on attachment 527287 [details] [diff] [review]
widget changes v.2

Review of attachment 527287 [details] [diff] [review]:
Comment 135 Jim Mathies [:jimm] 2011-04-25 15:35:42 PDT
(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.
Comment 136 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-25 16:20:51 PDT
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.
Comment 137 Jim Mathies [:jimm] 2011-04-25 16:21:54 PDT
Created attachment 528211 [details] [diff] [review]
layout changes v.4

Modified to use moz-win-exclude-glass.
Comment 138 Jim Mathies [:jimm] 2011-04-25 16:22:49 PDT
Created attachment 528212 [details] [diff] [review]
browser css v.2
Comment 139 Jim Mathies [:jimm] 2011-04-25 16:24:56 PDT
Created attachment 528213 [details] [diff] [review]
ext. about css v.1
Comment 140 Jim Mathies [:jimm] 2011-04-25 16:38:34 PDT
(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.
Comment 141 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-25 16:47:45 PDT
(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.
Comment 142 Jim Mathies [:jimm] 2011-04-25 16:56:12 PDT
(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?
Comment 143 Jim Mathies [:jimm] 2011-04-25 17:03:00 PDT
(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.
Comment 144 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-25 17:59:13 PDT
Those plugin areas won't be in the exclude-glass region, but we'll fix that in nsWindow::SetOpaqueRegion anyway, won't we?
Comment 145 Jim Mathies [:jimm] 2011-04-25 18:26:30 PDT
(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 146 Dão Gottwald [:dao] 2011-04-26 08:26:05 PDT
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>
Comment 147 Dão Gottwald [:dao] 2011-04-26 08:36:12 PDT
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.
Comment 148 Jim Mathies [:jimm] 2011-04-29 08:11:57 PDT
Created attachment 529100 [details] [diff] [review]
add moz-win-exclude-glass style
Comment 149 Jim Mathies [:jimm] 2011-04-29 08:13:03 PDT
Created attachment 529103 [details] [diff] [review]
new layout approach v.1
Comment 150 Jim Mathies [:jimm] 2011-04-29 08:14:10 PDT
Created attachment 529105 [details] [diff] [review]
browser css
Comment 151 Jim Mathies [:jimm] 2011-04-29 08:15:05 PDT
Created attachment 529107 [details] [diff] [review]
ext. about css
Comment 152 Jim Mathies [:jimm] 2011-04-29 15:10:59 PDT
Created attachment 529215 [details] [diff] [review]
new layout approach v.2

updated to use nsIFrame properties for storage.
Comment 153 Jim Mathies [:jimm] 2011-05-03 06:50:00 PDT
Created attachment 529705 [details] [diff] [review]
new layout approach v.3

- switched builder for storage and added clipping to the visible region.
Comment 154 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-03 15:38:15 PDT
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.
Comment 155 Jim Mathies [:jimm] 2011-05-04 10:37:34 PDT
Created attachment 530077 [details] [diff] [review]
add ToInsidePixels to nsRegion
Comment 156 Jim Mathies [:jimm] 2011-05-04 10:43:27 PDT
Created attachment 530081 [details] [diff] [review]
updates to UpdateOpaqueRegion part 2

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.
Comment 157 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-04 14:56:59 PDT
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 158 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-04 14:59:03 PDT
Comment on attachment 530081 [details] [diff] [review]
updates to UpdateOpaqueRegion part 2

Review of attachment 530081 [details] [diff] [review]:
Comment 159 Jim Mathies [:jimm] 2011-05-04 17:12:34 PDT
(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 160 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-04 19:35:02 PDT
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.
Comment 161 Benjamin Smedberg [:bsmedberg] 2011-05-05 06:51:39 PDT
*** Bug 654901 has been marked as a duplicate of this bug. ***
Comment 162 Dão Gottwald [:dao] 2011-05-06 23:14:21 PDT
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.)
Comment 163 Dão Gottwald [:dao] 2011-05-06 23:19:06 PDT
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.
Comment 164 Jim Mathies [:jimm] 2011-05-11 06:51:36 PDT
Created attachment 531619 [details]
border on about extension

(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.
Comment 165 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 16:30:48 PDT
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 :-(.
Comment 166 Jim Mathies [:jimm] 2011-05-12 14:43:19 PDT
(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.
Comment 168 Emanuel Hoogeveen [:ehoogeveen] 2011-05-23 07:32:40 PDT
I can verify that the glaze no longer moves (testing with 5 tabs open). Thanks for the hard work!
Comment 169 iain 2011-05-31 03:50:42 PDT
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.
Comment 170 Josh Tumath 2011-05-31 03:52:40 PDT
(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.
Comment 171 catfacem3n 2011-06-05 22:17:06 PDT
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?
Comment 172 Josh Tumath 2011-06-06 00:34:59 PDT
(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.
Comment 173 Dão Gottwald [:dao] 2011-06-17 01:07:26 PDT
*** Bug 664952 has been marked as a duplicate of this bug. ***
Comment 174 haridimos26 2011-08-17 10:27:45 PDT
This bug has not been fixed. Firefox 6 in Windows 7 Ultimate 64bit has the same behavior.
Comment 175 Jim Mathies [:jimm] 2011-08-17 10:57:53 PDT
(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?
Comment 176 haridimos26 2011-08-17 14:01:51 PDT
I'm sorry, my mistake. This has been resolved in 6.0. Ignore previous comment.
Comment 177 Jim Mathies [:jimm] 2011-08-18 10:53:44 PDT
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
Comment 178 Florian Scholz [:fscholz] (MDN) 2012-03-24 09:47:26 PDT
The new value is now documented on
https://developer.mozilla.org/en/CSS/-moz-appearance
and mentioned on the release notes for Firefox 6
https://developer.mozilla.org/en/Firefox_6_for_developers#Other_changes

Note You need to log in before you can comment on or make changes to this bug.