Bug 1435658: Deal with appearance changes from / to none correctly. r=mats
☠☠ backed out by 338ff3f5f227 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 05 Feb 2018 14:55:31 +0100
changeset 457400 c074e2c4b2ed6c733479cad11204244edb9e51ef
parent 457399 f7334fd418e40a7cb6a76e39451b011fca1333aa
child 457401 f2c4c01065768629a2f8f055d0a96dc6bb43199c
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1435658
milestone60.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 1435658: Deal with appearance changes from / to none correctly. r=mats MozReview-Commit-ID: Fl6VY0rAIiD
layout/base/nsCSSFrameConstructor.cpp
layout/style/nsStyleStruct.cpp
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/css/css-ui/appearance-dynamic-001-ref.html
testing/web-platform/tests/css/css-ui/appearance-dynamic-001.html
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -3884,16 +3884,19 @@ nsCSSFrameConstructor::FindInputData(Ele
   nsCOMPtr<nsIFormControl> control = do_QueryInterface(aElement);
   NS_ASSERTION(control, "input doesn't implement nsIFormControl?");
 
   auto controlType = control->ControlType();
 
   // radio and checkbox inputs with appearance:none should be constructed
   // by display type.  (Note that we're not checking that appearance is
   // not (respectively) NS_THEME_RADIO and NS_THEME_CHECKBOX.)
+  //
+  // If this block ever goes away, then NS_THEME_NONE can be removed from
+  // AppearanceValueChangeReconstructsFrames.
   if ((controlType == NS_FORM_INPUT_CHECKBOX ||
        controlType == NS_FORM_INPUT_RADIO) &&
       aStyleContext->StyleDisplay()->mAppearance == NS_THEME_NONE) {
     return nullptr;
   }
 
   return FindDataByInt(controlType, aElement, aStyleContext,
                        sInputData, ArrayLength(sInputData));
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -3750,16 +3750,43 @@ CompareTransformValues(const RefPtr<nsCS
     } else {
       result |= nsChangeHint_UpdateOverflow;
     }
   }
 
   return result;
 }
 
