philippe (part-time) Reporter
	Description • 17 years ago

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.

	Josh Aas
	Comment 2 • 17 years ago

This is not specific to Mac OS X - I can see it with a trunk nightly on Win XP.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 17 years ago

Do the themed controls ignore the CSS border, then?  If so, shouldn't they also ignore the CSS padding?

	Martijn Wargers (dead)
	Comment 4 • 17 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 17 years ago

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

	Martijn Wargers (dead)
	Comment 6 • 17 years ago

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

	Martijn Wargers (dead)
	Comment 7 • 17 years ago

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

	Martijn Wargers (dead)
	Comment 8 • 17 years ago

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)

	Daniel Holbert [:dholbert] Assignee
	Comment 9 • 17 years ago

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)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 17 years ago

are you going to go for review on this?

	Daniel Holbert [:dholbert] Assignee
	Comment 11 • 17 years ago

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

	Daniel Holbert [:dholbert] Assignee
	Comment 12 • 17 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 17 years ago

Comment on attachment 282745 [details] [diff] [review]
patch

seems to work fine on Mac

	Daniel Holbert [:dholbert] Assignee
	Comment 14 • 17 years ago

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

	philippe (part-time) Reporter
	Comment 15 • 17 years ago

verified with
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a9pre) Gecko/2007100118 Minefield/3.0a9pre