Bug 1561794 - Do not crop display text of themed comboboxes due to padding. r=dbaron a=RyanVM
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 20 Jul 2019 14:54:33 +0000
changeset 544871 d6feaa7bd4aedf27188fe706c9b05da8b8788678
parent 544870 0ea23ef025191a46847a96f77706b2cfe3544179
child 544872 648715eb3f46e6264b04ad2372a47d257868f8b3
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron, RyanVM
bugs1561794
milestone69.0
Bug 1561794 - Do not crop display text of themed comboboxes due to padding. r=dbaron a=RyanVM This is a potential fix that I thought it was worth doing rather than implementing Blink's platform-dependent silliness. This ensures that the display frame always has enough space to display itself. Note that it may still get clipped, if there's no room for both the display frame and the button. Differential Revision: https://phabricator.services.mozilla.com/D37922
dom/base/test/test_bug320799.html
layout/forms/nsComboboxControlFrame.cpp
layout/forms/nsComboboxControlFrame.h
layout/generic/ReflowInput.h
layout/reftests/forms/select/reftest.list
layout/reftests/forms/select/themed-select-padding-no-clip-ref.html
layout/reftests/forms/select/themed-select-padding-no-clip.html
layout/style/res/forms.css
--- a/dom/base/test/test_bug320799.html
+++ b/dom/base/test/test_bug320799.html
@@ -47,23 +47,28 @@ https://bugzilla.mozilla.org/show_bug.cg
     <option>x</option>
     <option>x</option>
     <option>x</option>
     <option>x</option>
   </select>
   <select id="s3">
     <option>x</option>
   </select>
+  <select id="s4" style="width: 100px; box-sizing: border-box; border: 0; margin: 10px">
+    <option>This is a test, it really is a test I tell you</option>
+  </select>
 </p>
 <div id="content" style="display: none">
   
 </div>
 <pre id="test">
 <script type="application/javascript">
 
 /** Test for Bug 320799 **/
 is($("s").scrollWidth, 100, "Scroll width should not include dropdown contents");
 is($("s2").clientWidth, $("s3").clientWidth,
    "Client width should not depend on the dropdown's vertical scrollbar");
+
+is($("s4").scrollWidth, 100, "Scroll width should not include dropdown contents");
 </script>
 </pre>
 </body>
 </html>
--- a/layout/forms/nsComboboxControlFrame.cpp
+++ b/layout/forms/nsComboboxControlFrame.cpp
@@ -223,16 +223,17 @@ static int32_t gReflowInx = -1;
 nsComboboxControlFrame::nsComboboxControlFrame(ComputedStyle* aStyle,
                                                nsPresContext* aPresContext)
     : nsBlockFrame(aStyle, aPresContext, kClassID),
       mDisplayFrame(nullptr),
       mButtonFrame(nullptr),
       mDropdownFrame(nullptr),
       mListControlFrame(nullptr),
       mDisplayISize(0),
+      mMaxDisplayISize(0),
       mRecentSelectedIndex(NS_SKIP_NOTIFY_INDEX),
       mDisplayedIndex(-1),
       mLastDropDownBeforeScreenBCoord(nscoord_MIN),
       mLastDropDownAfterScreenBCoord(nscoord_MIN),
       mDroppedDown(false),
       mInRedisplayText(false),
       mDelayedShowDropDown(false),
       mIsOpenInParentProcess(false){REFLOW_COUNTER_INIT()}
@@ -823,16 +824,19 @@ void nsComboboxControlFrame::Reflow(nsPr
         PresContext(), aReflowInput.mRenderingContext, wm);
     if (buttonISize > aReflowInput.ComputedISize()) {
       buttonISize = 0;
     }
   }
 
   mDisplayISize = aReflowInput.ComputedISize() - buttonISize;
 
+  mMaxDisplayISize =
+      mDisplayISize + aReflowInput.ComputedLogicalPadding().IEnd(wm);
+
   nsBlockFrame::Reflow(aPresContext, aDesiredSize, aReflowInput, aStatus);
 
   // The button should occupy the same space as a scrollbar
   nsSize containerSize = aDesiredSize.PhysicalSize();
   LogicalRect buttonRect = mButtonFrame->GetLogicalRect(containerSize);
 
   buttonRect.IStart(wm) =
       aReflowInput.ComputedLogicalBorderPadding().IStartEnd(wm) +
