629709 - White line of highlight pixels appears above navigation toolbar if the window is maximized

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Csaba Kozák [:WonderCsabo]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11pre) Gecko/20110128 Firefox/4.0b11pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11pre) Gecko/20110128 Firefox/4.0b11pre

The latest nightly (2011-01-28) introduced an unwanted white line above the navbar. It's only present in maximized window. See the screenshot.

Reproducible: Always

Comment 1

[Csaba Kozák [:WonderCsabo]]

Attachment: Screenshot demonstrating the issue

Comment 2

[Jim Mathies [:jimm]]

This probably won't be hard to track down, I'm guessing it's the Windows glass border leaking through after our changes to how we calculate the glass margins. I'd suggest this block as a soft blocker. If it opens a can of worms we probably don't want to worry about it, but if there's an easy fix, we should take it.

Comment 3

[Jim Mathies [:jimm]]

Interesting, the titlebar on win7 is 32px, and our margin gets set to 32px. But we've moved the client border out to zero via chrome margins, so I don't understand (if or why) windows would be drawing anything along the glass edge. Maybe this is something else.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Ok I think I got it. This is how Windows renders the edge between the glass/opaque area.
It's a 1px white border + 1px darker border (just like the window edges). Those are the white border mentioned here and the reason for the darker borders in bug 629722.

The kGlassMarginsAdjustment was meant to be a 2px increase in the final margin calculated, so that these borders fall completely behind the opaque area.
But if plugins intersecting with that border look incorrect, then it's one bug against another

Comment 5

[Valerio]

Attachment: white line in addons manager

the same white line appears when the addons manager is open also in non maximized window

Comment 6

[Jim Mathies [:jimm]]

Ok, I see what's going on here. I thought windows would only paint this border at the client edge, but it paints it at the glass edge. Working on a fix.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Just to add more info, on non-maximized mode this line is not visible because the top of the toolbar has rounded corners, which excludes the toolbar from the opaque region

Comment 8

[Jim Mathies [:jimm]]

Attachment: border

It's unfortunate but we have to make a trade off. The margin calculation changes in bug 622328 improved on the white line that borders plugins, but introduced bug 629722 because the glass was set to 2px instead of 3px. This bug was just a side effect of a bad assumption on my part. So I think the fix here is to just back out the margins changes (results attached in an image) and live with the ugly anomaly with windowed plugins.

Comment 9

[Jim Mathies [:jimm]]

Attachment: undo margin changes but keep logging statement

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 508168 [details] [diff] [review]
undo margin changes but keep logging statement

Alternatively I could back out the cset that had this a reland the log statement. Either way.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Don't think this blocks, but let's keep on the path and get it in through approvals.

Comment 12

[Jim Mathies [:jimm]]

(In reply to comment #11)
> Don't think this blocks, but let's keep on the path and get it in through
> approvals.

This is just backing out some changes in bug 622328 that introduced a regression, so it should probably land for 4.0.

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 508168 [details] [diff] [review]
undo margin changes but keep logging statement

Asking for approval for this partial backout to fix a really ugly visual regression (on mac the equivalent would be regressing the unified toolbar, IMO). The risk seems small and the benefits are large.

Comment 14

[Shawn Wilsher :sdwilsh]

Is there any way for us to test the behavior we want here so we don't accidentally regress it in the future?  (not sure if we can reftest a whole window)

Comment 15

[Jim Mathies [:jimm]]

(In reply to comment #14)
> Is there any way for us to test the behavior we want here so we don't
> accidentally regress it in the future?  (not sure if we can reftest a whole
> window)

Ref testing glass is hard since the background shows through. But the chance of regressing this specific glass margins problem is pretty slim. Roc and I have both worked on this code, so it's pretty well set what we do in this routine. Changes here aren't very likely.

Comment 16

[Valerio]

When probably will land this?

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7e12e3e16e6c

Comment 18

[(no longer active)]

I backed this patch as part of this pushlog <http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7e12e3e16e6c> because of the oranges it caused <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296852850.1296854923.2345.gz&fulltext=1>.

I'm not sure which one of the bugs was at fault, so I just backed them all out.  The assignee needs to investigate and make sure that his patch has not been the culprit, and then re-add checkin-needed.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Relanded all but one of the patches in that push.  For this bug, that's:
http://hg.mozilla.org/mozilla-central/rev/d49d590321e9