Last Comment Bug 618353 - window frame is one pixel too wide on win 7
: window frame is one pixel too wide on win 7
Status: RESOLVED FIXED
[for dev docs, see comment 36]
: dev-doc-complete
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows 7
: -- normal with 5 votes (vote)
: mozilla12
Assigned To: Jim Mathies [:jimm]
:
: Jim Mathies [:jimm]
Mentors:
data:text/html,<script>addEventListen...
: 596057 622163 625348 656478 (view as bug list)
Depends on: 718307 chromemargin_changed 718184 718185 1280748
Blocks: 588764
  Show dependency treegraph
 
Reported: 2010-12-10 10:04 PST by Asa Dotzler [:asa]
Modified: 2016-06-22 03:29 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot comparing mockup to latest nightly (62.40 KB, image/png)
2010-12-10 10:04 PST, Asa Dotzler [:asa]
no flags Details
8-1 / 9/1 (31.02 KB, image/png)
2011-01-10 14:23 PST, Jim Mathies [:jimm]
no flags Details
WIP v1 (7.44 KB, patch)
2011-01-27 15:37 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
caption buttons near the window border (4.46 KB, image/jpeg)
2011-02-02 08:42 PST, Valerio
no flags Details
experimental patch (1006 bytes, patch)
2011-02-06 13:56 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
window margins w/test patch (21.98 KB, image/png)
2011-02-06 13:56 PST, Jim Mathies [:jimm]
no flags Details
bleed over (11.30 KB, image/png)
2011-02-14 07:28 PST, Jim Mathies [:jimm]
no flags Details
fixup borders v.1 (3.22 KB, patch)
2011-02-14 13:49 PST, Jim Mathies [:jimm]
jmathies: review-
Details | Diff | Splinter Review
patch v.1 (3.67 KB, patch)
2012-01-05 10:55 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch v.2 (9.66 KB, patch)
2012-01-09 09:53 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch v.3 (10.57 KB, patch)
2012-01-09 14:34 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch v.4 (12.57 KB, patch)
2012-01-10 08:31 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch v.4 (12.76 KB, patch)
2012-01-10 08:49 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch v.4 (1.03 KB, patch)
2012-01-10 17:22 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch v.4 (14.12 KB, patch)
2012-01-10 17:23 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
patch v.5 (13.66 KB, patch)
2012-01-11 09:45 PST, Jim Mathies [:jimm]
felipc: review+
Details | Diff | Splinter Review
bug in windows button (23.66 KB, image/jpeg)
2012-01-14 03:43 PST, Valerio
no flags Details

Description Asa Dotzler [:asa] 2010-12-10 10:04:28 PST
Created attachment 496835 [details]
screenshot comparing mockup to latest nightly

Firefox's window frame is one pixel too wide on the left, bottom, and right sides. It gives the effect of being kind of fat. This looks especially bad next to Chrome's border which is actually one pixel narrower than Windows' standard width. 

We should match Windows here or if we are going to deviate, we should deviate in favor of the app area and not the window frame.
Comment 1 Alex Faaborg [:faaborg] (Firefox UX) 2010-12-31 13:09:24 PST
*** Bug 622163 has been marked as a duplicate of this bug. ***
Comment 2 Jim Mathies [:jimm] 2011-01-10 14:23:20 PST
Created attachment 502609 [details]
8-1 / 9/1

