BoxLayout now suports per-view margins.

ui/views/layout/box_layout.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/layout/box_layout.h"
 
 #include <algorithm>
 
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/view.h"
+#include "ui/views/view_properties.h"
 
 namespace views {
 
+BoxLayout::ViewWrapper::ViewWrapper()
+    : view_(nullptr), layout_(nullptr), orientation_(kHorizontal) {}
+
+BoxLayout::ViewWrapper::ViewWrapper(const BoxLayout* layout, View* view)
+    : view_(view), layout_(layout), orientation_(layout_->orientation_) {
+  gfx::Insets* margins = view_ ? view_->GetProperty(kMarginsKey) : nullptr;
+  if (margins)
+    margins_ = *margins;
+}
+
+BoxLayout::ViewWrapper::~ViewWrapper() {}
+
+int BoxLayout::ViewWrapper::GetHeightForWidth(int width) const {
+  if (layout_->collapse_margins_spacing_)
+    return view_->GetHeightForWidth(width);
+  if (orientation_ == Orientation::kHorizontal) {
+    return view_->GetHeightForWidth(std::max(0, width - margins_.width())) +
+           margins_.height();
+  } else {
+    return view_->GetHeightForWidth(width) + margins_.height();
+  }
+}
+
+gfx::Size BoxLayout::ViewWrapper::GetPreferredSize() const {
+  gfx::Size preferred_size = view_->GetPreferredSize();
+  if (!layout_->collapse_margins_spacing_)
+    preferred_size.Enlarge(margins_.width(), margins_.height());
+  return preferred_size;
+}
+
+void BoxLayout::ViewWrapper::SetBoundsRect(const gfx::Rect& bounds) {
+  gfx::Rect new_bounds = bounds;
+  if (!layout_->collapse_margins_spacing_) {
+    if (orientation_ == Orientation::kHorizontal ||
+        layout_->cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
+      new_bounds.set_x(bounds.x() + margins_.left());
+      new_bounds.set_width(std::max(0, bounds.width() - margins_.width()));
+    }
+    if (orientation_ == Orientation::kVertical ||
+        layout_->cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
+      new_bounds.set_y(bounds.y() + margins_.top());
+      new_bounds.set_height(std::max(0, bounds.height() - margins_.height()));
+    }
+  }
+  view_->SetBoundsRect(new_bounds);
+}
+
+bool BoxLayout::ViewWrapper::visible() const {
+  return view_->visible();
+}
+
 BoxLayout::BoxLayout(BoxLayout::Orientation orientation,
                      int inside_border_horizontal_spacing,
                      int inside_border_vertical_spacing,
-                     int between_child_spacing)
+                     int between_child_spacing,
+                     bool collapse_margins_spacing)
     : orientation_(orientation),
       inside_border_insets_(inside_border_vertical_spacing,
                             inside_border_horizontal_spacing),
       between_child_spacing_(between_child_spacing),
       main_axis_alignment_(MAIN_AXIS_ALIGNMENT_START),
       cross_axis_alignment_(CROSS_AXIS_ALIGNMENT_STRETCH),
       default_flex_(0),
       minimum_cross_axis_size_(0),
