Better support for height-for-width and custom flex rules.

It improves but does not completely fix support for height-for-width
calculations, allowing views to exceed preferred size in more cases
where a custom flex rule or height-for-width relationship is present,
especially in places where there is a vertical layout with horizontal
stretch alignment.

We have also added a specific regression test case for the bug in
question, which has example code showing how to make the requested case
work with the existing infrastructure.

Bug: 1012119
Change-Id: I88380771aacfea29e750dd5033f1fa11e9eb999a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1940714Reviewed-by: Collin Baker <collinbaker@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721236}

ui/views/layout/flex_layout.cc

@@ -402,20 +402,35 @@ void FlexLayout::InitializeChildData(
     flex_child.internal_padding = Normalize(
         orientation(),
         GetViewProperty(child, layout_defaults_, views::kInternalPaddingKey));
-    flex_child.preferred_size =
-        Normalize(orientation(), child->GetPreferredSize());
+
+    // FlexSpecification defines "preferred size" as the size returned when we
+    // do not bound the inputs (specifically the main axis). This is different
+    // from View::GetPreferredSize() which may not take into account e.g. an
+    // the necessity to alter a view's height in a vertical layout if the width
+    // is bounded. In the common case this will be equivalent to calling
+    // GetPreferredSize().
+    const base::Optional<int> available_cross_size =
+        GetAvailableCrossAxisSize(*data, view_index, bounds);
+
+    // In vertical layouts it's important to consider height-for-width type
+    // calculations when evaluating the base/preferred size of the child view.
+    const base::Optional<int> preferred_cross_size =
+        orientation() == LayoutOrientation::kVertical ? available_cross_size
+                                                      : base::nullopt;
+    flex_child.preferred_size = Normalize(
+        orientation(),
+        flex_child.flex.rule().Run(
+            child,
+            Denormalize(orientation(), {base::nullopt, preferred_cross_size})));
 
     // gfx::Size calculation depends on whether flex is allowed.
     if (main_axis_bounded) {
-      flex_child.available_size = {
-          0, GetAvailableCrossAxisSize(*data, view_index, bounds)};
+      flex_child.available_size = {0, available_cross_size};
       flex_child.current_size = Normalize(
           orientation(),
           flex_child.flex.rule().Run(
               child, Denormalize(orientation(), flex_child.available_size)));
 
-      // We should revisit whether this is a valid assumption for text views
-      // in vertical layouts.
       DCHECK_GE(flex_child.preferred_size.main(),
                 flex_child.current_size.main())
           << " in " << child->GetClassName();

ui/views/layout/flex_layout_types.cc

@@ -61,12 +61,33 @@ int InterpolateSize(MinimumFlexSizeRule minimum_size_rule,
   }
 }
 
+// A view's minimum size can in some cases be expensive to compute. This
+// provides a lazy-eval value that behaves like a smart pointer but is more
+// lightweight than base::LazyInstance.
+class LazyMinimumSize {
+ public:
+  explicit LazyMinimumSize(const View* view) : view_(view) {}
+  ~LazyMinimumSize() = default;
+
+  const gfx::Size* operator->() const { return get(); }
+  const gfx::Size& operator*() const { return *get(); }
+  const gfx::Size* get() const {
+    if (!size_)
+      size_ = view_->GetMinimumSize();
+    return &size_.value();
+  }
+
+ private:
+  const View* const view_;
+  mutable base::Optional<gfx::Size> size_;
+};
+
 gfx::Size GetPreferredSize(MinimumFlexSizeRule minimum_size_rule,
                            MaximumFlexSizeRule maximum_size_rule,
                            bool adjust_height_for_width,
-                           const views::View* view,
-                           const views::SizeBounds& maximum_size) {
-  gfx::Size min = view->GetMinimumSize();
+                           const View* view,
+                           const SizeBounds& maximum_size) {
+  LazyMinimumSize min(view);
   gfx::Size preferred = view->GetPreferredSize();
 
   int width, height;
@@ -76,24 +97,34 @@ gfx::Size GetPreferredSize(MinimumFlexSizeRule minimum_size_rule,
     // size; a view can't grow infinitely, so we go with its preferred size.
     width = preferred.width();
   } else {
-    width = InterpolateSize(minimum_size_rule, maximum_size_rule, min.width(),
+    width = InterpolateSize(minimum_size_rule, maximum_size_rule, min->width(),
                             preferred.width(), *maximum_size.width());
   }
 
   int preferred_height = preferred.height();
-  if (adjust_height_for_width && width < preferred.width()) {
-    // Allow views that need to grow vertically when they're compressed
-    // horizontally to do so.
-    //
-    // If we just went with GetHeightForWidth() we would have situations where
-    // an empty text control wanted no (or very little) height which could cause
-    // a layout to shrink vertically; we will always try to allocate at least
-    // the view's reported preferred height.
-    //
-    // Note that this is an adjustment made for practical considerations, and
-    // may not be "correct" in some absolute sense. Let's revisit at some point.
-    preferred_height =
-        std::max(preferred_height, view->GetHeightForWidth(width));
+  if (adjust_height_for_width) {
+    // The |adjust_height_for_width| flag is used in vertical layouts where we
+    // want views to be able to adapt to the horizontal available space by
+    // growing vertically. We therefore allow the horizontal size to shrink even
+    // if there's otherwise no flex allowed.
+    if (maximum_size.width() && maximum_size.width() > 0)
+      width = std::min(width, *maximum_size.width());
+
+    if (width < preferred.width()) {
+      // Allow views that need to grow vertically when they're compressed
+      // horizontally to do so.
+      //
+      // If we just went with GetHeightForWidth() we would have situations where
+      // an empty text control wanted no (or very little) height which could
+      // cause a layout to shrink vertically; we will always try to allocate at
+      // least the view's reported preferred height.
+      //
+      // Note that this is an adjustment made for practical considerations, and
+      // may not be "correct" in some absolute sense. Let's revisit at some
+      // point.
+      preferred_height =
+          std::max(preferred_height, view->GetHeightForWidth(width));
+    }
   }
 
   if (!maximum_size.height()) {
@@ -101,8 +132,9 @@ gfx::Size GetPreferredSize(MinimumFlexSizeRule minimum_size_rule,
     // size; a view can't grow infinitely, so we go with its preferred size.
     height = preferred_height;
   } else {
-    height = InterpolateSize(minimum_size_rule, maximum_size_rule, min.height(),
-                             preferred_height, *maximum_size.height());
+    height =
+        InterpolateSize(minimum_size_rule, maximum_size_rule, min->height(),
+                        preferred_height, *maximum_size.height());
   }
 
   return gfx::Size(width, height);

ui/views/layout/flex_layout_unittest.cc

@@ -32,6 +32,8 @@ using gfx::Size;
 
 class MockView : public View {
  public:
+  enum class SizeMode { kUsePreferredSize, kFixedArea };
+
   void SetMinimumSize(const Size& minimum_size) {
     minimum_size_ = minimum_size;
   }
@@ -40,6 +42,19 @@ class MockView : public View {
     return minimum_size_.value_or(GetPreferredSize());
   }
 
+  int GetHeightForWidth(int width) const override {
+    DCHECK(width > 0);
+    const gfx::Size preferred = GetPreferredSize();
+    switch (size_mode_) {
+      case SizeMode::kUsePreferredSize:
+        return preferred.height();
+      case SizeMode::kFixedArea:
+        return (preferred.width() * preferred.height()) / width;
+    }
+  }
+
+  void set_size_mode(SizeMode size_mode) { size_mode_ = size_mode; }
+
   void SetVisible(bool visible) override {
     View::SetVisible(visible);
     ++set_visible_count_;
@@ -52,6 +67,7 @@ class MockView : public View {
  private:
   Optional<Size> minimum_size_;
   int set_visible_count_ = 0;
+  SizeMode size_mode_ = SizeMode::kUsePreferredSize;
 };
 
 // Custom flex rule that snaps a view between its preffered size and half that
@@ -110,8 +126,9 @@ class FlexLayoutTest : public testing::Test {
   std::vector<Rect> GetChildBounds() const {
     std::vector<Rect> result;
     std::transform(host_->children().cbegin(), host_->children().cend(),
-                   std::back_inserter(result),
-                   [](const View* v) { return v->bounds(); });
+                   std::back_inserter(result), [](const View* v) {
+                     return v->GetVisible() ? v->bounds() : gfx::Rect();
+                   });
     return result;
   }
 
@@ -731,6 +748,188 @@ TEST_F(FlexLayoutTest, LayoutMultipleViews_InteriorPadding_Additive) {
   EXPECT_EQ(Size(70, 50), host_->GetPreferredSize());
 }
 
+// Height-for-width tests ------------------------------------------------------
+
+namespace {
+
+// Returns a flex specification which uses preferred size but allows
+// height-for-width flex.
+FlexSpecification GetHeightForWidthFlexSpecification() {
+  return FlexSpecification::ForSizeRule(MinimumFlexSizeRule::kPreferred,
+                                        MaximumFlexSizeRule::kPreferred, true)
+      .WithWeight(0);
+}
+
+}  // anonymous namespace
+
+TEST_F(FlexLayoutTest, HeightForWidth_Vertical_CrossStart) {
+  layout_->SetOrientation(LayoutOrientation::kVertical);
+  layout_->SetMainAxisAlignment(LayoutAlignment::kStart);
+  layout_->SetCrossAxisAlignment(LayoutAlignment::kStart);
+  layout_->SetDefault(kMarginsKey, gfx::Insets(5));
+  layout_->SetDefault(kFlexBehaviorKey, GetHeightForWidthFlexSpecification());
+  AddChild({10, 10})->set_size_mode(MockView::SizeMode::kFixedArea);
+  AddChild({10, 10});
+
+  EXPECT_EQ(gfx::Size(20, 40), host_->GetPreferredSize());
+  EXPECT_EQ(40, host_->GetHeightForWidth(26));
+  EXPECT_EQ(40, host_->GetHeightForWidth(20));
+  EXPECT_EQ(46, host_->GetHeightForWidth(16));
+
+  host_->SizeToPreferredSize();
+  std::vector<gfx::Rect> expected{{5, 5, 10, 10}, {5, 25, 10, 10}};
+  EXPECT_EQ(expected, GetChildBounds());
+  host_->SetSize({26, 50});
+  EXPECT_EQ(expected, GetChildBounds());
+  host_->SetSize({20, 50});
+  EXPECT_EQ(expected, GetChildBounds());
+  host_->SetSize({16, 50});
+  expected = {{5, 5, 6, 16}, {5, 31, 6, 10}};
+  EXPECT_EQ(expected, GetChildBounds());
+}
+
+TEST_F(FlexLayoutTest,
+       HeightForWidth_Vertical_CrossStretch_WidthChangesHeight) {
+  layout_->SetOrientation(LayoutOrientation::kVertical);
+  layout_->SetMainAxisAlignment(LayoutAlignment::kStart);
+  layout_->SetCrossAxisAlignment(LayoutAlignment::kStretch);
+  layout_->SetDefault(kMarginsKey, gfx::Insets(5));
+  layout_->SetDefault(kFlexBehaviorKey, GetHeightForWidthFlexSpecification());
+  AddChild({10, 10})->set_size_mode(MockView::SizeMode::kFixedArea);
+  AddChild({10, 10});
+
+  EXPECT_EQ(gfx::Size(20, 40), host_->GetPreferredSize());
+  EXPECT_EQ(40, host_->GetHeightForWidth(26));
+  EXPECT_EQ(40, host_->GetHeightForWidth(20));
+  EXPECT_EQ(46, host_->GetHeightForWidth(16));
+
+  host_->SizeToPreferredSize();
+  std::vector<gfx::Rect> expected{{5, 5, 10, 10}, {5, 25, 10, 10}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  host_->SetSize({26, 50});
+  expected = {{5, 5, 16, 10}, {5, 25, 16, 10}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  host_->SetSize({20, 50});
+  expected = {{5, 5, 10, 10}, {5, 25, 10, 10}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  host_->SetSize({16, 50});
+  expected = {{5, 5, 6, 16}, {5, 31, 6, 10}};
+  EXPECT_EQ(expected, GetChildBounds());
+}
+
+TEST_F(FlexLayoutTest, HeightForWidth_Vertical_CrossStretch_FlexPreferredSize) {
+  layout_->SetOrientation(LayoutOrientation::kVertical);
+  layout_->SetMainAxisAlignment(LayoutAlignment::kStart);
+  layout_->SetCrossAxisAlignment(LayoutAlignment::kStretch);
+  layout_->SetDefault(kMarginsKey, gfx::Insets(5));
+  layout_->SetDefault(
+      kFlexBehaviorKey,
+      FlexSpecification::ForSizeRule(MinimumFlexSizeRule::kScaleToZero,
+                                     MaximumFlexSizeRule::kUnbounded, true));
+  AddChild({10, 10})->set_size_mode(MockView::SizeMode::kFixedArea);
+  AddChild({10, 10});
+
+  EXPECT_EQ(gfx::Size(20, 40), host_->GetPreferredSize());
+  EXPECT_EQ(40, host_->GetHeightForWidth(26));
+  EXPECT_EQ(40, host_->GetHeightForWidth(20));
+  EXPECT_EQ(46, host_->GetHeightForWidth(16));
+
+  host_->SizeToPreferredSize();
+  host_->Layout();
+  std::vector<gfx::Rect> expected{{5, 5, 10, 10}, {5, 25, 10, 10}};
+  EXPECT_EQ(expected, GetChildBounds());
+}
+
+TEST_F(FlexLayoutTest, HeightForWidth_Vertical_CrossStretch_FlexLarger) {
+  layout_->SetOrientation(LayoutOrientation::kVertical);
+  layout_->SetMainAxisAlignment(LayoutAlignment::kStart);
+  layout_->SetCrossAxisAlignment(LayoutAlignment::kStretch);
+  layout_->SetDefault(kMarginsKey, gfx::Insets(5));
+  layout_->SetDefault(
+      kFlexBehaviorKey,
+      FlexSpecification::ForSizeRule(MinimumFlexSizeRule::kScaleToZero,
+                                     MaximumFlexSizeRule::kUnbounded, true));
+  AddChild({10, 10})->set_size_mode(MockView::SizeMode::kFixedArea);
+  AddChild({10, 10});
+
+  host_->SetSize({26, 50});
+  host_->Layout();
+  std::vector<gfx::Rect> expected{{5, 5, 16, 15}, {5, 30, 16, 15}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  host_->SetSize({20, 50});
+  host_->Layout();
+  expected = {{5, 5, 10, 15}, {5, 30, 10, 15}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  // TODO(crbug.com/1012134): This should be {{5, 5, 6, 18}, {5, 31, 6, 12}} but
+  // we don't correctly distribute deficit or excess from preferred size across
+  // child views.
+  host_->SetSize({16, 50});
+  host_->Layout();
+  expected = {{5, 5, 6, 15}, {5, 30, 6, 15}};
+  EXPECT_EQ(expected, GetChildBounds());
+}
+
+TEST_F(FlexLayoutTest, HeightForWidth_Vertical_CrossStretch_FlexSmaller) {
+  layout_->SetOrientation(LayoutOrientation::kVertical);
+  layout_->SetMainAxisAlignment(LayoutAlignment::kStart);
+  layout_->SetCrossAxisAlignment(LayoutAlignment::kStretch);
+  layout_->SetDefault(kMarginsKey, gfx::Insets(5));
+  layout_->SetDefault(
+      kFlexBehaviorKey,
+      FlexSpecification::ForSizeRule(MinimumFlexSizeRule::kScaleToZero,
+                                     MaximumFlexSizeRule::kUnbounded, true));
+  AddChild({10, 10})->set_size_mode(MockView::SizeMode::kFixedArea);
+  AddChild({10, 10});
+
+  EXPECT_EQ(gfx::Size(20, 40), host_->GetPreferredSize());
+  EXPECT_EQ(40, host_->GetHeightForWidth(26));
+  EXPECT_EQ(40, host_->GetHeightForWidth(20));
+  EXPECT_EQ(46, host_->GetHeightForWidth(16));
+
+  host_->SetSize({26, 30});
+  host_->Layout();
+  std::vector<gfx::Rect> expected{{5, 5, 16, 5}, {5, 20, 16, 5}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  host_->SetSize({20, 30});
+  host_->Layout();
+  expected = {{5, 5, 10, 5}, {5, 20, 10, 5}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  // TODO(crbug.com/1012134): This should be {{5, 5, 6, 8}, {5, 18, 6, 2}} but
+  // we don't correctly distribute deficit from preferred size across child
+  // views.
+  host_->SetSize({16, 30});
+  host_->Layout();
+  expected = {{5, 5, 6, 5}, {5, 20, 6, 5}};
+  EXPECT_EQ(expected, GetChildBounds());
+}
+
+TEST_F(FlexLayoutTest, HeightForWidth_Horizontal_PreferredSize) {
+  layout_->SetOrientation(LayoutOrientation::kVertical);
+  layout_->SetMainAxisAlignment(LayoutAlignment::kStart);
+  layout_->SetCrossAxisAlignment(LayoutAlignment::kStart);
+  layout_->SetDefault(
+      kFlexBehaviorKey,
+      FlexSpecification::ForSizeRule(MinimumFlexSizeRule::kScaleToZero,
+                                     MaximumFlexSizeRule::kUnbounded, true));
+  MockView* const child = AddChild({10, 10});
+  child->set_size_mode(MockView::SizeMode::kFixedArea);
+
+  // In horizontal views, the height can expand if a child is compressed
+  // horizontally and uses a height-for-width calculation, but it cannot
+  // contract (lest we have zero-height views in some cases).
+  EXPECT_EQ(gfx::Size(10, 10), host_->GetPreferredSize());
+  EXPECT_EQ(10, host_->GetHeightForWidth(10));
+  EXPECT_EQ(10, host_->GetHeightForWidth(20));
+  EXPECT_EQ(20, host_->GetHeightForWidth(5));
+}
+
 // Host insets tests -----------------------------------------------------------
 
 TEST_F(FlexLayoutTest, Layout_HostInsets_Horizontal) {
@@ -1413,7 +1612,7 @@ TEST_F(FlexLayoutTest,
 
   host_->SetSize(Size(20, 19));
   host_->Layout();
-  // TODO(dfried): Make this work.
+  // TODO(crbug.com/1012134): Make this work.
   // EXPECT_EQ(Size(10, 9), child1->size());
   EXPECT_EQ(Size(10, 7), child1->size());
   EXPECT_FALSE(child2->GetVisible());
@@ -1514,7 +1713,7 @@ TEST_F(FlexLayoutTest, Layout_Flex_TwoChildViews_UnequalWeight_SecondAtMax) {
   host_->SetSize(Size(50, 20));
   host_->Layout();
 
-  // TODO(dfried): Make this work.
+  // TODO(crbug.com/1012134): Make this work.
   // EXPECT_EQ(Size(15, 10), child1->size());
   EXPECT_EQ(Size(14, 10), child1->size());
   EXPECT_EQ(Size(20, 10), child2->size());
@@ -2111,6 +2310,48 @@ TEST_F(FlexLayoutTest, Between_Child_Spacing_With_Margins_Collapsed) {
   EXPECT_EQ(gfx::Rect(25, 7, 10, 86), v2->bounds());
 }
 
+// Specific Regression Cases ---------------------------------------------------
+
+// Test case (and example code) for crbug.com/1012119.
+TEST_F(FlexLayoutTest, FlexRuleContradictsPreferredSize) {
+  const FlexSpecification custom_spec = FlexSpecification::ForCustomRule(
+      base::BindRepeating([](const View*, const SizeBounds& maximum_size) {
+        return !maximum_size.width() || *maximum_size.width() >= 100
+                   ? gfx::Size(100, 100)
+                   : gfx::Size(0, 100);
+      }));
+
+  const FlexSpecification other_spec = FlexSpecification::ForSizeRule(
+      MinimumFlexSizeRule::kScaleToZero, MaximumFlexSizeRule::kUnbounded);
+
+  layout_->SetOrientation(LayoutOrientation::kHorizontal);
+  layout_->SetCrossAxisAlignment(LayoutAlignment::kStretch);
+  View* const v1 = AddChild(gfx::Size(7, 7));
+  View* const v2 = AddChild(gfx::Size(7, 7));
+  v1->SetProperty(kFlexBehaviorKey, custom_spec.WithOrder(1));
+  v2->SetProperty(kFlexBehaviorKey, other_spec.WithOrder(2));
+
+  host_->SetSize({200, 100});
+  std::vector<gfx::Rect> expected{{0, 0, 100, 100}, {100, 0, 100, 100}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  host_->SetSize({101, 100});
+  expected = {{0, 0, 100, 100}, {100, 0, 1, 100}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  host_->SetSize({100, 100});
+  expected = {{0, 0, 100, 100}, {}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  host_->SetSize({99, 100});
+  expected = {{}, {0, 0, 99, 100}};
+  EXPECT_EQ(expected, GetChildBounds());
+
+  host_->SetSize({1, 100});
+  expected = {{}, {0, 0, 1, 100}};
+  EXPECT_EQ(expected, GetChildBounds());
+}
+
 // Cross-axis Fit Tests --------------------------------------------------------
 
 // Tests for cross-axis alignment that checks three different conditions: