Bug 1624696 - Respect padding for all elements except checkbox/radio. r=spohl,mstange
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 24 Mar 2020 22:51:26 +0000
changeset 520322 e2f021fe0c3e0f3c0653d153ba1448627471edf7
parent 520321 167562affb96a1777609f6c6bb6f0a1dab520309
child 520323 fcba04973afc0462247576b9a097d972d7294e17
push id111008
push userealvarez@mozilla.com
push dateTue, 24 Mar 2020 22:54:54 +0000
treeherderautoland@fcba04973afc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersspohl, mstange
bugs1624696, 1624080
milestone76.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1624696 - Respect padding for all elements except checkbox/radio. r=spohl,mstange Only button / menulist-button were missing from the hard-coded if condition. I don't think we ever want to override author padding, and this can cause compat issues as the one in this bug. I'm making HasAuthorSpecifiedRules fast in bug 1624080, btw. Differential Revision: https://phabricator.services.mozilla.com/D68085
layout/reftests/bugs/reftest.list
layout/reftests/forms/button/author-padding-notref.html
layout/reftests/forms/button/author-padding.html
layout/reftests/forms/button/reftest.list
testing/web-platform/meta/html/rendering/widgets/baseline-alignment-and-overflow.tentative.html.ini
widget/nsNativeBasicTheme.cpp
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -523,17 +523,17 @@ random-if(cocoaWidget) == 350506-1.html 
 == 352980-3b.html 352980-3-ref.html
 == 352980-3c.html 352980-3-ref.html
 == 352980-3d.html 352980-3-ref.html
 == 352980-3e.html 352980-3-ref.html
 == 352980-3f.html 352980-3-ref.html
 == 356774-1.html 356774-1-ref.html
 == 356775-1.html 356775-1-ref.html
 == 359869-1.html 359869-1-ref.html
-fails-if(!nativeThemePref) != 359903-1.html 359903-1-ref.html # erosion of padding removed in bug 1010675
+!= 359903-1.html 359903-1-ref.html # erosion of padding removed in bug 1010675
 != 359903-2.html 359903-2-ref.html # erosion of padding removed in bug 1010675
 == 360065-1.html 360065-1-ref.html
 == 360746-1.html 360746-1-ref.html
 == 360757-1a.html 360757-1-ref.html
 == 360757-1b.html 360757-1-ref.html
 == 361091-1.html 361091-1-ref.html
 == 362594-1a.html 362594-1-quirks-ref.html
 == 362594-1b.html 362594-1-quirks-ref.html
new file mode 100644
--- /dev/null
+++ b/layout/reftests/forms/button/author-padding-notref.html
@@ -0,0 +1,2 @@
+<!doctype html>
+<button>Foo</button>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/forms/button/author-padding.html
@@ -0,0 +1,3 @@
+<!doctype html>
+<!-- This technically assumes that no theme would have 300px of padding by default, looks like a safe enough assumption -->
+<button style="padding: 300px">Foo</button>
--- a/layout/reftests/forms/button/reftest.list
+++ b/layout/reftests/forms/button/reftest.list
@@ -30,16 +30,18 @@ fuzzy-if(Android,0-125,0-20) == percent-
 fails-if(Android&&nativeThemePref) == disabled-1.html disabled-1-ref.html
 == disabled-2.html disabled-2-ref.html
 
 != disabled-3.html disabled-3-notref.html
 != disabled-4.html disabled-4-notref.html
 != disabled-5.html disabled-5-notref.html
 != disabled-6.html disabled-6-notref.html
 
+!= author-padding.html author-padding-notref.html
+
 == width-auto-size-em-ltr.html width-auto-size-em-ltr-ref.html
 == width-auto-size-ltr.html width-auto-size-ltr-ref.html
 == width-exact-fit-ltr.html width-auto-size-ltr-ref.html
 == width-auto-size-em-rtl.html width-auto-size-em-rtl-ref.html
 == width-auto-size-rtl.html width-auto-size-rtl-ref.html
 == width-exact-fit-rtl.html width-auto-size-rtl-ref.html
 == display-grid-flex-columnset.html display-grid-flex-columnset-ref.html
 == button-empty-columns.html button-empty-columns-ref.html
