Bug 24048 – extra windows button padding doesn't apply when there's no appearance

• Component: Forms
• Status: RESOLVED FIXED

[Ojan Vafai]

To match FF, we need to add non-overridable 3px padding-left/right and 1px padding-top/bottom. Currently we do so in RenderThemeWin via adjustButtonInnerStyle only if the button has an appearance. We should do this whether the button has an appearance or not.

As per hyatt's suggestion, will replace adjustButtonInnerStyle with buttonInternalPaddingLeft/Right/Top/Bottom and get rid of the hasAppearance check.

[Ojan Vafai]

Created attachment 27894 [details]
Replace adjustButtonInnerStyle with buttonInternalPaddingLeft/Right/Top/Bottom.

 WebCore/ChangeLog                            |   34 ++++++++++++++++++++++++++
 WebCore/rendering/RenderButton.cpp           |    7 ++++-
 WebCore/rendering/RenderTheme.cpp            |    4 ---
 WebCore/rendering/RenderTheme.h              |    8 ++++-
 WebCore/rendering/RenderThemeChromiumGtk.cpp |   23 +++++++++++++----
 WebCore/rendering/RenderThemeChromiumGtk.h   |    5 +++-
 WebCore/rendering/RenderThemeChromiumWin.cpp |   23 +++++++++++++----
 WebCore/rendering/RenderThemeChromiumWin.h   |    5 +++-
 WebCore/rendering/RenderThemeWin.cpp         |   23 +++++++++++++----
 WebCore/rendering/RenderThemeWin.h           |    5 +++-
 10 files changed, 108 insertions(+), 29 deletions(-)

[Ojan Vafai]

Created attachment 28032 [details]
Replace adjustButtonInnerStyle with buttonInternalPaddingLeft/Right/Top/Bottom.

 WebCore/ChangeLog                            |   34 ++++++++++++++++++++++++++
 WebCore/rendering/RenderButton.cpp           |    7 ++++-
 WebCore/rendering/RenderTheme.cpp            |    4 ---
 WebCore/rendering/RenderTheme.h              |    8 ++++-
 WebCore/rendering/RenderThemeChromiumGtk.cpp |   23 +++++++++++++----
 WebCore/rendering/RenderThemeChromiumGtk.h   |    5 +++-
 WebCore/rendering/RenderThemeChromiumWin.cpp |   23 +++++++++++++----
 WebCore/rendering/RenderThemeChromiumWin.h   |    5 +++-
 WebCore/rendering/RenderThemeWin.cpp         |   23 +++++++++++++----
 WebCore/rendering/RenderThemeWin.h           |    5 +++-
 10 files changed, 108 insertions(+), 29 deletions(-)

[Ojan Vafai]

Created attachment 28033 [details]
Replace adjustButtonInnerStyle with buttonInternalPaddingLeft/Right/Top/Bottom.

 WebCore/ChangeLog                            |   36 ++++++++++++++++++++++++++
 WebCore/rendering/RenderButton.cpp           |    7 +++-
 WebCore/rendering/RenderTheme.cpp            |    4 ---
 WebCore/rendering/RenderTheme.h              |    8 ++++-
 WebCore/rendering/RenderThemeChromiumGtk.cpp |   23 ++++++++++++----
 WebCore/rendering/RenderThemeChromiumGtk.h   |    5 +++-
 WebCore/rendering/RenderThemeChromiumWin.cpp |   23 ++++++++++++----
 WebCore/rendering/RenderThemeChromiumWin.h   |    5 +++-
 WebCore/rendering/RenderThemeWin.cpp         |   23 ++++++++++++----
 WebCore/rendering/RenderThemeWin.h           |    5 +++-
 10 files changed, 110 insertions(+), 29 deletions(-)

[Ojan Vafai]

Created attachment 28035 [details]
Replace adjustButtonInnerStyle with buttonInternalPaddingLeft/Right/Top/Bottom.

 WebCore/ChangeLog                            |   36 ++++++++++++++++++++++++++
 WebCore/rendering/RenderButton.cpp           |    7 +++-
 WebCore/rendering/RenderTheme.cpp            |    4 ---
 WebCore/rendering/RenderTheme.h              |    8 ++++-
 WebCore/rendering/RenderThemeChromiumGtk.cpp |   23 ++++++++++++----
 WebCore/rendering/RenderThemeChromiumGtk.h   |    5 +++-
 WebCore/rendering/RenderThemeChromiumWin.cpp |   23 ++++++++++++----
 WebCore/rendering/RenderThemeChromiumWin.h   |    5 +++-
 WebCore/rendering/RenderThemeWin.cpp         |   23 ++++++++++++----
 WebCore/rendering/RenderThemeWin.h           |    5 +++-
 10 files changed, 110 insertions(+), 29 deletions(-)

[Ojan Vafai]

Created attachment 28036 [details]
Fixes missing "}" that snuck in on my last sync.

 WebCore/ChangeLog                            |   36 ++++++++++++++++++++++++++
 WebCore/rendering/RenderButton.cpp           |    7 +++-
 WebCore/rendering/RenderTheme.cpp            |    4 ---
 WebCore/rendering/RenderTheme.h              |    8 ++++-
 WebCore/rendering/RenderThemeChromiumGtk.cpp |   23 ++++++++++++----
 WebCore/rendering/RenderThemeChromiumGtk.h   |    5 +++-
 WebCore/rendering/RenderThemeChromiumWin.cpp |   23 ++++++++++++----
 WebCore/rendering/RenderThemeChromiumWin.h   |    5 +++-
 WebCore/rendering/RenderThemeWin.cpp         |   23 ++++++++++++----
 WebCore/rendering/RenderThemeWin.h           |    5 +++-
 10 files changed, 110 insertions(+), 29 deletions(-)

[David Levin]

Committed as r41362.

[Jouni Koivuviita]

Hi,

Is there still a way to prevent these additional paddings?

If you're trying to mimic Firefox's behaviour, you should provide a way to remove the additional paddings as well. In Firefox they can be removed with the following CSS (the double colon is not a typo):

button::-moz-focus-inner {
	border: none;
	padding: 0;
}

And why does this concern only the Windows version, and not the OS X version as well? Firefox adds the extra paddings on the OS X version as well.

[Jouni Koivuviita]

Any other comments? Is this a dead issue? Should I open a new ticket?

[Shinichiro Hamaji]

Belated response...

> Is there still a way to prevent these additional paddings?

I think there are no way, unfortunately.

> If you're trying to mimic Firefox's behaviour, you should provide a way to
> remove the additional paddings as well. In Firefox they can be removed with the
> following CSS (the double colon is not a typo):
> 
> button::-moz-focus-inner {
>     border: none;
>     padding: 0;
> }

I agree with you. I've just opened another bug and sent a patch to add -webkit-focus-inner for buttons. Please check Bug 35090 for detail.

> And why does this concern only the Windows version, and not the OS X version as
> well? Firefox adds the extra paddings on the OS X version as well.

Agreed. This should be much easier to fix. I'll open another bug and try fixing this issue, too.