Content really hasn't moved, we've just removed some default bordering. Isn't this basically bug 588764?
Comment 4 Jim Mathies [:jimm] 2011-01-10 14:31:19 PST
This might have been caused by the removal of our content child widget. Regardless, we should be able to adjust content out a bit and then style things however we want them.
Comment 5 Jim Mathies [:jimm] 2011-01-10 14:33:22 PST
(assuming that 2px border is actually ours to draw in.)
Comment 6 Luke Iliffe (Harlequin99) 2011-01-13 08:23:31 PST
*** Bug 625348 has been marked as a duplicate of this bug. ***
Comment 7 Jim Mathies [:jimm] 2011-01-24 09:49:15 PST
(In reply to comment #5)
> (assuming that 2px border is actually ours to draw in.)

It's not. There's a 2 pixel shading border windows draws. Back in August we removed this by pushing the glass region in two pixels. The only way I can see for us to gain access to this area now would be through custom chrome margins on the left, right, and bottom side of the window. We might not be ready to make a change like that. I'll look into it a bit and see.
Comment 8 :Felipe Gomes (needinfo me!) 2011-01-27 15:37:38 PST
Created attachment 507683 [details] [diff] [review]
WIP v1

fwiw I have a WIP patch that changes this through the chromemargin attribute and fixes some oddities with the change. It works on standard settings but I still see some calc and invalidation probs if I set non-standard border sizes on my system.

I've left some comments on areas that still need investigation/understanding. Posting the WIP patch because it might have useful info
Comment 9 Valerio 2011-02-02 08:42:00 PST
Created attachment 509118 [details]
caption buttons near the window border

Felipe, your patch moves also the caption buttons near the window border as in chrome?
Comment 10 Kurt Felton 2011-02-06 13:27:59 PST
Any chance this is landed before Firefox 4 final comes out? Not a fan of having the borders not match. Wish I could figure out a way to fix this fast. =(
Comment 11 Asa Dotzler [:asa] 2011-02-06 13:35:00 PST
(In reply to comment #10)
> Any chance this is landed before Firefox 4 final comes out? Not a fan of having
> the borders not match. Wish I could figure out a way to fix this fast. =(
This kind of question isn't helpful. If you want to know whether a bug is going to be fixed for a particular release, I suggest you start following the dev.apps.firefox and dev.planning newsgroups so you can understand the various flags and other indicators in a bug and what they mean. Then you can answer your own questions instead of interrupting developers with your status update requests.
Comment 12 Jim Mathies [:jimm] 2011-02-06 13:56:10 PST
Created attachment 510160 [details] [diff] [review]
experimental patch

I was thinking about this the other day. I don't think we need to muck with chrome margins here, we should just adjust the client area out any time we have a a window with the borderless glass transparency mode set on it.

This patch is just a test, it's not the fix. So there's more to do here, but it seems to work pretty well.
Comment 13 Jim Mathies [:jimm] 2011-02-06 13:56:39 PST
Created attachment 510161 [details]
window margins w/test patch
Comment 14 Jim Mathies [:jimm] 2011-02-14 07:28:06 PST
Created attachment 512161 [details]
bleed over

I've put together a patch that accomplishes this. There's one problem though in transparent regions with the border we draw around content - there's bleed over of content over the border. Doesn't appear to have anything to do with the window margins. It might be caused by some assumptions we've made in css. Not sure yet.
Comment 15 Jim Mathies [:jimm] 2011-02-14 13:25:04 PST
(In reply to comment #14)
> Created attachment 512161 [details]
> bleed over
> 
> I've put together a patch that accomplishes this. There's one problem though in
> transparent regions with the border we draw around content - there's bleed over
> of content over the border. Doesn't appear to have anything to do with the
> window margins. It might be caused by some assumptions we've made in css. Not
> sure yet.

Pfft, this is also a problem in fx without these modifications. I've filed bug 634060 on this.
Comment 16 Jim Mathies [:jimm] 2011-02-14 13:49:21 PST
Created attachment 512265 [details] [diff] [review]
fixup borders v.1

In UpdateTransparentRegion/UpdateGlass, we push glass in kGlassMarginAdjustment (2px). This gets rid of a 2px border windows renders. But is doesn't expand the client area out 2px, so we end up with a fat window border. This expands our client area out by the same amount, so that after pushing glass in, the window borders are the right thickness.
Comment 17 :Felipe Gomes (needinfo me!) 2011-02-14 14:24:30 PST
Jim, take a look at the change to ClientMarginHitTestPoint from the patch I had posted. I believe we'll need similar changes here, otherwise the resizer border still uses the original size and then 2px around the content is not clickable and is considered resizer area instead.
Comment 18 Jim Mathies [:jimm] 2011-02-14 14:55:52 PST
(In reply to comment #17)
> Jim, take a look at the change to ClientMarginHitTestPoint from the patch I had
> posted. I believe we'll need similar changes here, otherwise the resizer border
> still uses the original size and then 2px around the content is not clickable
> and is considered resizer area instead.

Well, hmm, so the margin default on win7 is 8px. In UpdateGlass we pushed this in by kGlassMarginAdjustment making it 10px. ClientMarginHitTestPoint doesn't know anything about that change. This then pushes the client area back out by kGlassMarginAdjustment, making it 8px again. Seems like it should be ok. I compared IE9, Chrome, a Nightly, and this build by dragging the resizer slowly over the edge. They all change at the same point AFAICT.

Care to take the patch for a spin and look for problems?
Comment 19 Asa Dotzler [:asa] 2011-02-14 16:22:03 PST
I'd be thrilled (even in the middle of my vacation) to test a try build with this fix.
Comment 20 :Felipe Gomes (needinfo me!) 2011-02-14 16:26:44 PST
I tried the patch for a while this afternoon, and that's the only problem I've identified :)

Take this URL for example:

data:text/html,<script>addEventListener("click",function(e)alert(e.clientX),false)</script>

you'll see that in non-maximized mode, it's not possible to click on the webpage's first two pixels. In Chrome or in a Nightly it is (haven't tried IE9)
Comment 21 Jim Mathies [:jimm] 2011-02-14 16:35:25 PST
(In reply to comment #20)
> I tried the patch for a while this afternoon, and that's the only problem I've
> identified :)
> 
> Take this URL for example:
> 
> data:text/html,<script>addEventListener("click",function(e)alert(e.clientX),false)</script>
> 
> you'll see that in non-maximized mode, it's not possible to click on the
> webpage's first two pixels. In Chrome or in a Nightly it is (haven't tried IE9)

Hmm, in chrome this isn't working for me. In Fx nightly I can click on both 0px and 1px. With this patch I can click on 1px but not 0px. (Zero turns into the resizer.)
Comment 22 Jim Mathies [:jimm] 2011-02-14 16:37:05 PST
Comment on attachment 512265 [details] [diff] [review]
fixup borders v.1

I think you're right though. I want that last pixel, and maybe even the gray border we draw.
Comment 23 :Felipe Gomes (needinfo me!) 2011-02-14 16:43:42 PST
oh chrome doesn't support the js1.8 shorthand for functions, try this instead

data:text/html,<script>addEventListener("click",function(e){alert(e.clientX)},false)</script>

The gray border is not part of the content so it can't trigger anything using this URL, but if that border becomes part of the resizer that's actually a win for us because at the moment that border is not part of the resizer either, so it's a dead 1px column
Comment 24 Jim Mathies [:jimm] 2011-02-15 09:01:41 PST
Hrm, this has become messier than I had hoped. What I would like to do is remove the mCustomNonClient flag and have widget manage the client area for all windows. We have two systems here (glass / custom client bounds) that aren't hooked together. I'd also like to get rid of custom chrome margin values as they complicate this way too much, and really aren't that useful for devs. But these are changes I'm not willing to make this late in the release cycle. (bug 590945 looks like the likely target for that work.) So I'm going to try and work something up here that changes the custom chrome margins when we use them, and directly subtracts them (fixup borders v.1) when we don't.
Comment 25 Jim Mathies [:jimm] 2011-02-15 11:29:22 PST
I think we should punt on this until our first quick turn around 5.0 release. The tree is getting locked down for mostly hard blockers, these changes are pretty invasive, and don't interfere with functionality.
Comment 26 Asa Dotzler [:asa] 2011-02-15 14:31:33 PST
I don't think this will be held against us much. I'm good with pushing it off until the next release.
Comment 27 Jim Mathies [:jimm] 2011-02-15 16:41:23 PST
I'll roll this in with bug 590945, which I'm about ready to start in on for 5.0.
Comment 28 Dão Gottwald [:dao] 2011-03-02 01:47:28 PST
*** Bug 596057 has been marked as a duplicate of this bug. ***
Comment 29 Mats Palmgren (:mats) 2011-05-12 04:51:56 PDT
*** Bug 656478 has been marked as a duplicate of this bug. ***
Comment 30 Gingerbread Man 2011-07-20 23:27:37 PDT
(In reply to comment #10)
> Wish I could figure out a way to fix this fast. =(

The ChromeMarginDoohickey extension can be used as a workaround.
http://soapyhamhocks.deviantart.com/gallery/#/d36f4no
Comment 31 Dão Gottwald [:dao] 2011-09-09 06:52:02 PDT
Considering the WIP patches, could we get this done independently from bug 590945?
Comment 32 henryfhchan 2011-12-01 22:50:28 PST
can we fix this? this should be trivial and Firefox/Nightly is the only application which has an extra pixel. (Chrome has 1 missing pixel)
Comment 33 Jim Mathies [:jimm] 2011-12-02 07:31:22 PST
(In reply to henry.fai.hang.chan from comment #32)
> can we fix this? this should be trivial and Firefox/Nightly is the only
> application which has an extra pixel. (Chrome has 1 missing pixel)

We want to, the problem is finding someone with the time to work on it.
Comment 34 Jim Mathies [:jimm] 2011-12-06 14:40:44 PST
I'm going to be able to get to this this week.
Comment 35 Jim Mathies [:jimm] 2012-01-05 10:55:43 PST
Created attachment 586143 [details] [diff] [review]
patch v.1

First rev., need to do some testing and run this through try.
Comment 36 Jim Mathies [:jimm] 2012-01-06 09:26:38 PST
I'm thinking of changing the spec on the xul chomemargin attribute a bit. Currently the value is a margin list, where the possible values are:

-1  = default system border/client area edge
 0  = no system border/extend the client area out to the window edge
n>0 = set the border to n pixels

The n value option is currently not in use. The idea there was that you could do what we want to accomplish in this bug - decrease the border width by setting a custom value < than the system default. But since the system default value varies across platform and system settings managing this value is tricky.

So I'm thinking of changing the use of n to:

width = default border width - n, where if width < 0 width = 0  

This way we can control the border shrink factor without worrying about the varying width of the default border. It would also avoid applying the value to all windows and also avoid a hard coded value for n down in widget. (re: the patch I already posted.)
Comment 37 Jim Mathies [:jimm] 2012-01-06 09:28:30 PST
Also, another perk - in Windows 8 the rounded corners on window borders are gone, so values for n > 1 or 2 will work and look great.
Comment 38 Jim Mathies [:jimm] 2012-01-09 09:53:48 PST
Created attachment 587026 [details] [diff] [review]
patch v.2

Updated patch. This has the changes I mentioned in comment 36 plus the new chrome margins up in browser.js. Need to do some testing before I seek reviews.
Comment 39 Jim Mathies [:jimm] 2012-01-09 14:34:11 PST
Created attachment 587155 [details] [diff] [review]
patch v.3

bug fixes
Comment 40 Jim Mathies [:jimm] 2012-01-10 08:31:35 PST
Created attachment 587330 [details] [diff] [review]
patch v.4

Additional updates. This removes a theme code one off fix for the odd offset Windows applies to maximized windows. Adjustments for maximized windows now occur in nsWindow when we calculate frame margin widths.
Comment 41 Jim Mathies [:jimm] 2012-01-10 08:49:56 PST
Created attachment 587335 [details] [diff] [review]
patch v.4

nits update
Comment 42 Jim Mathies [:jimm] 2012-01-10 17:22:18 PST
Created attachment 587551 [details] [diff] [review]
patch v.4

Tested on xp and win7 both aero and aero basic. 

Felipe, you are familiar with this code, would you mind doing the initial review?
Comment 43 Jim Mathies [:jimm] 2012-01-10 17:23:27 PST
Created attachment 587552 [details] [diff] [review]
patch v.4

oops, wrong patch.
Comment 44 :Felipe Gomes (needinfo me!) 2012-01-10 18:42:27 PST
(In reply to Jim Mathies [:jimm] from comment #42) 
> Felipe, you are familiar with this code, would you mind doing the initial
> review?

sure, I will do it
Comment 46 Dão Gottwald [:dao] 2012-01-11 07:38:11 PST
(In reply to Jim Mathies [:jimm] from comment #45)
> try builds:
> 
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.
> com-5764d234bc67/

Window controls seem to be busted in maximized mode.
Comment 47 Eddward 2012-01-11 07:41:52 PST
and I see also greater space above tabs in maximized mode
Comment 48 Jim Mathies [:jimm] 2012-01-11 09:45:35 PST
Created attachment 587733 [details] [diff] [review]
patch v.5

(In reply to Dão Gottwald [:dao] from comment #46)
> Window controls seem to be busted in maximized mode.

This sucks. The code is correct, but dwmdefproc doesn't like the client area sitting on the screen bounds. I've added a couple comments explaining this both in UpdateNonClientMargins and in GetWidgetPadding, and will file a follow up bug to investigate. For the time being I'm putting the upper margin padding back on the titlebar element.
Comment 49 Jim Mathies [:jimm] 2012-01-11 10:07:50 PST
(In reply to Jim Mathies [:jimm] from comment #48)
> Created attachment 587733 [details] [diff] [review]
> patch v.5
> 
> (In reply to Dão Gottwald [:dao] from comment #46)
> > Window controls seem to be busted in maximized mode.
> 
> This sucks. The code is correct, but dwmdefproc doesn't like the client area
> sitting on the screen bounds. I've added a couple comments explaining this
> both in UpdateNonClientMargins and in GetWidgetPadding, and will file a
> follow up bug to investigate. For the time being I'm putting the upper
> margin padding back on the titlebar element.

This might have something to do with us not rendering to that off screen area, which might confuse the dwm into thinking we want to control mouse clicks in the area below the client area that is off screen and opaque. I'll have to work up a test app and experiment around a bit. We might be able to handle the mouse clicks ourselves in this case and skip over the dwm def wnd proc.

Anyway, in the interest of getting this implemented in time for the next branch, I'm going to put this work in a follow up bug.
Comment 51 :Felipe Gomes (needinfo me!) 2012-01-11 21:14:06 PST
Comment on attachment 587733 [details] [diff] [review]
patch v.5

Review of attachment 587733 [details] [diff] [review]:
-----------------------------------------------------------------

There are a few questions for clarification and some nits, but I'm already giving r+ with those changed. That said, feel free to send the patch for more review passes if you prefer to do so.

(In reply to Jim Mathies [:jimm] from comment #36)
> So I'm thinking of changing the use of n to:
> 
> width = default border width - n, where if width < 0 width = 0  
> 
> This way we can control the border shrink factor without worrying about the
> varying width of the default border. It would also avoid applying the value
> to all windows and also avoid a hard coded value for n down in widget. (re:
> the patch I already posted.)

Sounds good, I like this scheme!

::: browser/base/content/browser.js
@@ +5459,5 @@
>  #ifdef CAN_DRAW_IN_TITLEBAR
>    document.getElementById("titlebar").hidden = !displayAppButton;
>  
>    if (displayAppButton)
> +    document.documentElement.setAttribute("chromemargin", "0,2,2,2");

Now that these values are more complicated, let's add a brief comment here in browser.js explaining what do the values mean.

And I say we should go with "0,3,3,3" for a super-thin look :)

::: widget/windows/nsNativeThemeWin.cpp
@@ +1805,5 @@
>      aResult->SizeTo(0, 0, 0, 0);
> +    // XXX Maximized windows have an offscreen offset equal to
> +    // the border padding. This should be addressed in nsWindow,
> +    // but currently can't be, see UpdateNonClientMargins.
> +    if (aWidgetType == NS_THEME_WINDOW_TITLEBAR_MAXIMIZED)

> This sucks. The code is correct, but dwmdefproc doesn't like the client area
> sitting on the screen bounds. I've added a couple comments explaining this
> both in UpdateNonClientMargins and in GetWidgetPadding, and will file a
> follow up bug to investigate. For the time being I'm putting the upper
> margin padding back on the titlebar element.

I don't think this needs an XXX and a follow-up. I don't see what's the problem of leaving it this way; I think that's just how the DWM works. IIRC you experimented with this before during the draw-in-titlebar development and saw the same problems, then concluded that having this top border for maximized mode was the right thing to do.

::: widget/windows/nsWindow.cpp
@@ +352,1 @@
>  

OK, maybe the comment could clarify that we will still draw over the edge of the window, it's only the mouse cursor that will always have a minimum of 3px for resizing, possibly overlapping content if someone already have a very thin border size.

@@ +2076,5 @@
>    if (!mNonClientMargins.bottom)
>      mNonClientOffset.bottom = mVertResizeMargin;
>    else if (mNonClientMargins.bottom > 0)
> +    mNonClientOffset.bottom = NS_MIN(mHorResizeMargin, mNonClientMargins.bottom);
> +

mVertResizeMargin in this last one

@@ +2080,5 @@
> +
> +  // Disable chrome margins > 0 in two cases:
> +  // - For non-glass desktops: The window frame is painted with textures that
> +  //   require the entire space of the default frame. We allow a full frame or
> +  //   no frame at all.

Hm I don't see this problem with the "Windows 7 Basic". Is this really a concern? Covering this bug for Aero Basic too would be nice.

@@ +2113,5 @@
> +    // buttons. This is unexplained - filed as bug XYZ. To compensate, we
> +    // position the client area off screen by mVertResizeMargin, and add
> +    // widget padding in nsNativeThemeWin::GetWidgetPadding().
> +    if (!mNonClientMargins.top)
> +      mNonClientOffset.top = mCaptionHeight;

As mentioned above, I don't think this needs to be an XXX. The simpler comment removed above ("Note, for maximized windows...") should suffice.

It's unclear to me if any value other than 0 or -1 for the top margin will ever be useful. But I guess one could in theory have a resizable window with no WS_CAPTION in which they want to reduce the resizer border through chromemargin. In that case I guess we still need to apply this correction here, so the `if` seems more correct as if (mNonClientMargins.top >= 0).

@@ +2114,5 @@
> +    // position the client area off screen by mVertResizeMargin, and add
> +    // widget padding in nsNativeThemeWin::GetWidgetPadding().
> +    if (!mNonClientMargins.top)
> +      mNonClientOffset.top = mCaptionHeight;
> +

.

sigh, the logic in this function is getting really messy. Specially due to first calculating the values and then possibly resetting them. I wonder if there's some way to simplify this, specially the two very similar ifs for nsSizeMode_Maximized.  Maybe follow-up material..

@@ +5643,2 @@
>      right = true;
>  

The <= >= etc on these comparisons are wrong, please keep the current ones. E.g.
if (my >= winRect.top && my <= topBounds) should be
if (my >= winRect.top && my < topBounds)
See bug 599681 for when this was fixed before.

The bug in the patch has the advantage that the 1px gray border that we draw around the content area also become part of the resizer area, but it breaks the edges of the screen in maximized mode (e.g. can't click on the edge of the screen to use the Back button).

It would be nice to find a way to declare that on a non-maximized window we should add +1px on the resizer border.. But this is probably a special case for our browser window, and wouldn't apply to every window using chromemargins.

@@ +5671,5 @@
>    if (mSizeMode == nsSizeMode_Maximized) {
>      // There's no HTTOP in maximized state (bug 575493)
>      if (testResult == HTTOP) {
>        testResult = HTCAPTION;
>      }

if we make nsSizeMode_Maximized mark isResizable = false we can remove this HTTOP -> HTCAPTION special case by changing the above to:

if (top) {
  testResult = HTCAPTION;
} else if (bottom || left || right) {
  testResult = HTBORDER;
}

This would be cleaner IMO, but up to you.
Comment 52 Jim Mathies [:jimm] 2012-01-12 11:18:14 PST
Updated per review comments, some feedback below:

(In reply to Felipe Gomes (:felipe) from comment #51)
> Comment on attachment 587733 [details] [diff] [review]
> patch v.5
> 
> Review of attachment 587733 [details] [diff] [review]:
> 
> And I say we should go with "0,3,3,3" for a super-thin look :)

I tried both but with 3 px value window frame corners get a little weird. Lets let Dao make the end call on this.

> > This sucks. The code is correct, but dwmdefproc doesn't like the client area
> > sitting on the screen bounds. I've added a couple comments explaining this
> > both in UpdateNonClientMargins and in GetWidgetPadding, and will file a
> > follow up bug to investigate. For the time being I'm putting the upper
> > margin padding back on the titlebar element.
> 
> I don't think this needs an XXX and a follow-up. I don't see what's the
> problem of leaving it this way; I think that's just how the DWM works. IIRC
> you experimented with this before during the draw-in-titlebar development
> and saw the same problems, then concluded that having this top border for
> maximized mode was the right thing to do.

Right, it's just, this should work but it doesn't. Either we are doing something that Windows doesn't like (which can be fixed) or this is a bug in Windows code. Not sure, either way I want to experiment a bit to try and figure it out. I'll zap the bug comment though for now since I don't know when I'll get back to this.

> > +
> > +  // Disable chrome margins > 0 in two cases:
> > +  // - For non-glass desktops: The window frame is painted with textures that
> > +  //   require the entire space of the default frame. We allow a full frame or
> > +  //   no frame at all.
> 
> Hm I don't see this problem with the "Windows 7 Basic". Is this really a
> concern? Covering this bug for Aero Basic too would be nice.

Basic hides this pretty well, but on xp it's obvious since the frame has more detail. I could make basic an exception here, but I figured uniform treatment of non-glass desktops was better. 

Dao, any thoughts on this? 

> It's unclear to me if any value other than 0 or -1 for the top margin will
> ever be useful. But I guess one could in theory have a resizable window with
> no WS_CAPTION in which they want to reduce the resizer border through
> chromemargin. In that case I guess we still need to apply this correction
> here, so the `if` seems more correct as if (mNonClientMargins.top >= 0).

Hmm, you identified a bug in the original code, this should check for the lack of a caption and adjust the value accordingly. I'll do some experimenting with a non-caption window and make some adjustments.

> 
> @@ +2114,5 @@
> > +    // position the client area off screen by mVertResizeMargin, and add
> > +    // widget padding in nsNativeThemeWin::GetWidgetPadding().
> > +    if (!mNonClientMargins.top)
> > +      mNonClientOffset.top = mCaptionHeight;
> > +
> 
> .
> 
> sigh, the logic in this function is getting really messy.


Tell me about it, I know!


> Specially due to
> first calculating the values and then possibly resetting them. I wonder if
> there's some way to simplify this, specially the two very similar ifs for
> nsSizeMode_Maximized.  Maybe follow-up material..

I tried to simplify it as much as possible. 

> It would be nice to find a way to declare that on a non-maximized window we
> should add +1px on the resizer border.. But this is probably a special case
> for our browser window, and wouldn't apply to every window using
> chromemargins.

Yep, I don't think we can do this. Our browser window is a special case.
Comment 53 Dão Gottwald [:dao] 2012-01-12 11:35:34 PST
(In reply to Jim Mathies [:jimm] from comment #52)
> > And I say we should go with "0,3,3,3" for a super-thin look :)
> 
> I tried both but with 3 px value window frame corners get a little weird.
> Lets let Dao make the end call on this.

I think we should just this bug fixed and file a followup bug for consideration.

> Basic hides this pretty well, but on xp it's obvious since the frame has
> more detail. I could make basic an exception here, but I figured uniform
> treatment of non-glass desktops was better. 
> 
> Dao, any thoughts on this?

Maybe I'm not looking hard enough, but it seems to me that this bug doesn't affect aero basic at all. We also add the gray border within the client area only with aero glass.
Comment 54 Jim Mathies [:jimm] 2012-01-12 13:11:31 PST
I've pushed the updated patch to try for a full set of test and relevant talos runs. Assuming everything looks good I'll land on inbound tomorrow.
Comment 56 Marco Bonardo [::mak] 2012-01-14 01:36:05 PST
https://hg.mozilla.org/mozilla-central/rev/0ab265a62148
Comment 57 Eddward 2012-01-14 03:19:40 PST
sorry, but I think there is one situation where window border is still wider: http://img52.imageshack.us/img52/7936/sdgd.png
using latest m-c tinderbox build http://hg.mozilla.org/mozilla-central/rev/27a7f197c6fc
Comment 58 Valerio 2012-01-14 03:43:57 PST
Created attachment 588620 [details]
bug in windows button

The patch also reduces the space to the right of the red button.
This problem is also present in Google Chrome and annoy many people.
Comment 59 Dão Gottwald [:dao] 2012-01-14 04:59:05 PST
(In reply to Eddward from comment #57)
> sorry, but I think there is one situation where window border is still
> wider: http://img52.imageshack.us/img52/7936/sdgd.png
> using latest m-c tinderbox build
> http://hg.mozilla.org/mozilla-central/rev/27a7f197c6fc

Please file a bug.

(In reply to Valerio from comment #58)
> Created attachment 588620 [details]
> bug in windows button
> 
> The patch also reduces the space to the right of the red button.
> This problem is also present in Google Chrome and annoy many people.

Ditto, please file a bug. (I suspect we can't control this, though.)
Comment 60 Jim Mathies [:jimm] 2012-01-14 09:18:19 PST
(In reply to Eddward from comment #57)
> sorry, but I think there is one situation where window border is still
> wider: http://img52.imageshack.us/img52/7936/sdgd.png


This is a standard window with a titlebar, the work here doesn't apply to these windows. I would like to shrink the border on standard, non custom chrome margin windows, but I didn't address it here.
Comment 61 henryfhchan 2012-01-27 23:23:17 PST
this doesn't really fix the bug, the borders on the standard windows (popups) are still 1 pixel too wide, while the main browser window is 1 pixel too thin.

I know this is picky, but why don't we fix what "-1" is supposed to mean instead of adjusting the chromemargins for separate windows?
Comment 62 Jim Mathies [:jimm] 2012-01-28 12:26:35 PST
(In reply to henry.fai.hang.chan from comment #61)
> this doesn't really fix the bug, the borders on the standard windows
> (popups) are still 1 pixel too wide, while the main browser window is 1
> pixel too thin.

bug 718307.
Comment 63 Eric Shepherd [:sheppy] 2012-02-27 14:00:29 PST
Documentation updated:

https://developer.mozilla.org/en/XUL/Attribute/chromemargin

And mentioned on Firefox 12 for developers.
Comment 64 cor 2013-10-28 06:11:40 PDT
On the latest release (v24 on Win7 x64) this is still an issue.

My cute macros that pop browser windows to pre-configured sizes fail with Firefox. This one pixel makes all the difference in responsive layout design!

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