+static bool
+AppearanceValueChangeReconstructsFrames(const nsStyleDisplay& aDisplay)
+{
+  if (aDisplay.mAppearance == NS_THEME_TEXTFIELD) {
+    // This is for <input type=number> where we allow authors to specify a
+    // |-moz-appearance:textfield| to get a control without a spinner. (The
+    // spinner is present for |-moz-appearance:number-input| but also other
+    // values such as 'none'.) We need to reframe since we want to use
+    // nsTextControlFrame instead of nsNumberControlFrame if the author
+    // specifies 'textfield'.
+    return true;
+  }
+
+  if (aDisplay.mAppearance == NS_THEME_NONE) {
+    // This is for checkboxes / radio buttons. Changing their appearance value
+    // to none makes them non-replaced elements, and thus we need to reconstruct
+    // the frame.
+    //
+    // TODO(emilio): It's kind of unfortunate that we can't be more granular
+    // about this so it really only applies to the inputs, but I guess it's not
+    // a huge deal.
+    return true;
+  }
+
+  return false;
+}
+
 nsChangeHint
 nsStyleDisplay::CalcDifference(const nsStyleDisplay& aNewData) const
 {
   nsChangeHint hint = nsChangeHint(0);
 
   if (!DefinitelyEqualURIsAndPrincipal(mBinding, aNewData.mBinding)
       || mPosition != aNewData.mPosition
       || mDisplay != aNewData.mDisplay
@@ -3771,26 +3798,20 @@ nsStyleDisplay::CalcDifference(const nsS
       || mScrollSnapPointsX != aNewData.mScrollSnapPointsX
       || mScrollSnapPointsY != aNewData.mScrollSnapPointsY
       || mScrollSnapDestination != aNewData.mScrollSnapDestination
       || mTopLayer != aNewData.mTopLayer
       || mResize != aNewData.mResize) {
     return nsChangeHint_ReconstructFrame;
   }
 
-  if ((mAppearance == NS_THEME_TEXTFIELD &&
-       aNewData.mAppearance != NS_THEME_TEXTFIELD) ||
-      (mAppearance != NS_THEME_TEXTFIELD &&
-       aNewData.mAppearance == NS_THEME_TEXTFIELD)) {
-    // This is for <input type=number> where we allow authors to specify a
-    // |-moz-appearance:textfield| to get a control without a spinner. (The
-    // spinner is present for |-moz-appearance:number-input| but also other
-    // values such as 'none'.) We need to reframe since we want to use
-    // nsTextControlFrame instead of nsNumberControlFrame if the author
-    // specifies 'textfield'.
+  // See if we need to reframe due to our appearance changing.
+  if (mAppearance != aNewData.mAppearance &&
+      (AppearanceValueChangeReconstructsFrames(*this) ||
+       AppearanceValueChangeReconstructsFrames(aNewData))) {
     return nsChangeHint_ReconstructFrame;
   }
 
   if (mOverflowX != aNewData.mOverflowX
       || mOverflowY != aNewData.mOverflowY) {
     hint |= nsChangeHint_CSSOverflowChange;
   }
 
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -146540,16 +146540,28 @@
       [
        "/css/css-transitions/reference/transition-test-ref.html",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "css/css-ui/appearance-dynamic-001.html": [
+    [
+     "/css/css-ui/appearance-dynamic-001.html",
+     [
+      [
+       "/css/css-ui/appearance-dynamic-001-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/css-ui/box-sizing-001.html": [
     [
      "/css/css-ui/box-sizing-001.html",
      [
       [
        "/css/css-ui/reference/box-sizing-001-ref.html",
        "=="
       ]
@@ -254181,16 +254193,21 @@
      {}
     ]
    ],
    "css/css-ui/OWNERS": [
     [
      {}
     ]
    ],
+   "css/css-ui/appearance-dynamic-001-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/css-ui/reference/box-sizing-001-ref.html": [
     [
      {}
     ]
    ],
    "css/css-ui/reference/box-sizing-007-ref.html": [
     [
      {}
@@ -514151,16 +514168,24 @@
   "css/css-typed-om/the-stylepropertymap/interface.html": [
    "73aac61c85d142f38b871ef21c8ce75bd468cf40",
    "testharness"
   ],
   "css/css-ui/OWNERS": [
    "beeb8a77d396e48731fd1e69a922b6e2c84c2caa",
    "support"
   ],
+  "css/css-ui/appearance-dynamic-001-ref.html": [
+   "5759d04966f969c591959f86390af98725d57f3d",
+   "support"
+  ],
+  "css/css-ui/appearance-dynamic-001.html": [
+   "4a2a1333835df29e7048fa7fc115346b279cc7e5",
+   "reftest"
+  ],
   "css/css-ui/box-sizing-001.html": [
    "5e913f2edc75ae0369eb59f67f320ec552472160",
    "reftest"
   ],
   "css/css-ui/box-sizing-003.html": [
    "8a4fae73f15bf08fa1bfef48a23a4927aa1cf897",
    "reftest"
   ],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-ui/appearance-dynamic-001-ref.html
@@ -0,0 +1,12 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test Reference</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+
+<p>Should see form controls with their default styling</p>
+
+<input type="checkbox">
+<input type="radio">
+<input type="text">
+<input type="number">
+<input type="date">
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-ui/appearance-dynamic-001.html
@@ -0,0 +1,23 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Basic User Interface Test: dynamic appearance switching</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://drafts.csswg.org/css-ui-4/#appearance-switching">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1435658">
+<link rel="match" href="appearance-dynamic-001-ref.html">
+
+<p>Should see form controls with their default styling</p>
+
+<input style="-moz-appearance: none; appearance: none;" type="checkbox">
+<input style="-moz-appearance: none; appearance: none;" type="radio">
+<input style="-moz-appearance: none; appearance: none;" type="text">
+<input style="-moz-appearance: none; appearance: none;" type="number">
+<input style="-moz-appearance: none; appearance: none;" type="date">
+
+<script>
+  document.body.offsetTop;
+  for (const el of Array.from(document.querySelectorAll('input'))) {
+    el.style.appearance = "";
+    el.style.MozAppearance = "";
+  }
+</script>