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

@@ -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 ==

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,6 +435,7 @@ void nsComboboxControlFrame::Reflow(nsPresContext* aPresContext,
 
   // The button should occupy the same space as a scrollbar, and its position
   // starts from the border edge.
+  if (mButtonFrame) {
     LogicalRect buttonRect(wm);
     buttonRect.IStart(wm) = borderPadding.IStart(wm) + mMaxDisplayISize;
     buttonRect.BStart(wm) = border.BStart(wm);
@@ -444,6 +445,7 @@ void nsComboboxControlFrame::Reflow(nsPresContext* aPresContext,
 
     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,12 +683,12 @@ 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;
+    if (!mButtonContent) {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
 
     // make someone to listen to the button.
     mButtonContent->SetAttr(kNameSpaceID_None, nsGkAtoms::type, u"button"_ns,
@@ -701,10 +703,8 @@ nsresult nsComboboxControlFrame::CreateAnonymousContent(
                               wm.IsVerticalRL() ? u"left"_ns : u"right"_ns,
                               false);
     }
-
-  // XXX(Bug 1631371) Check if this should use a fallible operation as it
-  // pretended earlier.
     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);
   }
 }

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,21 +2386,15 @@ 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`.
+  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);
   if (mPosition != aNewData.mPosition) {