-      host_(NULL) {
-}
+      collapse_margins_spacing_(collapse_margins_spacing),
+      host_(NULL) {}
 
 BoxLayout::~BoxLayout() {
 }
 
 void BoxLayout::SetFlexForView(const View* view, int flex_weight) {
   DCHECK(host_);
   DCHECK(view);
   DCHECK_EQ(host_, view->parent());
   DCHECK_GE(flex_weight, 0);
   flex_map_[view] = flex_weight;
 }
 
 void BoxLayout::ClearFlexForView(const View* view) {
   DCHECK(view);
   flex_map_.erase(view);
 }
 
 void BoxLayout::SetDefaultFlex(int default_flex) {
   DCHECK_GE(default_flex, 0);
   default_flex_ = default_flex;
 }
 
 void BoxLayout::Layout(View* host) {
   DCHECK_EQ(host_, host);
-  gfx::Rect child_area(host->GetLocalBounds());
-  child_area.Inset(host->GetInsets());
-  child_area.Inset(inside_border_insets_);
+  gfx::Rect child_area(host->GetContentsBounds());
+
+  AdjustMainAxisForMargin(&child_area);
+  AdjustCrossAxisForInsets(&child_area);
+  gfx::Insets max_cross_axis_margin = CrossAxisMaxViewMargin();
 
   int total_main_axis_size = 0;
   int num_visible = 0;
   int flex_sum = 0;
   // Calculate the total size of children in the main axis.
   for (int i = 0; i < host->child_count(); ++i) {
-    View* child = host->child_at(i);
-    if (!child->visible())
+    ViewWrapper child(this, host->child_at(i));
+    ViewWrapper next(this, NextVisibleView(i));

[sky, 2017/05/25 19:16:51]

move after visible check.

[kylix_rd, 2017/05/25 20:11:19]

Done.

+    if (!child.visible())
       continue;
-    int flex = GetFlexForView(child);
+    int flex = GetFlexForView(child.view());
     int child_main_axis_size = MainAxisSizeForView(child, child_area.width());
     if (child_main_axis_size == 0 && flex == 0)
       continue;
-    total_main_axis_size += child_main_axis_size + between_child_spacing_;
+    total_main_axis_size +=
+        child_main_axis_size + MainAxisMarginBetweenViews(child, next);
     ++num_visible;
     flex_sum += flex;
   }
 
   if (!num_visible)
     return;
 
   total_main_axis_size -= between_child_spacing_;
   // Free space can be negative indicating that the views want to overflow.
   int main_free_space = MainAxisSize(child_area) - total_main_axis_size;
@@ skipping 20 matching lines @@
     gfx::Rect new_child_area(child_area);
     SetMainAxisPosition(position, &new_child_area);
     SetMainAxisSize(size, &new_child_area);
     child_area.Intersect(new_child_area);
   }
 
   int main_position = MainAxisPosition(child_area);
   int total_padding = 0;
   int current_flex = 0;
   for (int i = 0; i < host->child_count(); ++i) {
-    View* child = host->child_at(i);
-    if (!child->visible())
+    ViewWrapper child(this, host->child_at(i));
+    ViewWrapper next(this, NextVisibleView(i));

[sky, 2017/05/25 19:16:51]

move after visible check.

[kylix_rd, 2017/05/25 20:11:19]

Done.

+    if (!child.visible())
       continue;
 
     // TODO(bruthig): Fix this. The main axis should be calculated before
     // the cross axis size because child Views may calculate their cross axis
     // size based on their main axis size. See https://crbug.com/682266.
 
     // Calculate cross axis size.
     gfx::Rect bounds(child_area);
     SetMainAxisPosition(main_position, &bounds);
     if (cross_axis_alignment_ != CROSS_AXIS_ALIGNMENT_STRETCH) {
-      int free_space = CrossAxisSize(bounds) - CrossAxisSizeForView(child);
+      int view_cross_axis_size = CrossAxisSizeForView(child);
+      int cross_axis_margin_width = CrossAxisMarginSizeForView(child);
+      int free_space = CrossAxisSize(bounds) - view_cross_axis_size;
       int position = CrossAxisPosition(bounds);
       if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
-        position += free_space / 2;
+        position += ((free_space + cross_axis_margin_width) / 2) -
+                    CrossAxisMarginTopLeftForView(child);
       } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
-        position += free_space;
+        position += free_space + cross_axis_margin_width;
+      } else {
+        view_cross_axis_size -= cross_axis_margin_width;
       }
       SetCrossAxisPosition(position, &bounds);
-      SetCrossAxisSize(CrossAxisSizeForView(child), &bounds);
+      SetCrossAxisSize(view_cross_axis_size, &bounds);
     }
 
     // Calculate flex padding.
     int current_padding = 0;
