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

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Pablo Greco]

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

Comment 1

[Pablo Greco]

Attachment: 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.

Comment 3

[Dão Gottwald [::dao]]

Note that this isn't limited to classic.

Comment 4

[Jim Mathies [:jimm]]

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

Comment 5

[Jim Mathies [:jimm]]

(Ehsan, if you don't have time for this please re-assign to me.)

Comment 6

[:ehsan akhgari]

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

Comment 7

[Jim Mathies [:jimm]]

Attachment: padding survey

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.

Comment 8

[Dão Gottwald [::dao]]

Considering bug 610335, isn't any padding added with CSS suboptimal?

Comment 9

[Jim Mathies [:jimm]]

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

Comment 10

[Jim Mathies [:jimm]]

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.

Comment 11

[Dão Gottwald [::dao]]

"border-right-width" as in "computed style"? Using what OS / theme?
There's no front-end code setting a border on #titlebar.

Comment 12

[Jim Mathies [:jimm]]

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

Comment 13

[Jim Mathies [:jimm]]

(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

Comment 14

[:Felipe Gomes (needinfo for replies!)]

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

Comment 15

[Jim Mathies [:jimm]]

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

Comment 16

[Jim Mathies [:jimm]]

Attachment: padding patch v.1

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.

Comment 17

[Jim Mathies [:jimm]]

Attachment: padding patch v.1

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.

Comment 18

[Jim Mathies [:jimm]]

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/[e-mail]-831ddff1070e/tryserver-win32/

Comment 19

[Dão Gottwald [::dao]]

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

Comment 20

[Jim Mathies [:jimm]]

Attachment: css fx padding

This matches the padding added to the bottom of the caption buttons.

Comment 21

[Dão Gottwald [::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?

Comment 22

[Jim Mathies [:jimm]]

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

Comment 23

[Dão Gottwald [::dao]]

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

Comment 24

[Jim Mathies [:jimm]]

Attachment: css fx padding

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

Comment 25

[Dão Gottwald [::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?

Comment 26

[Jim Mathies [:jimm]]

Attachment: css fx padding v.3

combining the @media not all and (-moz-windows-compositor) in browser-aero with another one.

Comment 27

[Jim Mathies [:jimm]]

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

Comment 28

[Dão Gottwald [::dao]]

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

Comment 29

[Dão Gottwald [::dao]]

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

Comment 30

[Dão Gottwald [::dao]]

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 */
  }
}

Comment 31

[Dão Gottwald [::dao]]

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

[Dão Gottwald [::dao]]

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.

Comment 33

[Dão Gottwald [::dao]]

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 34

[Jim Mathies [:jimm]]

Comment on attachment 490913 [details]
padding survey

this is obsolete, the latest is in the patch.

Comment 35

[Jim Mathies [:jimm]]

Attachment: css fx padding v.4

Comment 36

[Jim Mathies [:jimm]]

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

Comment 37

[Jim Mathies [:jimm]]

Attachment: css fx padding v.4

2px for aero basic look better.

Comment 38

[Jim Mathies [:jimm]]

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

Comment 39

[Dão Gottwald [::dao]]

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

[Dão Gottwald [::dao]]

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.

Comment 41

[Jim Mathies [:jimm]]

Attachment: theme patch v.1

Touched up some comments and removed the browser css changes.

Comment 42

[Jim Mathies [:jimm]]

Attachment: css patch v.1

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!

Comment 43

[[e-mail]]

Comment on attachment 491552 [details] [diff] [review]
theme patch v.1

I can see this becoming a maintenance nightmare :-(

Comment 44

[Jim Mathies [:jimm]]

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

Comment 45

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/0a758eabb2bb
http://hg.mozilla.org/mozilla-central/rev/c0d1cdc4553a