Closed Bug 606160 Opened 9 years ago Closed 9 years ago

Topmost and Rightmost pixels of Close, Maximize, don't work

Categories

(Core :: Widget: Win32, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: pablo, Assigned: jimm)

References

(Blocks 1 open bug)

Details

(Keywords: polish, regression)

Attachments

(3 files, 8 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101020 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101020 Firefox/4.0b8pre

XP, Classic theme. When drawing in the title bar is active, rightmost and topmost pixels are not detected as part of the click area. This works as expected when paint in title bar is not active, or in windows such as "Show cookies".
Screenshot explaining coming up

Reproducible: Always
Attached image Screenshot
The first buttons belong to a "paint in title bar" minefield window, clicking in the red area doesn't activate the closest button.
The second buttons belong to the "show cookies" window in the same minefield version, clicking in the blue area activates the nearest button.

Also, the whole group is shifted 1px to the right, but I guess that should be another bug.
Blocks: 513157
Keywords: polish
Component: Shell Integration → Widget: Win32
Product: Firefox → Core
QA Contact: shell.integration → win32
blocking2.0: --- → ?
Keywords: regression
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → ehsan
Blocks: 574454
No longer blocks: 513157
Duplicate of this bug: 610916
Blocks: 610335
Note that this isn't limited to classic.
Blocks: 612348
I'm wondering if these areas would become active if the padding was applied in nsNativeThemeWin::GetWidgetPadding instead of css.

We also need to touch up classic maximized padding too, which would need to happen here. Here's a patch that fixes the problem in css:

https://bugzilla.mozilla.org/attachment.cgi?id=490411&action=diff
(Ehsan, if you don't have time for this please re-assign to me.)
(In reply to comment #5)
> (Ehsan, if you don't have time for this please re-assign to me.)

I haven't had a chance to look at this yet, so if you have some ideas, please feel free to try them here.  :-)
Assignee: ehsan → jmathies
Attached file padding survey (obsolete) —
Here's a button padding survey I did. The 'hot padding' I'll handle in the theme code, normal padding in css. I think there are some slight tweaks to normal padding between normal/maximized windows as well. I'll flesh those out once I get this implemented. If anyone spots any errors here, please let me know.
Considering bug 610335, isn't any padding added with CSS suboptimal?
(In reply to comment #8)
> Considering bug 610335, isn't any padding added with CSS suboptimal?

I don't see how I can add padding through the theme code that isn't 'hot'. There's no margin support:

http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsITheme.h

As far as bug 610335, I'm not sure what's going on there, I haven't had a chance to look at it. We have limited information on XP regarding button placement.
Dao, #titlebar has a border-right-width of 1px, which seems to be preventing the close button from sliding all the way to the right so that it can pick up mouse clicks. Any idea where that's set? I tried

border-right-width: 0 !important;

in #titlebar in browser.css but it didn't have any effect.
"border-right-width" as in "computed style"? Using what OS / theme?
There's no front-end code setting a border on #titlebar.
(In reply to comment #11)
> "border-right-width" as in "computed style"? Using what OS / theme?
> There's no front-end code setting a border on #titlebar.

yeah, that was just a guess. I'm not setting it on the button box or the button, so I'm trying to find the 1 px space on the right side when the window is maximized.
(In reply to comment #12)
> (In reply to comment #11)
> > "border-right-width" as in "computed style"? Using what OS / theme?
> > There's no front-end code setting a border on #titlebar.
> 
> yeah, that was just a guess. I'm not setting it on the button box or the
> button, so I'm trying to find the 1 px space on the right side when the window
> is maximized.

aero basic
I'm not completely sure, but I think I saw this 1px border coming from here: http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp#1518
(In reply to comment #14)
> I'm not completely sure, but I think I saw this 1px border coming from here:
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp#1518

That was it! Thanks.
Attached patch padding patch v.1 (obsolete) — Splinter Review
It's unfortunate this padding data needed to be hard coded. (AFAICT there's no way to pull it from the system.) I also need to figure out what we should do when an undetected theme is installed, we might turn all of this off. I'll visit that subject when I get to bug 610335.
Depends on: 610643
Blocks: 595330
Attached patch padding patch v.1 (obsolete) — Splinter Review
Updated commenting. Also ran this through talos looking for txul regressions.

What we're doing here is removing some of the padding around caption buttons defined in css and in the button box container, and placing that padding on the buttons themselves. With this these areas become part of the clickable area of the button.
Attachment #491070 - Attachment is obsolete: true
Attachment #491161 - Flags: review?(neil)
One side effect of this is the fx button gets taller, since the caption buttons grow. Dao, would you like me to try and fix that here of file a follow up?

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jmathies@mozilla.com-831ddff1070e/tryserver-win32/
(In reply to comment #18)
> One side effect of this is the fx button gets taller, since the caption buttons
> grow. Dao, would you like me to try and fix that here of file a follow up?

I'll leave that to you.
Attached patch css fx padding (obsolete) — Splinter Review
This matches the padding added to the bottom of the caption buttons.
Attachment #491174 - Flags: review?(dao)
Comment on attachment 491174 [details] [diff] [review]
css fx padding

nit: use %ifdef WINSTRIPE_AERO around the last rule

Why all the !important flags?
(In reply to comment #21)
> Comment on attachment 491174 [details] [diff] [review]
> css fx padding
> 
> nit: use %ifdef WINSTRIPE_AERO around the last rule
> 
> Why all the !important flags?

Something is modifying the margin settings father down the chain, |margin: 0 0 2px;| doesn't have any effect.
(In reply to comment #22)
> Something is modifying the margin settings father down the chain, |margin: 0 0
> 2px;| doesn't have any effect.

This can't really be true if DOM Inspector is to be trusted.
Did you compile this or put it in userChrome.css?
Attached patch css fx padding (obsolete) — Splinter Review
browser-aero.css was setting it to -1 for aero basic. This new patch seems to cover all the basis. 

XP: browser.css, #appmenu-button, 2px

XP classic: browser.css, @media all and (-moz-windows-classic), 1px

aero glass: browser-aero.css, #appmenu-button, -1

aero basic: browser-aero.css, @media not all and (-moz-windows-compositor), 2px

aero classic: browser-aero.css, 1px, media all and (-moz-windows-classic), 1px
Attachment #491174 - Attachment is obsolete: true
Attachment #491174 - Flags: review?(dao)
> browser-aero.css was setting it to -1 for aero basic. This new patch seems to
> cover all the basis.

So shouldn't it stop doing that?
Attached patch css fx padding v.3 (obsolete) — Splinter Review
combining the @media not all and (-moz-windows-compositor) in browser-aero with another one.
Attachment #491189 - Attachment is obsolete: true
Attachment #491192 - Flags: review?(dao)
(In reply to comment #25)
> > browser-aero.css was setting it to -1 for aero basic. This new patch seems to
> > cover all the basis.
> 
> So shouldn't it stop doing that?

I don't want to touch aero glass, the positioning there seems fine. That -1 is there for a reason right?
(In reply to comment #27)
> (In reply to comment #25)
> > > browser-aero.css was setting it to -1 for aero basic. This new patch seems to
> > > cover all the basis.
> > 
> > So shouldn't it stop doing that?
> 
> I don't want to touch aero glass, the positioning there seems fine. That -1 is
> there for a reason right?

It was, but it predates your changes and you seem to be overriding it in the @media not all and (-moz-windows-compositor) block.
(In reply to comment #28)
> and you seem to be overriding it in the
> @media not all and (-moz-windows-compositor) block.

(for aero basic)
How about:

browser.css:

#appmenu-button {
  margin-bottom: 2px;
}

@media all and (-moz-windows-classic) {
  #appmenu-button {
    margin-bottom: 1px;
  }
}

browser-aero.css:

@media all and (-moz-windows-default-theme) {
  #appmenu-button-container {
    margin-bottom: -1px; /* compensate the button's outer border */
  }
}
(In reply to comment #30)
Nevermind, I think this was wrong for aero glass/basic. I need to think more about this and study your padding survey...
Comment on attachment 491192 [details] [diff] [review]
css fx padding v.3

As I understand it, we need this:

2px by default
1px for classic
1px for aero basic (because of the outer extra border)
-1px for earo glass (because of the outer extra border)

Whereas your patch has 2px for aero basic.
Attachment #491192 - Flags: review?(dao) → review-
Next try:

browser.css:

#appmenu-button {
  margin: 0 0 2px;
}

@media all and (-moz-windows-classic) {
  #appmenu-button {
    margin-bottom: 1px;
  }
}

browser-aero.css:

@media all and (-moz-windows-default-theme) {
  #appmenu-button {
    border: ...
    margin-bottom: 1px; /* compensate white outer border */
  }
  #appmenu-button:-moz-system-metric(windows-compositor) {
    margin-bottom: -1px; /* compensate white outer border */
  }
}

@media all and (-moz-windows-compositor) {
  #appmenu-button {
    margin-bottom: 0;
  }
}

Note that windows-compositor doesn't imply windows-default-theme and the negative margin actually needs both, since we set the extra border only for windows-default-theme. A better solution might be to change the border to be added for everything but windows-classic in browser-aero.css:

@media not all and (-moz-windows-classic) {
  #appmenu-button {
    border: ...
    margin-bottom: 1px; /* compensate white outer border */
  }
}

@media all and (-moz-windows-compositor) {
  #appmenu-button {
    margin-bottom: -1px; /* compensate white outer border */
  }
}
Comment on attachment 490913 [details]
padding survey

this is obsolete, the latest is in the patch.
Attachment #490913 - Attachment is obsolete: true
Attached patch css fx padding v.4 (obsolete) — Splinter Review
Attachment #491192 - Attachment is obsolete: true
(In reply to comment #33)
> Note that windows-compositor doesn't imply windows-default-theme and the
> negative margin actually needs both, since we set the extra border only for
> windows-default-theme. A better solution might be to change the border to be
> added for everything but windows-classic in browser-aero.css:

Why do we only apply this border for windows-default-theme?
Attached patch css fx padding v.4 (obsolete) — Splinter Review
2px for aero basic look better.
Attachment #491323 - Attachment is obsolete: true
Attachment #491336 - Flags: review?(dao)
(In reply to comment #37)
> Created attachment 491336 [details] [diff] [review]
> css fx padding v.4
> 
> 2px for aero basic look better.

Hmm, in which case it's covered by the browser.css 2px margin. So I don't believe I need the extra bits in browser-aero.css.
(In reply to comment #36)
> (In reply to comment #33)
> > Note that windows-compositor doesn't imply windows-default-theme and the
> > negative margin actually needs both, since we set the extra border only for
> > windows-default-theme. A better solution might be to change the border to be
> > added for everything but windows-classic in browser-aero.css:
> 
> Why do we only apply this border for windows-default-theme?

Mostly because we don't it for classic, and because third-party OS themes are a moving target, but adding the border for all of them doesn't seem worse than omitting it for all of them.
Comment on attachment 491336 [details] [diff] [review]
css fx padding v.4

>+@media all and (-moz-windows-compositor) {
>+  #appmenu-button {
>+    margin-bottom: -1px; /* compensate white outer border */
>+  }
>+}

See comment 33 again -- the border currently depends on windows-default-theme, but windows-compositor can match non-default themes, so this is wrong.
Attachment #491336 - Flags: review?(dao) → review-
Attachment #491161 - Attachment is obsolete: true
Attachment #491161 - Flags: review?(neil)
Attachment #491336 - Attachment is obsolete: true
Attached patch theme patch v.1Splinter Review
Touched up some comments and removed the browser css changes.
Attachment #491552 - Flags: review?(neil)
Attached patch css patch v.1Splinter Review
merged the css changes from the other patch and touched up the handling of default, non-default themes.

So in browser.css we start off with 2px for everything:

margin: 0 0 2px;

then we touch up classic which has 1px padding:

margin-bottom: 1px;

In browser-aero.css we adjust default aero windows themes (aero glass is the target, it has no new padding):

margin-bottom: -1px; /* compensate white outer border */

Then we adjust for aero basic, which has the 1px white border and padding of 2px:

margin-bottom: 1px;

and for 'something non-default' aero basic, the original 2px still applies.

I hope that's everything!
Attachment #491555 - Flags: review?(dao)
Attachment #491555 - Flags: review?(dao) → review+
Comment on attachment 491552 [details] [diff] [review]
theme patch v.1

I can see this becoming a maintenance nightmare :-(
Attachment #491552 - Flags: review?(neil) → review+
(In reply to comment #43)
> Comment on attachment 491552 [details] [diff] [review]
> theme patch v.1
> 
> I can see this becoming a maintenance nightmare :-(

Me too. But the padding doesn't appear to be exposed. For example on aero basic, we have a more advanced version of GetTtitlebarInfo through WM_GETTITLEBARINFOEX, but the padding isn't there. If you have any ideas on how we could query for this, I'd love to hear them.
http://hg.mozilla.org/mozilla-central/rev/0a758eabb2bb
http://hg.mozilla.org/mozilla-central/rev/c0d1cdc4553a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.