844948 - [Mac OS X] Changing the padding of a button element does not change the layout

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Alice Lieutier]

When changing the padding of a button element, the layout is not changed.
However, if the padding is changed as well as an other property that changes the layout, then the padding is taken into account.

Compare the three buttons here:

data:text/html,
<button>button</button>
<button style="padding: 20px;">button</button>
<button style="padding: 20px; background: green;">button</button>

The first two buttons are rendered the same way, but the last one takes into account the 20px padding.

Comment 1

[Alice Lieutier]

Attachment: simple testcase as described in the bug

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sounds like the OS theme provides a padding and we don't drop native theming for padding changes, right?

Comment 3

[Mounir Lamouri (:mounir)]

In bug 381639, we decided to no longer listen to the padding for native themed buttons. For some reasons, I do not have the problem being described on my Firefox with GTK (and the native behaviour is also to not move). I wonder where that is handled.

See:
https://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsNativeThemeCocoa.mm#2506

Comment 4

[Alice Lieutier]

Attachment: patch and reftest

This patch solves the problem on mac OS X.
In case that the padding is set by the author, we do not override it with zeros in nsNativeThemeCocoa.mm
(which was done in the patch to bug 381639)

Included is a reftest checking that a button with no padding is different than a button with a padding set.

Comment 5

[Mounir Lamouri (:mounir)]

Full test suite run on linux64 and macosx:
https://tbpl.mozilla.org/?tree=Try&rev=c0b2e8dddeb3

Reftests on all other systems:
https://tbpl.mozilla.org/?tree=Try&rev=c070d5f89ef8

Comment 6

[Alice Lieutier]

Attachment: difference between w/ and w/o patch

Some reftests failed on Try.
I'm not sure I understand what those reftests test, but it made me look more into the problem.

Attached is an image of the following 4 buttons, before the patch at the top, and after the patch at the bottom.

<button>button</button>
<button style="padding: 0px;">button</button>
<button style="padding-top: 0px;">button</button>
<button style="padding: 20px;">button</button>
<button style="padding: 20px; background: green;">button</button>

We can see that the padding 20px is applied correctly with the patch. However, the | padding:0 | and | padding-top: 0 | are behaving strangely.

If we set the padding to 0, the native style is kept (even though it has padding).
If we set the padding-top to zero, we do not apply the native style anymore, but the style defined in forms.css:
padding: 0px 6px;


So.. I do not know what to do with this info, but it seems that getting a coherent behaviour on those mac OS styled button will be more difficult than I thought.

- Is cocoa the only native style that defines padding for the buttons?
- Should we change forms.css so that it gives padding to the button element in a way that is coherent with cocoa native style?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 718330 [details] [diff] [review]
patch and reftest

It looks to me like Mac OS X is the only nsINativeTheme::GetWidgetPadding implementation that overrides padding for NS_THEME_BUTTON.


>diff -r 06935f2db267 layout/reftests/forms/button/reftest.list
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/layout/reftests/forms/button/reftest.list	Tue Feb 26 09:52:02 2013 +0000
>@@ -0,0 +1,1 @@
>+!= button-padding.html button-ref.html

If you want to test that 6 different things are *each* different between the test and reference, you need 6 different tests.  The test you have written will pass if any 1 of the 6 is different, which isn't what you want.

>\ No newline at end of file

Also please include a newline at end of file.

>diff -r 06935f2db267 layout/reftests/forms/reftest.list
>--- a/layout/reftests/forms/reftest.list	Mon Feb 25 14:16:48 2013 -0500
>+++ b/layout/reftests/forms/reftest.list	Tue Feb 26 09:52:02 2013 +0000
>@@ -80,8 +80,11 @@ include meter/reftest.list
> 
> # output element
> include output/reftest.list
> 
> # progress element
> include progress/reftest.list
> 
> == input-percentage-padding.html input-percentage-padding-ref.html
>+
>+# button
>+include	button/reftest.list

Avoid the extra spaces after "include".

(Also, I'm not sure if it's really worth making a subdirectory for these tests; maybe they should just go in the parent directory.)

>\ No newline at end of file

Same here.

