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

RESOLVED FIXED in Firefox 46

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: Alice Lieutier, Assigned: xidorn)

Tracking

({testcase})

Trunk
mozilla46
All
Mac OS X
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 717987 [details]
simple testcase as described in the bug
Sounds like the OS theme provides a padding and we don't drop native theming for padding changes, right?
Flags: needinfo?(dbaron)
Keywords: testcase
Hardware: x86 → All
Version: unspecified → Trunk
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
Blocks: 381639
(Reporter)

Comment 4

5 years ago
Created attachment 718330 [details] [diff] [review]
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.
Assignee: nobody → alice.lieutier
Status: NEW → ASSIGNED
Attachment #718330 - Flags: review?(dbaron)
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
(Reporter)

Comment 6

5 years ago
Created attachment 718577 [details]
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 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.
Attachment #718330 - Flags: review?(dbaron) → review-
Flags: needinfo?(dbaron)
Summary: Changing the padding of a button element does not change the layout → [Mac OS X] Changing the padding of a button element does not change the layout
Duplicate of this bug: 1216808
I must say we fixed bug 381639 in a very wrong way. That could be fixed by several simple changes to the forms.css.
Created attachment 8682856 [details] [diff] [review]
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.
Clear the previous assignee because she doesn't seem to be active on bugzilla anymore.
Assignee: alice.lieutier → nobody
Status: ASSIGNED → NEW
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.
Attachment #8682856 - Attachment description: wip patch → patch
Attachment #8682856 - Attachment filename: bug844948-wip.patch → bug844948.patch
Attachment #8682856 - Flags: review?(mstange)
Created attachment 8707772 [details]
testcase for comparing behavior between browsers
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.
Attachment #8682856 - Flags: review?(mstange) → review+
Comment on attachment 8682856 [details] [diff] [review]
patch

dbaron is not reviewing currently. I suppose heycam can review that as well.
Attachment #8682856 - Flags: review?(cam)
Attachment #8682856 - Flags: review?(cam) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ad04f3a65f8f7e524433473f2dacbc15669d09
Bug 844948 - Allow changing padding of themed button on OS X. r=mstange,heycam
Assignee: nobody → quanxunzhen
Blocks: 1145589
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/36447eaea97930cd675b5868acc0982684d80082
Backed out changeset e8ad04f3a65f (bug 844948) for reftest failure on OS X
(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?
Flags: needinfo?(bzbarsky)
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...
Created attachment 8709193 [details] [diff] [review]
patch, r=mstange,heycam

This patch is a subset of the previous patch so I suppose no additional review is needed.
Attachment #8682856 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=edf0834835cf
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f89869a36b16
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
> 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.
Flags: needinfo?(bzbarsky)
Depends on: 1294102
You need to log in before you can comment on or make changes to this bug.