--- a/testing/web-platform/meta/html/rendering/widgets/baseline-alignment-and-overflow.tentative.html.ini
+++ b/testing/web-platform/meta/html/rendering/widgets/baseline-alignment-and-overflow.tentative.html.ini
@@ -17,19 +17,8 @@
 
   [<input type="file" style="overflow: scroll; appearance: none;">]
     expected:
       if os == "linux": FAIL
 
   [<input type="file" style="overflow: hidden; appearance: auto;">]
     expected:
       if os == "linux": FAIL
-
-  [<input type="color" value="#000000" style="overflow: visible; appearance: none;">]
-    expected:
-      if os == "android": FAIL
-    bug: Test makes wrong assumptions
-  [<input type="color" value="#000000" style="overflow: hidden; appearance: none;">]
-    expected:
-      if os == "android": FAIL
-  [<input type="color" value="#000000" style="overflow: scroll; appearance: none;">]
-    expected:
-      if os == "android": FAIL
--- a/widget/nsNativeBasicTheme.cpp
+++ b/widget/nsNativeBasicTheme.cpp
@@ -720,39 +720,40 @@ LayoutDeviceIntMargin nsNativeBasicTheme
       return LayoutDeviceIntMargin();
   }
 }
 
 bool nsNativeBasicTheme::GetWidgetPadding(nsDeviceContext* aContext,
                                           nsIFrame* aFrame,
                                           StyleAppearance aAppearance,
                                           LayoutDeviceIntMargin* aResult) {
-  if (aAppearance == StyleAppearance::Menulist ||
-      aAppearance == StyleAppearance::MenulistTextfield ||
-      aAppearance == StyleAppearance::NumberInput ||
-      aAppearance == StyleAppearance::Textarea ||
-      aAppearance == StyleAppearance::Textfield) {
-    // If we have author-specified padding for these elements, don't do the
-    // fixups below.
-    if (aFrame->PresContext()->HasAuthorSpecifiedRules(
-            aFrame, NS_AUTHOR_SPECIFIED_PADDING)) {
-      return false;
-    }
-  }
-
-  uint32_t dpi = GetDPIRatio(aFrame);
   switch (aAppearance) {
     // Radios and checkboxes return a fixed size in GetMinimumWidgetSize
     // and have a meaningful baseline, so they can't have
     // author-specified padding.
     case StyleAppearance::Radio:
     case StyleAppearance::Checkbox:
     case StyleAppearance::MozMenulistArrowButton:
       aResult->SizeTo(0, 0, 0, 0);
       return true;
+    default:
+      break;
+  }
+
+  // Respect author padding.
+  //
+  // TODO(emilio): Consider just unconditionally returning false, so that the
+  // default size of all elements matches other platforms and the UA stylesheet.
+  if (aFrame->PresContext()->HasAuthorSpecifiedRules(
+          aFrame, NS_AUTHOR_SPECIFIED_PADDING)) {
+    return false;
+  }
+
+  uint32_t dpi = GetDPIRatio(aFrame);
+  switch (aAppearance) {
     case StyleAppearance::Textarea:
     case StyleAppearance::Listbox:
     case StyleAppearance::Menulist:
     case StyleAppearance::MenulistButton:
     case StyleAppearance::MenulistTextfield:
     case StyleAppearance::NumberInput:
       aResult->SizeTo(6 * dpi, 7 * dpi, 6 * dpi, 7 * dpi);
       return true;
@@ -762,20 +763,18 @@ bool nsNativeBasicTheme::GetWidgetPaddin
     case StyleAppearance::Textfield:
       if (IsDateTimeTextField(aFrame)) {
         aResult->SizeTo(7 * dpi, 7 * dpi, 5 * dpi, 7 * dpi);
         return true;
       }
       aResult->SizeTo(6 * dpi, 7 * dpi, 6 * dpi, 7 * dpi);
       return true;
     default:
-      break;
+      return false;
   }
-
-  return false;
 }
 
 bool nsNativeBasicTheme::GetWidgetOverflow(nsDeviceContext* aContext,
                                            nsIFrame* aFrame,
                                            StyleAppearance aAppearance,
                                            nsRect* aOverflowRect) {
   // TODO(bug 1620360): This should return non-zero for
   // StyleAppearance::FocusOutline, if we implement outline-style: auto.