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.

Content really hasn't moved, we've just removed some default bordering. Isn't this basically bug 588764?

regression range - http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e1d55bbd1d1d&tochange=6e3f6d18c124

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.

(assuming that 2px border is actually ours to draw in.)

(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.

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

Felipe, your patch moves also the caption buttons near the window border as in chrome?

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. =(

(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.

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.

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.

(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.

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.

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.

(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?

I'd be thrilled (even in the middle of my vacation) to test a try build with this fix.

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)

(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 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.

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

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.

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.

I don't think this will be held against us much. I'm good with pushing it off until the next release.

I'll roll this in with bug 590945, which I'm about ready to start in on for 5.0.

(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

Considering the WIP patches, could we get this done independently from bug 590945?

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)

(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.

I'm going to be able to get to this this week.

First rev., need to do some testing and run this through try.

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.)

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.

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.

bug fixes

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.

nits update

Tested on xp and win7 both aero and aero basic. Felipe, you are familiar with this code, would you mind doing the initial review?

oops, wrong patch.

(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

try builds: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-5764d234bc67/

(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.

and I see also greater space above tabs in maximized mode

(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.

(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.

try builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-53a25da2aa06

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.

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.

(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.

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.

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

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.

(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.)

(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.

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?

(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.

Documentation updated: https://developer.mozilla.org/en/XUL/Attribute/chromemargin And mentioned on Firefox 12 for developers.

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!