-    if (GetFlexForView(child) > 0) {
-      current_flex += GetFlexForView(child);
+    if (GetFlexForView(child.view()) > 0) {
+      current_flex += GetFlexForView(child.view());
       int quot = (main_free_space * current_flex) / flex_sum;
       int rem = (main_free_space * current_flex) % flex_sum;
       current_padding = quot - total_padding;
       // Use the current remainder to round to the nearest pixel.
       if (std::abs(rem) * 2 >= flex_sum)
         current_padding += main_free_space > 0 ? 1 : -1;
       total_padding += current_padding;
     }
 
     // Set main axis size.
     // TODO(bruthig): Use the allocated width to determine the cross axis size.
     // See https://crbug.com/682266.
     int child_main_axis_size = MainAxisSizeForView(child, child_area.width());
     SetMainAxisSize(child_main_axis_size + current_padding, &bounds);
-    if (MainAxisSize(bounds) > 0 || GetFlexForView(child) > 0)
-      main_position += MainAxisSize(bounds) + between_child_spacing_;
+    if (MainAxisSize(bounds) > 0 || GetFlexForView(child.view()) > 0)
+      main_position +=
+          MainAxisSize(bounds) + MainAxisMarginBetweenViews(child, next);
 
     // Clamp child view bounds to |child_area|.
-    bounds.Intersect(child_area);
-    child->SetBoundsRect(bounds);
+    gfx::Rect min_child_area(child_area);
+    if (collapse_margins_spacing_ ||
+        cross_axis_alignment_ != CROSS_AXIS_ALIGNMENT_CENTER)
+      min_child_area.Inset(max_cross_axis_margin);
+    else
+      min_child_area.Inset(max_cross_axis_margin - child.margins());
+    bounds.Intersect(min_child_area);
+    child.SetBoundsRect(bounds);
   }
 
   // Flex views should have grown/shrunk to consume all free space.
   if (flex_sum)
     DCHECK_EQ(total_padding, main_free_space);
 }
 
 gfx::Size BoxLayout::GetPreferredSize(const View* host) const {
   DCHECK_EQ(host_, host);
   // Calculate the child views' preferred width.
   int width = 0;
   if (orientation_ == kVertical) {
-    for (int i = 0; i < host->child_count(); ++i) {
-      const View* child = host->child_at(i);
-      if (!child->visible())
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      if (!child.visible())
         continue;
 
-      width = std::max(width, child->GetPreferredSize().width());
+      gfx::Size child_size = child.GetPreferredSize();
+      width = std::max(width, child_size.width());
     }
     width = std::max(width, minimum_cross_axis_size_);
   }
 
   return GetPreferredSizeForChildWidth(host, width);
 }
 
 int BoxLayout::GetPreferredHeightForWidth(const View* host, int width) const {
   DCHECK_EQ(host_, host);
   int child_width = width - NonChildSize(host).width();
   return GetPreferredSizeForChildWidth(host, child_width).height();
 }
 
 void BoxLayout::Installed(View* host) {
   DCHECK(!host_);
   host_ = host;
 }
 
 void BoxLayout::ViewRemoved(View* host, View* view) {
   ClearFlexForView(view);
 }
 
 int BoxLayout::GetFlexForView(const View* view) const {
-  std::map<const View*, int>::const_iterator it = flex_map_.find(view);
+  FlexMap::const_iterator it = flex_map_.find(view);
   if (it == flex_map_.end())
     return default_flex_;
 
   return it->second;
 }
 
 int BoxLayout::MainAxisSize(const gfx::Rect& rect) const {
   return orientation_ == kHorizontal ? rect.width() : rect.height();
 }
 
@@ skipping 30 matching lines @@
     rect->set_height(size);
 }
 
 void BoxLayout::SetCrossAxisPosition(int position, gfx::Rect* rect) const {
   if (orientation_ == kVertical)
     rect->set_x(position);
   else
     rect->set_y(position);
 }
 