@@ -1240,22 +1244,34 @@ void nsComboboxDisplayFrame::Reflow(nsPr
     // (This frame has 'line-height: -moz-block-height' in the UA
     // sheet which is suitable when there's a specified block-size.)
     auto lh = ReflowInput::CalcLineHeight(mComboBox->GetContent(),
                                           mComboBox->Style(), aPresContext,
                                           NS_UNCONSTRAINEDSIZE, inflation);
     state.SetComputedBSize(lh);
   }
   WritingMode wm = aReflowInput.GetWritingMode();
-  nscoord computedISize = mComboBox->mDisplayISize -
-                          state.ComputedLogicalBorderPadding().IStartEnd(wm);
-  if (computedISize < 0) {
-    computedISize = 0;
+  nscoord inlineBp = state.ComputedLogicalBorderPadding().IStartEnd(wm);
+  nscoord computedISize = mComboBox->mDisplayISize - inlineBp;
+
+  // Other UAs ignore padding in some (but not all) platforms for (themed only)
+  // comboboxes. Instead of doing that, we prevent that padding if present from
+  // clipping the display text, by enforcing the display text minimum size in
+  // that situation.
+  const bool shouldHonorMinISize =
+      mComboBox->StyleDisplay()->mAppearance == StyleAppearance::Menulist;
+  if (shouldHonorMinISize) {
+    computedISize = std::max(state.ComputedMinISize(), computedISize);
+    // Don't let this size go over mMaxDisplayISize, since that'd be
+    // observable via clientWidth / scrollWidth.
+    computedISize =
+        std::min(computedISize, mComboBox->mMaxDisplayISize - inlineBp);
   }
-  state.SetComputedISize(computedISize);
+
+  state.SetComputedISize(std::max(0, computedISize));
   nsBlockFrame::Reflow(aPresContext, aDesiredSize, state, aStatus);
   aStatus.Reset();  // this type of frame can't be split
 }
 
 void nsComboboxDisplayFrame::BuildDisplayList(nsDisplayListBuilder* aBuilder,
                                               const nsDisplayListSet& aLists) {
   nsDisplayListCollection set(aBuilder);
   nsBlockFrame::BuildDisplayList(aBuilder, set);
--- a/layout/forms/nsComboboxControlFrame.h
+++ b/layout/forms/nsComboboxControlFrame.h
@@ -284,16 +284,21 @@ class nsComboboxControlFrame final : pub
   nsContainerFrame* mDisplayFrame;     // frame to display selection
   nsIFrame* mButtonFrame;              // button frame
   nsIFrame* mDropdownFrame;            // dropdown list frame
   nsListControlFrame* mListControlFrame;  // ListControl for the dropdown frame
 
   // The inline size of our display area.  Used by that frame's reflow
   // to size to the full inline size except the drop-marker.
   nscoord mDisplayISize;
+  // The maximum inline size of our display area, which is the
+  // nsComoboxControlFrame's border-box.
+  //
+  // Going over this would be observable via DOM APIs like client / scrollWidth.
+  nscoord mMaxDisplayISize;
 
   nsRevocableEventPtr<RedisplayTextEvent> mRedisplayTextEvent;
 
   int32_t mRecentSelectedIndex;
   int32_t mDisplayedIndex;
   nsString mDisplayedOptionTextOrPreview;
 
   // make someone to listen to the button. If its programmatically pressed by
--- a/layout/generic/ReflowInput.h
+++ b/layout/generic/ReflowInput.h
@@ -624,17 +624,18 @@ struct ReflowInput : public SizeComputat
   nscoord mComputedHeight;
 
   // Computed values for 'left/top/right/bottom' offsets. Only applies to
   // 'positioned' elements. These are PHYSICAL coordinates (for now).
   nsMargin mComputedOffsets;
 
   // Computed values for 'min-width/max-width' and 'min-height/max-height'
   // XXXldb The width ones here should go; they should be needed only
-  // internally.
+  // internally, except for nsComboboxDisplayFrame, which still wants to honor
+  // min-inline-size even though it wants to trump inline-size.
   MOZ_INIT_OUTSIDE_CTOR
   nscoord mComputedMinWidth, mComputedMaxWidth;
   MOZ_INIT_OUTSIDE_CTOR
   nscoord mComputedMinHeight, mComputedMaxHeight;
 
  public:
   // Our saved containing block dimensions.
   LogicalSize mContainingBlockSize = LogicalSize(mWritingMode);
--- a/layout/reftests/forms/select/reftest.list
+++ b/layout/reftests/forms/select/reftest.list
@@ -9,8 +9,13 @@ fuzzy(0-1,0-4) == padding-button-placeme
 fuzzy-if(skiaContent,0-4,0-1) needs-focus == focusring-1.html focusring-1-ref.html
 needs-focus == focusring-2.html focusring-2-ref.html
 needs-focus == focusring-3.html focusring-3-ref.html
 == dynamic-text-indent-1.html dynamic-text-indent-1-ref.html
 == dynamic-text-overflow-1.html dynamic-text-overflow-1-ref.html
 == listbox-zero-row-initial.html listbox-zero-row-initial-ref.html
 == 1246836.html 1246836-ref.html
 skip-if(Android) == select-option-display-none-inline-size.html select-option-display-none-inline-size-ref.html
+
+# Android and Windows actually use the anonymous select > button (rather than
+# drawing the arrow as a background like Linux and Mac), so most of this test
+# doesn't apply since when over-constrained it gets zero-sized.
+skip-if(Android||winWidget) == themed-select-padding-no-clip.html themed-select-padding-no-clip-ref.html
new file mode 100644
--- /dev/null
+++ b/layout/reftests/forms/select/themed-select-padding-no-clip-ref.html
@@ -0,0 +1,37 @@
+<!doctype html>
+<title>Bug 1561794 - padding-inline-end does not clip display text for themed comboboxes.</title>
+<style>
+  select, .unthemed {
+    font: 16px / 1 monospace;
+    width: 20ch;
+    display: block;
+  }
+  .unthemed {
+    box-sizing: border-box;
+    border: 1px solid grey;
+  }
+</style>
+<!--- Ensure the text isn't chopped -->
+<select><option>XXXXXXXXXXXX</option></select>
+<select><option>X XXXXXXXXXX</option></select>
+
+<!--- Test that the display text doesn't overflow the select. -->
+<select style="overflow: hidden"><option>XXXXXXXXXXXXXXXXX</option></select>
+
+<!-- Test that this only affects themed comboboxes -->
+<div class="unthemed" style="padding-right: 15ch"><div style="padding: 1px 4px; overflow: hidden;">XXXXXXXXXX</div></div>
+
+<select id="src" style="padding-left: 10ch; padding-right: 10ch"><option>XXXXXXXXXX</option></select>
+<select id="ref" style="padding-left: 10ch;"><option>XXXXXXXXXX</option></select>
+<script>
+  // This one is a bit special, because we want `ref` to have the same size as
+  // `src`, which for some reason, with or without the patch for this bug,
+  // grows outside the specified width. Given this width depends on scrollbar,
+  // width, which is platform dependent, we just calculate it from `src`, and
+  // then hide it.
+  //
+  // We just really want to test that the padding right doesn't clip the text
+  // anyhow.
+  ref.style.width = getComputedStyle(src).width;
+  src.style.display = "none";
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/forms/select/themed-select-padding-no-clip.html
@@ -0,0 +1,26 @@
+<!doctype html>
+<title>Bug 1561794 - padding-inline-end does not clip display text for themed comboboxes.</title>
+<style>
+  select {
+    font: 16px / 1 monospace;
+    width: 20ch;
+    display: block;
+  }
+  .unthemed {
+    -moz-appearance: none;
+    -webkit-appearance: none;
+    border: 1px solid grey;
+    background: transparent;
+  }
+</style>
+<!--- Ensure the text isn't chopped -->
+<select style="padding-right: 15ch"><option>XXXXXXXXXXXX</option></select>
+<select style="padding-right: 15ch"><option>X XXXXXXXXXX</option></select>
+
+<!--- Test that the display text doesn't overflow the select. It'd be nice to test it doesn't overlap the button (which is true), but that seems hard to test via a reftest -->
+<select style="padding-right: 15ch"><option>XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX</option></select>
+
+<!-- Test that this only affects themed comboboxes; 4px is the padding-inline-start of the comboboxcontrol-frame anon box, so as to clip exactly at a character boundary. -->
+<select class="unthemed" style="color: initial; padding-right: 15ch"><option>XXXXXXXXXX</option></select>
+
+<select style="padding-left: 10ch; padding-right: 10ch"><option>XXXXXXXXXX</option></select>
--- a/layout/style/res/forms.css
+++ b/layout/style/res/forms.css
@@ -329,18 +329,20 @@ select > button[orientation="right"]:act
   padding-block-end: 1px;
   padding-inline-start: 4px;
   padding-inline-end: 4px;
   color: unset;
   white-space: nowrap;
   text-align: unset;
   user-select: none;
   /* Make sure to size correctly if the combobox has a non-auto block-size. */
-  block-size: 100% ! important;
-  box-sizing: border-box ! important;
+  block-size: 100%;
+  /* Try to always display our own text */
+  min-inline-size: max-content;
+  box-sizing: border-box;
   line-height: -moz-block-height;
 }
 
 option {
   display: block;
   float: none !important;
   position: static !important;
   min-block-size: 1em;