Closed
Bug 606160
Opened 11 years ago
Closed 11 years ago
Topmost and Rightmost pixels of Close, Maximize, don't work
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: pablo, Assigned: jimm)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: polish, regression)
Attachments
(3 files, 8 obsolete files)
1.97 KB,
image/jpeg
|
Details | |
10.75 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
2.22 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•11 years ago
|
||
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.
Updated•11 years ago
|
Component: Shell Integration → Widget: Win32
Product: Firefox → Core
QA Contact: shell.integration → win32
Updated•11 years ago
|
blocking2.0: --- → ?
Keywords: regression
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
blocking2.0: ? → final+
Updated•11 years ago
|
Assignee: nobody → ehsan
Updated•11 years ago
|
![]() |
Assignee | |
Comment 4•11 years ago
|
||
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
![]() |
Assignee | |
Comment 5•11 years ago
|
||
(Ehsan, if you don't have time for this please re-assign to me.)
Comment 6•11 years ago
|
||
(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
![]() |
Assignee | |
Comment 7•11 years ago
|
||
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•11 years ago
|
||
Considering bug 610335, isn't any padding added with CSS suboptimal?
![]() |
Assignee | |
Comment 9•11 years ago
|
||
(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.
![]() |
Assignee | |
Comment 10•11 years ago
|
||
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•11 years ago
|
||
"border-right-width" as in "computed style"? Using what OS / theme? There's no front-end code setting a border on #titlebar.
![]() |
Assignee | |
Comment 12•11 years ago
|
||
(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.
![]() |
Assignee | |
Comment 13•11 years ago
|
||
(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•11 years ago
|
||
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
![]() |
Assignee | |
Comment 15•11 years ago
|
||
(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.
![]() |
Assignee | |
Comment 16•11 years ago
|
||
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.
![]() |
Assignee | |
Comment 17•11 years ago
|
||
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)
![]() |
Assignee | |
Comment 18•11 years ago
|
||
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/
Comment 19•11 years ago
|
||
(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.
![]() |
Assignee | |
Comment 20•11 years ago
|
||
This matches the padding added to the bottom of the caption buttons.
Attachment #491174 -
Flags: review?(dao)
Comment 21•11 years ago
|
||
Comment on attachment 491174 [details] [diff] [review] css fx padding nit: use %ifdef WINSTRIPE_AERO around the last rule Why all the !important flags?
![]() |
Assignee | |
Comment 22•11 years ago
|
||
(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•11 years ago
|
||
(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?
![]() |
Assignee | |
Comment 24•11 years ago
|
||
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)
Comment 25•11 years ago
|
||
> 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?
![]() |
Assignee | |
Comment 26•11 years ago
|
||
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)
![]() |
Assignee | |
Comment 27•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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-
Comment 33•11 years ago
|
||
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 */ } }
![]() |
Assignee | |
Comment 34•11 years ago
|
||
Comment on attachment 490913 [details]
padding survey
this is obsolete, the latest is in the patch.
Attachment #490913 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 35•11 years ago
|
||
Attachment #491192 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 36•11 years ago
|
||
(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?
![]() |
Assignee | |
Comment 37•11 years ago
|
||
2px for aero basic look better.
Attachment #491323 -
Attachment is obsolete: true
Attachment #491336 -
Flags: review?(dao)
![]() |
Assignee | |
Comment 38•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
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-
![]() |
Assignee | |
Updated•11 years ago
|
Attachment #491161 -
Attachment is obsolete: true
Attachment #491161 -
Flags: review?(neil)
![]() |
Assignee | |
Updated•11 years ago
|
Attachment #491336 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 41•11 years ago
|
||
Touched up some comments and removed the browser css changes.
Attachment #491552 -
Flags: review?(neil)
![]() |
Assignee | |
Comment 42•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #491555 -
Flags: review?(dao) → review+
Comment 43•11 years ago
|
||
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+
![]() |
Assignee | |
Comment 44•11 years ago
|
||
(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.
![]() |
Assignee | |
Comment 45•11 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/0a758eabb2bb http://hg.mozilla.org/mozilla-central/rev/c0d1cdc4553a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•