Bug 1435658: Deal with appearance changes from / to none correctly. r?mats draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 05 Feb 2018 14:55:31 +0100
changeset 751133 8b1ecc9a0b66557030666c78360ef7fc6149efcd
parent 751117 0db1add642754848f7c6105db388f4d0a09180d5
push id97867
push userbmo:emilio@crisal.io
push dateMon, 05 Feb 2018 13:59:41 +0000
reviewersmats
bugs1435658
milestone60.0a1
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>