From d4a37bb7998bcc9ade1a1eb9c96f52810e735667 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Fri, 6 May 2022 14:45:49 +0000
Subject: [PATCH] Bug 1768155 - Allow nsComboboxControlFrame to be laid out
 without a <button>. r=jwatt

Don't generate the button if the combobox doesn't need it anyways (if the
appearance is not menulist).

While at it improve the code that reframes on appearance: textfield changes so
that it only applies to number inputs as it should.

This shouldn't change behavior.

Differential Revision: https://phabricator.services.mozilla.com/D145712
---
 layout/base/nsCSSFrameConstructor.cpp   |  2 +-
 layout/forms/nsComboboxControlFrame.cpp | 57 ++++++++++++-------------
 layout/style/nsStyleStruct.cpp          | 40 +++++++++++------
 3 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp
index 7430d9c02a6a..39052cd2fe54 100644
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -3029,7 +3029,7 @@ nsIFrame* nsCSSFrameConstructor::ConstructSelectFrame(
     DebugOnly<nsresult> rv =
         GetAnonymousContent(content, comboboxFrame, newAnonymousItems);
     MOZ_ASSERT(NS_SUCCEEDED(rv));
-    MOZ_ASSERT(newAnonymousItems.Length() == 2);
+    MOZ_ASSERT(!newAnonymousItems.IsEmpty());
 
     // Manually create a frame for the special NAC.
     MOZ_ASSERT(newAnonymousItems[0].mContent ==
diff --git a/layout/forms/nsComboboxControlFrame.cpp b/layout/forms/nsComboboxControlFrame.cpp
index 400dec98fbe3..25a3f742e77f 100644
--- a/layout/forms/nsComboboxControlFrame.cpp
+++ b/layout/forms/nsComboboxControlFrame.cpp
@@ -406,7 +406,7 @@ void nsComboboxControlFrame::Reflow(nsPresContext* aPresContext,
   // 4) Inline size of display area is whatever is left over from our
   //    inline size after allocating inline size for the button.
 
-  if (!mDisplayFrame || !mButtonFrame) {
+  if (!mDisplayFrame) {
     NS_ERROR("Why did the frame constructor allow this to happen?  Fix it!!");
     return;
   }
@@ -435,15 +435,17 @@ void nsComboboxControlFrame::Reflow(nsPresContext* aPresContext,
 
   // The button should occupy the same space as a scrollbar, and its position
   // starts from the border edge.
-  LogicalRect buttonRect(wm);
-  buttonRect.IStart(wm) = borderPadding.IStart(wm) + mMaxDisplayISize;
-  buttonRect.BStart(wm) = border.BStart(wm);
+  if (mButtonFrame) {
+    LogicalRect buttonRect(wm);
+    buttonRect.IStart(wm) = borderPadding.IStart(wm) + mMaxDisplayISize;
+    buttonRect.BStart(wm) = border.BStart(wm);
 
-  buttonRect.ISize(wm) = buttonISize;
-  buttonRect.BSize(wm) = mDisplayFrame->BSize(wm) + padding.BStartEnd(wm);
+    buttonRect.ISize(wm) = buttonISize;
+    buttonRect.BSize(wm) = mDisplayFrame->BSize(wm) + padding.BStartEnd(wm);
 
-  const nsSize containerSize = aDesiredSize.PhysicalSize();
-  mButtonFrame->SetRect(buttonRect, containerSize);
+    const nsSize containerSize = aDesiredSize.PhysicalSize();
+    mButtonFrame->SetRect(buttonRect, containerSize);
+  }
 
   if (!aStatus.IsInlineBreakBefore() && !aStatus.IsFullyComplete()) {
     // This frame didn't fit inside a fragmentation container.  Splitting
@@ -681,30 +683,28 @@ nsresult nsComboboxControlFrame::CreateAnonymousContent(
   }
   ActuallyDisplayText(false);
 
-  // XXX(Bug 1631371) Check if this should use a fallible operation as it
-  // pretended earlier.
   aElements.AppendElement(mDisplayContent);
+  if (HasDropDownButton()) {
+    mButtonContent = mContent->OwnerDoc()->CreateHTMLElement(nsGkAtoms::button);
+    if (!mButtonContent) {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
 
-  mButtonContent = mContent->OwnerDoc()->CreateHTMLElement(nsGkAtoms::button);
-  if (!mButtonContent) return NS_ERROR_OUT_OF_MEMORY;
-
-  // make someone to listen to the button.
-  mButtonContent->SetAttr(kNameSpaceID_None, nsGkAtoms::type, u"button"_ns,
-                          false);
-  // Set tabindex="-1" so that the button is not tabbable
-  mButtonContent->SetAttr(kNameSpaceID_None, nsGkAtoms::tabindex, u"-1"_ns,
-                          false);
-
-  WritingMode wm = GetWritingMode();
-  if (wm.IsVertical()) {
-    mButtonContent->SetAttr(kNameSpaceID_None, nsGkAtoms::orientation,
-                            wm.IsVerticalRL() ? u"left"_ns : u"right"_ns,
+    // make someone to listen to the button.
+    mButtonContent->SetAttr(kNameSpaceID_None, nsGkAtoms::type, u"button"_ns,
+                            false);
+    // Set tabindex="-1" so that the button is not tabbable
+    mButtonContent->SetAttr(kNameSpaceID_None, nsGkAtoms::tabindex, u"-1"_ns,
                             false);
-  }
 
-  // XXX(Bug 1631371) Check if this should use a fallible operation as it
-  // pretended earlier.
-  aElements.AppendElement(mButtonContent);
+    WritingMode wm = GetWritingMode();
+    if (wm.IsVertical()) {
+      mButtonContent->SetAttr(kNameSpaceID_None, nsGkAtoms::orientation,
+                              wm.IsVerticalRL() ? u"left"_ns : u"right"_ns,
+                              false);
+    }
+    aElements.AppendElement(mButtonContent);
+  }
 
   return NS_OK;
 }
@@ -888,7 +888,6 @@ void nsComboboxControlFrame::SetInitialChildList(ChildListID aListID,
         break;
       }
     }
-    NS_ASSERTION(mButtonFrame, "missing button frame in initial child list");
     nsBlockFrame::SetInitialChildList(aListID, aChildList);
   }
 }
diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
index dc3708f87240..e3413f21ebc2 100644
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -2351,6 +2351,24 @@ static bool ScrollbarGenerationChanged(const nsStyleDisplay& aOld,
          changed(aOld.mOverflowY, aNew.mOverflowY);
 }
 
+static bool AppearanceValueAffectsFrames(StyleAppearance aDefaultAppearance,
+                                         StyleAppearance aAppearance) {
+  switch (aAppearance) {
+    case StyleAppearance::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 this affects the
+      // spinbox creation in nsNumberControlFrame::CreateAnonymousContent.
+      return aDefaultAppearance == StyleAppearance::NumberInput;
+    case StyleAppearance::Menulist:
+      // This affects the menulist button creation.
+      return aDefaultAppearance == StyleAppearance::Menulist;
+    default:
+      return false;
+  }
+}
+
 nsChangeHint nsStyleDisplay::CalcDifference(
     const nsStyleDisplay& aNewData, const nsStylePosition& aOldPosition) const {
   if (mDisplay != aNewData.mDisplay || mContain != aNewData.mContain ||
@@ -2368,20 +2386,14 @@ nsChangeHint nsStyleDisplay::CalcDifference(
 
   auto oldAppearance = EffectiveAppearance();
   auto newAppearance = aNewData.EffectiveAppearance();
-
-  if ((oldAppearance == StyleAppearance::Textfield &&
-       newAppearance != StyleAppearance::Textfield) ||
-      (oldAppearance != StyleAppearance::Textfield &&
-       newAppearance == StyleAppearance::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'.
-    // TODO: Now that we have -moz-default appearance we should do this only if
-    // `mDefaultAppearance` is or was `number-input`.
-    return nsChangeHint_ReconstructFrame;
+  if (oldAppearance != newAppearance) {
+    // Changes to the relevant default appearance changes in
+    // AppearanceValueRequiresFrameReconstruction require reconstruction on
+    // their own, so we can just pick either the new or the old.
+    if (AppearanceValueAffectsFrames(oldAppearance, mDefaultAppearance) ||
+        AppearanceValueAffectsFrames(newAppearance, mDefaultAppearance)) {
+      return nsChangeHint_ReconstructFrame;
+    }
   }
 
   auto hint = nsChangeHint(0);