-int BoxLayout::MainAxisSizeForView(const View* view,
+int BoxLayout::MainAxisSizeForView(const ViewWrapper& view,
                                    int child_area_width) const {
   return orientation_ == kHorizontal
-             ? view->GetPreferredSize().width()
-             : view->GetHeightForWidth(cross_axis_alignment_ ==
-                                               CROSS_AXIS_ALIGNMENT_STRETCH
-                                           ? child_area_width
-                                           : view->GetPreferredSize().width());
+             ? view.GetPreferredSize().width()
+             : view.GetHeightForWidth(cross_axis_alignment_ ==
+                                              CROSS_AXIS_ALIGNMENT_STRETCH
+                                          ? child_area_width
+                                          : view.GetPreferredSize().width());
 }
 
-int BoxLayout::CrossAxisSizeForView(const View* view) const {
+gfx::Insets BoxLayout::MaxAxisInsets(Axis axis,
+                                     const gfx::Insets& left1,
+                                     const gfx::Insets& left2,
+                                     const gfx::Insets& right1,
+                                     const gfx::Insets& right2) const {
+  if (axis == HORIZONTAL_AXIS)
+    return gfx::Insets(0, std::max(left1.left(), left2.left()), 0,
+                       std::max(right1.right(), right2.right()));
+  return gfx::Insets(std::max(left1.top(), left2.top()), 0,
+                     std::max(right1.bottom(), right2.bottom()), 0);
+}
+
+int BoxLayout::MainAxisLeadingInset(const gfx::Insets& insets) const {
+  return orientation_ == kHorizontal ? insets.left() : insets.top();

[sky, 2017/05/25 19:16:51]

Much easier to read, thank you!

+}
+
+int BoxLayout::MainAxisTrailingInset(const gfx::Insets& insets) const {
+  return orientation_ == kHorizontal ? insets.right() : insets.bottom();
+}
+
+int BoxLayout::CrossAxisLeadingInset(const gfx::Insets& insets) const {
+  return orientation_ == kVertical ? insets.left() : insets.top();
+}
+
+int BoxLayout::CrossAxisTrailingInset(const gfx::Insets& insets) const {
+  return orientation_ == kVertical ? insets.right() : insets.bottom();
+}
+
+int BoxLayout::MainAxisMarginBetweenViews(const ViewWrapper& left,

[sky, 2017/05/25 19:16:51]

leading/trailing

[kylix_rd, 2017/05/25 20:11:19]

Done.

+                                          const ViewWrapper& right) const {
+  if (!collapse_margins_spacing_ || !left.view() || !right.view())
+    return between_child_spacing_;
+  return std::max(between_child_spacing_,
+                  std::max(MainAxisLeadingInset(left.margins()),
+                           MainAxisTrailingInset(right.margins())));
+}
+
+gfx::Insets BoxLayout::MainAxisOuterMargin() const {
+  if (collapse_margins_spacing_) {
+    const ViewWrapper first(this, FirstVisibleView());
+    const ViewWrapper last(this, LastVisibleView());
+    return MaxAxisInsets(
+        orientation_ == kHorizontal ? HORIZONTAL_AXIS : VERTICAL_AXIS,
+        inside_border_insets_, first.margins(), inside_border_insets_,
+        last.margins());
+  }
+  return MaxAxisInsets(
+      orientation_ == kHorizontal ? HORIZONTAL_AXIS : VERTICAL_AXIS,
+      inside_border_insets_, gfx::Insets(), inside_border_insets_,
+      gfx::Insets());
+}
+
+gfx::Insets BoxLayout::CrossAxisMaxViewMargin() const {
+  int top_or_left = 0;

[sky, 2017/05/25 19:16:51]

leading

[kylix_rd, 2017/05/25 20:11:19]

Done.

+  int bottom_or_right = 0;

[sky, 2017/05/25 19:16:51]

trailing

[kylix_rd, 2017/05/25 20:11:19]

Done.

+  for (int i = 0; i < host_->child_count(); ++i) {
+    const ViewWrapper child(this, host_->child_at(i));
+    if (!child.visible())
+      continue;
+    top_or_left = std::max(top_or_left, CrossAxisLeadingInset(child.margins()));
+
+    bottom_or_right =
+        std::max(bottom_or_right, CrossAxisTrailingInset(child.margins()));
+  }
+  if (orientation_ == Orientation::kVertical)
+    return gfx::Insets(0, top_or_left, 0, bottom_or_right);
+  return gfx::Insets(top_or_left, 0, bottom_or_right, 0);
+}
+
+gfx::Insets BoxLayout::CrossAxisOuterMargin() const {
+  gfx::Insets max_margin = CrossAxisMaxViewMargin();
+  bool is_vertical = orientation_ == Orientation::kVertical;
+  int top_or_left = CrossAxisLeadingInset(inside_border_insets_);

[sky, 2017/05/25 19:16:51]

leading

[kylix_rd, 2017/05/25 20:11:19]

Done.

+  int bottom_or_right = CrossAxisTrailingInset(inside_border_insets_);

[sky, 2017/05/25 19:16:51]

trailing

[kylix_rd, 2017/05/25 20:11:19]

Done.

+  if (collapse_margins_spacing_) {
+    return MaxAxisInsets(is_vertical ? HORIZONTAL_AXIS : VERTICAL_AXIS,

[sky, 2017/05/25 19:16:51]

optional: is_vertical is *super* confusing here. Look at he conditional you
have, it's basically:
if (is_vertical)
  return HORIZONTAL_AXIS;
...
Ugh.

[kylix_rd, 2017/05/25 20:11:19]

Understandable. It's certainly a little more confusing than most of the other
similar conditionals throughout the file where you're dealing with the cross
axis. In those instances, whatever the current orientation is, you return the
*other* axis.

How about this?

Axis cross_axis = orientation_ == kVertical ? HORIZONTAL_AXIS : VERTICAL_AXIS;

then use cross_axis directly in the MaxAxisInsets() call and explicitly test
(cross_axis == HORIZONTAL_AXIS) in the "if" further down? Next patch will
reflect that.

+                         gfx::Insets(top_or_left, bottom_or_right), max_margin,
+                         gfx::Insets(top_or_left, bottom_or_right), max_margin);
+  }
+  if (is_vertical)
+    return gfx::Insets(0, top_or_left + max_margin.left(), 0,
+                       bottom_or_right + max_margin.right());
+  return gfx::Insets(top_or_left + max_margin.top(), 0,
+                     bottom_or_right + max_margin.bottom(), 0);
+}
+
+void BoxLayout::AdjustMainAxisForMargin(gfx::Rect* rect) const {
+  rect->Inset(MainAxisOuterMargin());
+}
+
+void BoxLayout::AdjustCrossAxisForInsets(gfx::Rect* rect) const {
+  rect->Inset(orientation_ == Orientation::kVertical
+                  ? gfx::Insets(0, inside_border_insets_.left(), 0,
+                                inside_border_insets_.right())
+                  : gfx::Insets(inside_border_insets_.top(), 0,
+                                inside_border_insets_.bottom(), 0));
+}
+
+int BoxLayout::CrossAxisSizeForView(const ViewWrapper& view) const {
   // TODO(bruthig): For horizontal case use the available width and not the
   // preferred width. See https://crbug.com/682266.
   return orientation_ == kVertical
-             ? view->GetPreferredSize().width()
-             : view->GetHeightForWidth(view->GetPreferredSize().width());
+             ? view.GetPreferredSize().width()
+             : view.GetHeightForWidth(view.GetPreferredSize().width());
+}
+
+int BoxLayout::CrossAxisMarginSizeForView(const ViewWrapper& view) const {
+  return collapse_margins_spacing_
+             ? 0
+             : (orientation_ == kVertical ? view.margins().width()
+                                          : view.margins().height());
+}
+
+int BoxLayout::CrossAxisMarginTopLeftForView(const ViewWrapper& view) const {
+  return collapse_margins_spacing_ ? 0 : CrossAxisLeadingInset(view.margins());
 }
 
 gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
                                                    int child_area_width) const {
+  DCHECK_EQ(host, host_);
   gfx::Rect child_area_bounds;
 
   if (orientation_ == kHorizontal) {
     // Horizontal layouts ignore |child_area_width|, meaning they mimic the
     // default behavior of GridLayout::GetPreferredHeightForWidth().
     // TODO(estade|bruthig): Fix this See // https://crbug.com/682266.
     int position = 0;
-    for (int i = 0; i < host->child_count(); ++i) {
-      const View* child = host->child_at(i);
-      if (!child->visible())
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      const ViewWrapper next(this, NextVisibleView(i));

[sky, 2017/05/25 19:16:51]

Move after visible and preferred size checks? Same comment in next branch.

[kylix_rd, 2017/05/25 20:11:19]

Done.

+      if (!child.visible())
         continue;
 
-      gfx::Size size(child->GetPreferredSize());
+      gfx::Size size(child.GetPreferredSize());
       if (size.IsEmpty())
         continue;
 
       gfx::Rect child_bounds(position, 0, size.width(), size.height());
       child_area_bounds.Union(child_bounds);
-      position += size.width() + between_child_spacing_;
+      position += size.width() + MainAxisMarginBetweenViews(child, next);
     }
     child_area_bounds.set_height(
         std::max(child_area_bounds.height(), minimum_cross_axis_size_));
   } else {
     int height = 0;
-    for (int i = 0; i < host->child_count(); ++i) {
-      const View* child = host->child_at(i);
-      if (!child->visible())
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      const ViewWrapper next(this, NextVisibleView(i));
+      if (!child.visible())
         continue;
 
       // Use the child area width for getting the height if the child is
       // supposed to stretch. Use its preferred size otherwise.
       int extra_height = MainAxisSizeForView(child, child_area_width);
       // Only add |between_child_spacing_| if this is not the only child.
-      if (height != 0 && extra_height > 0)
-        height += between_child_spacing_;
+      if (next.view() && extra_height > 0)
+        height += MainAxisMarginBetweenViews(child, next);
       height += extra_height;
     }
 
     child_area_bounds.set_width(child_area_width);
     child_area_bounds.set_height(height);
   }
 
-  gfx::Size non_child_size = NonChildSize(host);
+  gfx::Size non_child_size = NonChildSize(host_);

[sky, 2017/05/25 19:16:51]

Won't this result in a layout like option (3). Aren't you effectively adding the
max of the cross axis margins to the max of the preferred size?

[kylix_rd, 2017/05/25 20:11:19]

If the margins aren't collapsed, this will only be the host view's current
insets plus the inside border insets. All the previous calculations have already
taken the margins for each view into account.

For collapsed margins, the views' margins aren't additive and are accounted for
independently by calculating the max for each margin/spacing butting against
each other.

[sky, 2017/05/25 22:37:27]

I think I'm still confused. I left a diagram on the board in your office. I'll
stop by tomorrow to talk about it.

   return gfx::Size(child_area_bounds.width() + non_child_size.width(),
                    child_area_bounds.height() + non_child_size.height());
 }
 
 gfx::Size BoxLayout::NonChildSize(const View* host) const {
   gfx::Insets insets(host->GetInsets());
-  return gfx::Size(insets.width() + inside_border_insets_.width(),
-                   insets.height() + inside_border_insets_.height());
+  if (!collapse_margins_spacing_)
+    return gfx::Size(insets.width() + inside_border_insets_.width(),
+                     insets.height() + inside_border_insets_.height());
+  gfx::Insets main_axis = MainAxisOuterMargin();
+  gfx::Insets cross_axis = CrossAxisOuterMargin();
+  return gfx::Size(insets.width() + main_axis.width() + cross_axis.width(),
+                   insets.height() + main_axis.height() + cross_axis.height());
+}
+
+View* BoxLayout::NextVisibleView(int index) const {
+  for (int i = index + 1; i < host_->child_count(); ++i) {
+    View* result = host_->child_at(i);
+    if (result->visible())
+      return result;
+  }
+  return nullptr;
+}
+
+View* BoxLayout::FirstVisibleView() const {
+  return NextVisibleView(-1);
+}
+
+View* BoxLayout::LastVisibleView() const {
+  for (int i = host_->child_count() - 1; i >= 0; --i) {
+    View* result = host_->child_at(i);
+    if (result->visible())
+      return result;
+  }
+  return nullptr;
 }
 
 }  // namespace views