Closed Bug 382147 Opened 15 years ago Closed 15 years ago

disabled buttons are bigger than enabled buttons

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: phiw2, Assigned: dholbert)

References

Details

(Keywords: testcase)

Attachments

(4 files, 2 obsolete files)

Attached file test case
All disabled buttons are 2px bigger than their enabled counterparts.
In the test case, the enabled input[type="submit"] is sized 99px by 19px, while the disabled one is sized at 101px by 21px, as reported by the DomInspector on Minefield 20070526.

Bug 379297 established/improves some logic to size the buttons based on overriding the border-width.
However there is a clash with forms.css. 
Enabled gfx buttons have a 2px thick border, whereas disabled gfx buttons have a 1px thick border and 1px padding on each side (padding is used there to keep the same size between enabled and disabled controls.
That 1px padding is applied to the aqua-themed disabled button, making it effectively bigger than the enabled button.
Attached image screenshot of test case
Flags: blocking1.9+
This is not specific to Mac OS X - I can see it with a trunk nightly on Win XP.
Assignee: joshmoz → nobody
Component: Widget: Cocoa → Layout: Form Controls
OS: Mac OS X → All
QA Contact: cocoa → layout.form-controls
Hardware: Macintosh → All
Do the themed controls ignore the CSS border, then?  If so, shouldn't they also ignore the CSS padding?
Attached file testcase2
Indeed, padding isn't set by the native theme code, so the css in forms.css is used. That has different padding and border-width, depending on the disabled state.
This could be fixed by setting a padding for the native theme code, but that would not solve the case when a web developer changes the border-width.
It seems to me disabled buttons should also have a 2px thick border, just like enabled buttons. In IE7 (on windowsXP), disabled buttons have 2px thick borders, whether they are enabled or disabled.

Btw, I don't understand this:
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsNativeThemeWin.cpp#1367
"// use different padding for non-focusable buttons"
That doesn't seem to work at all in my testing, themed buttons always get 3px wide borders.
> but that would not solve the case when a web developer changes the
> border-width.

Don't we drop native theming in that case?  Or do you mean that the button would jiggle?  That's really the web developer's problem -- they should also set the padding...

We could make disabled buttons have 2px borders, I guess.  It just didn't look as nice, I think.
(In reply to comment #4)
> http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsNativeThemeWin.cpp#1367
> "// use different padding for non-focusable buttons"
> That doesn't seem to work at all in my testing, themed buttons always get 3px
> wide borders. 

Sorry, I misunderstood, that lxr link is for the classic theme. That themed buttons get 3px wide borders is actually a recentish regression. I filed bug 388379 for it.
(In reply to comment #5)
> Don't we drop native theming in that case?  Or do you mean that the button
> would jiggle?  That's really the web developer's problem -- they should also
> set the padding...

Yes, native theming gets dropped in that case. In that case, the button would jiggle, just like in the native theming case.
IE7 doesn't alter padding or border-widths when a button becomes disabled, so I don't agree that's a web developer's problem.

> We could make disabled buttons have 2px borders, I guess.  It just didn't look
> as nice, I think.

I don't think it looks as bad. Fwiw, IE7 seems to have the same border for disabled and normal buttons for the non-native theme. For the native theme, the borders look different (although the border-width stays equual, 2px).
Attached patch patch (obsolete) — Splinter Review
Ok, this solves the issue for me on windows. I think it would also fix it on mac (and linux).
Borders and padding get the same values now, resulting in the same height for disabled and enabled buttons.
The native theme buttons are 2px higher (24px), but I think that's because of bug 388379, which I just filed.
(actually, the buttons in IE7 have a 24px height, while in Mozilla a 22px height seems to be normal)
Attached patch martijn's patch (updated) (obsolete) — Splinter Review
I can't apply Martijn's patch on a current Trunk build -- it looks like in the time since he posted the patch, a different function was swapped in on some of the patch's affected lines.

(Specifically, ConvertBorderToAppUnits replaced ConvertMarginToAppUnits in Trunk.)

Here's an updated version of Martijn's patch that applies to the current Trunk.

Incidentally, this patch fixes the issue on Linux.  That is, in testcase2, the measurements all match up for disabled vs. enabled buttons. (and they didn't match before the patch)
Attachment #272577 - Attachment is obsolete: true
are you going to go for review on this?
Attached patch patchSplinter Review
Removed the "outerrect" stuff from Martijn's patch, to get just the essentials.  (I'm not sure what it did, and it didn't seem to have an effect on this bug.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Basically the patch does two things:
 - Widen border on disabled buttons
 - Change disabled-button-border-color to match that of normal buttons.  (from "ButtonShadow" color to "ButtonFace")  This is because the shadow color is too dark for a 2px-wide border, and it makes disabled buttons not look disabled enough when it's that wide.

There are a few Windows-specific changes required to make this work (hence the nsNativeThemeWin changes)

This fixes the issue on Windows and Linux.  Not sure about Mac -- can someone with a mac test this out?
Attachment #282745 - Flags: review?(roc)
Comment on attachment 282745 [details] [diff] [review]
patch

seems to work fine on Mac
Attachment #282745 - Flags: superreview+
Attachment #282745 - Flags: review?(roc)
Attachment #282745 - Flags: review+
Attachment #280814 - Attachment is obsolete: true
Fix committed:

Checking in layout/style/forms.css;
/cvsroot/mozilla/layout/style/forms.css,v  <--  forms.css
new revision: 3.138; previous revision: 3.137
done
Checking in widget/src/windows/nsNativeThemeWin.cpp;
/cvsroot/mozilla/widget/src/windows/nsNativeThemeWin.cpp,v  <--  nsNativeThemeWin.cpp
new revision: 3.94; previous revision: 3.93
done
Checking in widget/src/xpwidgets/nsNativeTheme.cpp;
/cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.cpp,v  <--  nsNativeTheme.cpp
new revision: 1.39; previous revision: 1.38
done
Checking in widget/src/xpwidgets/nsNativeTheme.h;
/cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.h,v  <--  nsNativeTheme.h
new revision: 1.24; previous revision: 1.23
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
verified with
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a9pre) Gecko/2007100118 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.