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

RESOLVED FIXED in mozilla2.0b12

Status

()

Core
Widget: Win32
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: WonderCsabo, Assigned: jimm)

Tracking

({polish, regression})

Trunk
mozilla2.0b12
x86
Windows 7
polish, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(4 attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
Created attachment 507890 [details]
Screenshot demonstrating the issue
(Reporter)

Updated

7 years ago
Version: unspecified → Trunk

Updated

7 years ago
Blocks: 622328
Component: Theme → General
Keywords: regression
Product: Firefox → Core
QA Contact: theme → general
(Assignee)

Comment 2

7 years ago
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.
blocking2.0: --- → ?
Keywords: polish
(Assignee)

Comment 3

7 years ago
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.
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

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

7 years ago
Summary: White line appears above navtoolbar if the window is maximized → White line of highlight pixels appears above navigation toolbar if the window is maximized

Comment 5

7 years ago
Created attachment 508091 [details]
white line in addons manager

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

Comment 6

7 years ago
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.
Assignee: nobody → jmathies
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
(Assignee)

Comment 8

7 years ago
Created attachment 508162 [details]
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.
(Assignee)

Comment 9

7 years ago
Created attachment 508168 [details] [diff] [review]
undo margin changes but keep logging statement
(Assignee)

Comment 10

7 years ago
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.
Attachment #508168 - Flags: review?(roc)
Blocks: 629722
Don't think this blocks, but let's keep on the path and get it in through approvals.
blocking2.0: ? → -
(Assignee)

Comment 12

7 years ago
(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

7 years ago
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.
Attachment #508168 - Flags: approval2.0?
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)
(Assignee)

Comment 15

7 years ago
(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.
Attachment #508168 - Flags: approval2.0? → approval2.0+

Comment 16

7 years ago
When probably will land this?

Updated

7 years ago
Keywords: checkin-needed

Updated

7 years ago
Component: General → Widget: Win32
QA Contact: general → win32

Comment 17

7 years ago
http://hg.mozilla.org/mozilla-central/rev/7e12e3e16e6c
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12

Comment 18

7 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded all but one of the patches in that push.  For this bug, that's:
http://hg.mozilla.org/mozilla-central/rev/d49d590321e9
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.