>diff -r 06935f2db267 widget/cocoa/nsNativeThemeCocoa.mm
>--- a/widget/cocoa/nsNativeThemeCocoa.mm	Mon Feb 25 14:16:48 2013 -0500
>+++ b/widget/cocoa/nsNativeThemeCocoa.mm	Tue Feb 26 09:52:02 2013 +0000
>@@ -2507,16 +2507,22 @@ nsNativeThemeCocoa::GetWidgetPadding(nsD
>                                      nsIFrame* aFrame,
>                                      uint8_t aWidgetType,
>                                      nsIntMargin* aResult)
> {
>   // We don't want CSS padding being used for certain widgets.
>   // See bug 381639 for an example of why.
>   switch (aWidgetType) {
>     case NS_THEME_BUTTON:
>+      if (IsWidgetStyled(aFrame->PresContext(), aFrame, aWidgetType) ||
>+          aFrame->PresContext()->HasAuthorSpecifiedRules(aFrame, NS_AUTHOR_SPECIFIED_PADDING)) {
>+        return false;
>+      }
>+      aResult->SizeTo(0, 0, 0, 0);
>+      return true;      

I don't think it makes sense to have code this complicated.  What this code basically says is:
  If there is author-specified padding, don't override the padding.
  If there isn't author-specifed padding, override the padding to 0.
Given that, I think it makes more sense to *never* override the padding, since that's substantially simpler.

However, doing this requires fixing *two* other things:

  (1) GetWidgetBorder correctly needs to report the part of the space that should be considered border rather than including some that should be considered padding.  (It looks like that might have been adjusted incorectly in bug 381639, although maybe it's correct.)  In other words, Mac OS X has a control that we draw some stuff in the middle of.  With this change, we now need to make a sensible decision about which part of the area around the edge of that control is "border" and which part of it is "padding", so that when authors adjust the padding they get sensible results.

  (2) the styles for padding in forms.css (ifdef-ed for Mac) need to match the default Mac behavior that we want, which probably means our current behavior (although might not, if that current behavior doesn't match other browsers and/or other apps on the system).  The constraints we need to meet are:
     (a) we want to have a behavior that makes sense in terms of what border and padding mean
     (b) we want to have a behavior that produces results that are compatible with Web content, which generally means that the buttons are within the range of sizes that occur in other browsers (though other platforms count here)
     (c) we want to have a behavior that matches what other Mac OS X applications do.  (other browsers on Mac OS X sort of count here, but more native applications are probably more relevant for this criterion)


I'm really sorry for taking so long to get to this; I hope you're still interested in helping fix it.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

I must say we fixed bug 381639 in a very wrong way. That could be fixed by several simple changes to the forms.css.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

This is a WIP patch. It is mostly fine and doesn't regress bug 381639. But it doesn't handle the minimum size properly.

Other browsers have min-height of 15px and min-width of 0px, which they do not specify that in the UA style sheet, but those values are reflected in the computed value.

Our current behavior is weird to me. We always add 6px to the width and has a minimum height of 2px on the content-box. The returned height of GetMinimumWidgetSize() is not used because the height is usually NS_UNCONSTRAINEDSIZE and thus the height does not win there. In addition, we do not report the correct min-height.

Anyone who is interested in fixing this: please feel free to take this bug and continue working on this. I'm not going to go deeper at present.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Clear the previous assignee because she doesn't seem to be active on bugzilla anymore.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

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

I guess the remaining issues I mentioned in comment 10 are not as important as this issue itself. We may want to fix them in a separate bug.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase for comparing behavior between browsers

Comment 14

[Markus Stange [:mstange]]

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

Review of attachment 8682856 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but the forms.css change needs to be reviewed by dbaron.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

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

dbaron is not reviewing currently. I suppose heycam can review that as well.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ad04f3a65f8f7e524433473f2dacbc15669d09
Bug 844948 - Allow changing padding of themed button on OS X. r=mstange,heycam

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... I just found there is a comment immediately above the change I made in forms.css which says:
> The sum of border and padding on block-start and block-end
> must be the same here, for text inputs, and for <select>. For
> buttons, make sure to include the -moz-focus-inner border/padding.

I guess I break the requirement here.

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36447eaea97930cd675b5868acc0982684d80082
Backed out changeset e8ad04f3a65f (bug 844948) for reftest failure on OS X

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> Hmmm... I just found there is a comment immediately above the change I made
> in forms.css which says:
> > The sum of border and padding on block-start and block-end
> > must be the same here, for text inputs, and for <select>. For
> > buttons, make sure to include the -moz-focus-inner border/padding.
> 
> I guess I break the requirement here.

But I guess breaking this here is fine, because the assumption has already been broken. Before this change, GetWidgetBorder returns 4px in total for block-axis borders, while the values for text input is got from the system. (FWIW, it seems we use two 3px for block-axis borders of <input>, plus 1px paddings, while Safari uses two 2px borders with 1px paddings.)

bz, it seems this comment was added by you in bug 167236 (13 years ago, wow). I'm not pretty sure what does it really mean if the assumption is broken. Could you shed some light here?

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... It doesn't seem easy to fix all those things... Actually, the question is whether we want to make details match what Safari and Chrome are doing. Specifically, whether we want to count the predefined vertical space into padding (like Safari and Chrome do) or keep counting it as border?

It seems from this aspect, our behavior is different with Edge and Chrome on Windows as well.

I guess we could just go the easiest way here and avoid breaking so many reftests... Making details matching isn't really the goal of this bug...

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch, r=mstange,heycam

This patch is a subset of the previous patch so I suppose no additional review is needed.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edf0834835cf

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f89869a36b165956bcb3800ed33c65f66aa7c620
Bug 844948 - Allow changing padding of themed button on OS X. r=mstange,heycam

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f89869a36b16

Comment 25

[Boris Zbarsky [:bzbarsky]]

> I'm not pretty sure what does it really mean if the assumption is broken.

It means that if you have a button next to a text input it will look crappy, right?  The text will line up, but the actual top/bottom edges will not.  

You can see a testcase at http://web.mit.edu/bzbarsky/www/testcases/form-baseline/textInputInRunningText.html and I do agree that it already looks somewhat crappy in a nightly from a few days ago on Mac, even without your changes.  :(  The question is whether it looks significantly worse with those changes.