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)
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.
Reporter | ||
Updated•13 years ago
|
Version: unspecified → Trunk
Assignee | ||
Updated•13 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 1•13 years ago
|
||
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
Comment 2•13 years ago
|
||
The glass margins are computed from the visible region, right? What is the visible region in both cases?
Comment 3•13 years ago
|
||
The pixel-difference depends on the number of open tabs. See screens in https://bugzilla.mozilla.org/show_bug.cgi?id=631236
Assignee | ||
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
Yeah, that is odd.
Comment 7•13 years ago
|
||
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•13 years ago
|
||
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: ? → -
Comment 9•13 years ago
|
||
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.
Assignee | ||
Comment 10•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Severity: trivial → major
Assignee | ||
Updated•13 years ago
|
Component: Theme → Layout
Product: Firefox → Core
QA Contact: theme → layout
Comment 14•13 years ago
|
||
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?
Assignee | ||
Comment 15•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 18•13 years ago
|
||
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?
Assignee | ||
Comment 19•13 years ago
|
||
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)
Assignee | ||
Comment 22•13 years ago
|
||
(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
Comment 23•13 years ago
|
||
That would always make our aero glaze effect be too low as bug 623092 is about, if we are okay with that.
Assignee | ||
Comment 24•13 years ago
|
||
(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.)
Assignee | ||
Comment 25•13 years ago
|
||
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?
Assignee | ||
Comment 26•13 years ago
|
||
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.
Assignee | ||
Comment 28•13 years ago
|
||
(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.
Assignee | ||
Comment 30•13 years ago
|
||
(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.
Assignee | ||
Comment 31•13 years ago
|
||
Assignee | ||
Comment 32•13 years ago
|
||
New rev based on the list I posted.
Attachment #516679 -
Attachment is obsolete: true
Attachment #516679 -
Flags: feedback?(roc)
Assignee | ||
Comment 33•13 years ago
|
||
Attachment #517005 -
Attachment is obsolete: true
Assignee | ||
Comment 34•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 36•13 years ago
|
||
(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 37•13 years ago
|
||
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.
Assignee | ||
Comment 38•13 years ago
|
||
(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.
Assignee | ||
Comment 40•13 years ago
|
||
(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.
Assignee | ||
Comment 41•13 years ago
|
||
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.
Comment 43•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Yes, we are talking about the actual tab in the tab bar that is visible.
Comment 46•13 years ago
|
||
(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•13 years ago
|
||
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.
Reporter | ||
Comment 48•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 51•13 years ago
|
||
(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•13 years ago
|
||
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 55•13 years ago
|
||
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•13 years ago
|
||
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
Updated•13 years ago
|
Keywords: regression
Comment 59•13 years ago
|
||
FWIW, I didn't mean comment 37 to be fatal to that patch. Just pointing out a potential problem.
Comment 64•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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 68•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 71•13 years ago
|
||
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]
Comment 72•13 years ago
|
||
(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?
Assignee | ||
Comment 74•13 years ago
|
||
(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•13 years ago
|
||
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/).
Assignee | ||
Comment 78•13 years ago
|
||
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 80•13 years ago
|
||
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.
Assignee | ||
Comment 81•13 years ago
|
||
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
Assignee | ||
Comment 82•13 years ago
|
||
Assignee | ||
Comment 83•13 years ago
|
||
Assignee | ||
Comment 84•13 years ago
|
||
Assignee | ||
Comment 85•13 years ago
|
||
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.
Assignee | ||
Comment 87•13 years ago
|
||
(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 89•13 years ago
|
||
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.
Assignee | ||
Comment 90•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 92•13 years ago
|
||
(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.
Assignee | ||
Comment 93•13 years ago
|
||
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
Assignee | ||
Comment 94•13 years ago
|
||
Comment 95•13 years ago
|
||
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.
Assignee | ||
Comment 96•13 years ago
|
||
the original was out of date when I posted it. fixes a compile error.
Attachment #527098 -
Attachment is obsolete: true
Assignee | ||
Comment 97•13 years ago
|
||
(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.
Assignee | ||
Comment 98•13 years ago
|
||
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
Assignee | ||
Comment 99•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #527096 -
Flags: review?(tnikkel)
Assignee | ||
Updated•13 years ago
|
Attachment #527284 -
Flags: review?(roc)
Assignee | ||
Comment 100•13 years ago
|
||
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)
Assignee | ||
Comment 101•13 years ago
|
||
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)
Assignee | ||
Comment 102•13 years ago
|
||
Comment on attachment 527302 [details] [diff] [review] downloads changes v.1 downloads.xul -> Shawn
Attachment #527302 -
Flags: review?(sdwilsh)
Comment 103•13 years ago
|
||
(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?
Assignee | ||
Comment 104•13 years ago
|
||
(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•13 years ago
|
||
"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.
Assignee | ||
Comment 106•13 years ago
|
||
(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.
Comment 108•13 years ago
|
||
(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?
Comment 110•13 years ago
|
||
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.
Comment 112•13 years ago
|
||
(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•13 years ago
|
||
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 114•13 years ago
|
||
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.
Assignee | ||
Comment 115•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 120•13 years ago
|
||
(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.
Assignee | ||
Comment 121•13 years ago
|
||
(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.
Assignee | ||
Comment 122•13 years ago
|
||
(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.
Assignee | ||
Comment 123•13 years ago
|
||
- removed ROOT_CONTENT_DOC_BG
Attachment #527096 -
Attachment is obsolete: true
Attachment #528170 -
Flags: review?(tnikkel)
Attachment #527096 -
Flags: review?(tnikkel)
Assignee | ||
Comment 124•13 years ago
|
||
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)
Assignee | ||
Comment 125•13 years ago
|
||
Attachment #527300 -
Attachment is obsolete: true
Attachment #528173 -
Flags: review?(dao)
Attachment #527300 -
Flags: review?(dao)
Assignee | ||
Comment 126•13 years ago
|
||
last but not least, downloads css changes.
Attachment #527302 -
Attachment is obsolete: true
Attachment #528175 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 127•13 years ago
|
||
(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•13 years ago
|
||
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+
Comment 129•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
toolkit/themes/winstripe/mozapps/extensions/about.css uses -moz-win-glass as well.
Assignee | ||
Comment 132•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #528170 -
Flags: review?(tnikkel) → review+
Comment 133•13 years ago
|
||
> > > #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 on attachment 527287 [details] [diff] [review] widget changes v.2 Review of attachment 527287 [details] [diff] [review]:
Attachment #527287 -
Flags: review?(roc) → review+
Assignee | ||
Comment 135•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
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.
Assignee | ||
Comment 137•13 years ago
|
||
Modified to use moz-win-exclude-glass.
Attachment #528172 -
Attachment is obsolete: true
Attachment #528211 -
Flags: review?(roc)
Attachment #528172 -
Flags: review?(roc)
Assignee | ||
Comment 138•13 years ago
|
||
Attachment #528173 -
Attachment is obsolete: true
Attachment #528212 -
Flags: review?(dao)
Attachment #528173 -
Flags: review?(dao)
Assignee | ||
Comment 139•13 years ago
|
||
Attachment #528213 -
Flags: review?(dtownsend)
Assignee | ||
Comment 140•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #528213 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 142•13 years ago
|
||
(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?
Assignee | ||
Comment 143•13 years ago
|
||
(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?
Assignee | ||
Comment 145•13 years ago
|
||
(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•13 years ago
|
||
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 147•13 years ago
|
||
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-
Assignee | ||
Comment 148•13 years ago
|
||
Attachment #528211 -
Attachment is obsolete: true
Attachment #528212 -
Attachment is obsolete: true
Attachment #528213 -
Attachment is obsolete: true
Attachment #528211 -
Flags: review?(roc)
Assignee | ||
Comment 149•13 years ago
|
||
Assignee | ||
Comment 150•13 years ago
|
||
Assignee | ||
Comment 151•13 years ago
|
||
Assignee | ||
Comment 152•13 years ago
|
||
updated to use nsIFrame properties for storage.
Assignee | ||
Comment 153•13 years ago
|
||
- switched builder for storage and added clipping to the visible region.
Attachment #529215 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #529705 -
Flags: review?(roc)
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+
Assignee | ||
Comment 155•13 years ago
|
||
Attachment #530077 -
Flags: review?(roc)
Assignee | ||
Comment 156•13 years ago
|
||
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+
Assignee | ||
Comment 159•13 years ago
|
||
(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+
Assignee | ||
Updated•13 years ago
|
Attachment #529105 -
Flags: review?(dao)
Assignee | ||
Updated•13 years ago
|
Attachment #529103 -
Attachment is obsolete: true
Comment 162•13 years ago
|
||
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 163•13 years ago
|
||
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+
Assignee | ||
Comment 164•13 years ago
|
||
(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 :-(.
Assignee | ||
Comment 166•13 years ago
|
||
(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.
Assignee | ||
Comment 167•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/12d72a30fe94 http://hg.mozilla.org/mozilla-central/rev/4cc51c619a9e http://hg.mozilla.org/mozilla-central/rev/0faa6e1353fd http://hg.mozilla.org/mozilla-central/rev/2215d8d01819 http://hg.mozilla.org/mozilla-central/rev/7cad85d7872b http://hg.mozilla.org/mozilla-central/rev/b9f2ad2a6954 http://hg.mozilla.org/mozilla-central/rev/a1631f53c948 http://hg.mozilla.org/mozilla-central/rev/d0cec327d933 http://hg.mozilla.org/mozilla-central/rev/232e2bf06b46
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: dev-doc-needed
Resolution: --- → FIXED
Whiteboard: [see comment 71]
Updated•13 years ago
|
Target Milestone: --- → mozilla6
Reporter | ||
Comment 168•13 years ago
|
||
I can verify that the glaze no longer moves (testing with 5 tabs open). Thanks for the hard work!
Status: RESOLVED → VERIFIED
Comment 169•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
(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 174•13 years ago
|
||
This bug has not been fixed. Firefox 6 in Windows 7 Ultimate 64bit has the same behavior.
Assignee | ||
Comment 175•13 years ago
|
||
(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•13 years ago
|
||
I'm sorry, my mistake. This has been resolved in 6.0. Ignore previous comment.
Assignee | ||
Comment 177•13 years ago
|
||
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•12 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•