BoxLayout now suports per-view margins.

ui/views/layout/box_layout.cc

Index: ui/views/layout/box_layout.cc
diff --git a/ui/views/layout/box_layout.cc b/ui/views/layout/box_layout.cc
index 7e5abff4880af33a0b34583c3d0c7f127b7480c5..2f2d9b30ea526997dca108ba24cd0fda4ac56b06 100644
--- a/ui/views/layout/box_layout.cc
+++ b/ui/views/layout/box_layout.cc
@@ -11,10 +11,50 @@
 
 namespace views {
 
+BoxLayout::ViewWrapper::ViewWrapper() : view_(nullptr), layout_(nullptr) {}
+
+BoxLayout::ViewWrapper::ViewWrapper(const BoxLayout* layout, View* view)
+    : view_(view), layout_(layout) {
+  gfx::Insets* margins = view_ ? view_->GetProperty(kMarginsKey) : nullptr;
+  if (margins)
+    margins_ = *margins;
+}
+
+BoxLayout::ViewWrapper::~ViewWrapper() {}
+
+int BoxLayout::ViewWrapper::GetHeightForWidth(int width) const {
+  return layout_->collapse_margins_spacing_
+             ? view_->GetHeightForWidth(width)
+             : view_->GetHeightForWidth(std::max(0, width - margins_.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) {
+  if (layout_->collapse_margins_spacing_) {
+    view_->SetBounds(bounds.x(), bounds.y(), bounds.width(), bounds.height());

[sky, 2017/05/15 20:29:09]

view_->SetBoundsRect(bounds)

[kylix_rd, 2017/05/16 18:45:47]

Done.

+  } else {
+    view_->SetBounds(bounds.x() + margins_.left(), bounds.y() + margins_.top(),
+                     std::max(0, bounds.width() - margins_.width()),
+                     std::max(0, bounds.height() - margins_.height()));
+  }
+}
+
+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),
@@ -23,8 +63,8 @@ BoxLayout::BoxLayout(BoxLayout::Orientation orientation,
       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() {
 }
@@ -49,23 +89,29 @@ void BoxLayout::SetDefaultFlex(int 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());
+  if (collapse_margins_spacing_) {
+    AdjustMainAxisForMargin(&child_area);
+    AdjustCrossAxisForMargin(&child_area);
+  } else {
+    child_area.Inset(inside_border_insets_);
+  }
 
   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));
+    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;
   }
@@ -106,8 +152,9 @@ void BoxLayout::Layout(View* host) {
   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));
+    if (!child.visible())
       continue;
 
     // TODO(bruthig): Fix this. The main axis should be calculated before
@@ -131,8 +178,8 @@ void BoxLayout::Layout(View* host) {
 
     // 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;
@@ -147,12 +194,13 @@ void BoxLayout::Layout(View* host) {
     // 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);
+    child.SetBoundsRect(bounds);
   }
 
   // Flex views should have grown/shrunk to consume all free space.
@@ -165,12 +213,12 @@ gfx::Size BoxLayout::GetPreferredSize(const View* host) const {
   // 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());
+      width = std::max(width, child.GetPreferredSize().width());
     }
     width = std::max(width, minimum_cross_axis_size_);
   }
@@ -194,7 +242,7 @@ void BoxLayout::ViewRemoved(View* host, View* 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_;
 
@@ -245,26 +293,88 @@ void BoxLayout::SetCrossAxisPosition(int position, gfx::Rect* rect) const {
     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::MainAxisMarginBetweenViews(const ViewWrapper& left,
+                                          const ViewWrapper& right) const {
+  return orientation_ == Orientation::kHorizontal
+             ? (collapse_margins_spacing_ && left.view() && right.view()
+                    ? std::max(between_child_spacing_,

[sky, 2017/05/15 20:29:09]

optional: this code is nigh unreadable because of the excessive ternary
operators. Peter may disagree as he uses the ternary operator a lot, so I'm
leaving this optional. Specifically I would rather you use multiple 'ifs.' I'm
ok with one ternary operator, but 3 ternary operators is not particularly
readable.

Maybe checking if either view is null or !collapse_margins_spacing_ first would
help, e.g.
if (!collapse_margins_spacing_ || !left.view() || !right.right())
  return between_child_spacing_;

[kylix_rd, 2017/05/16 18:45:47]

What? You don't appreciate my "mad ternary operator skillz" :)?

Yes, a little rework would make this more readable...

Done.

+                               std::max(left.margins().right(),
+                                        right.margins().left()))
+                    : between_child_spacing_)
+             : (collapse_margins_spacing_ && left.view() && right.view()
+                    ? std::max(between_child_spacing_,
+                               std::max(left.margins().bottom(),
+                                        right.margins().top()))
+                    : between_child_spacing_);
+}
+
+gfx::Insets BoxLayout::MainAxisOuterMargin() const {
+  const ViewWrapper first(this, FirstVisibleView());
+  const ViewWrapper last(this, LastVisibleView());
+  if (orientation_ == Orientation::kHorizontal) {
+    return gfx::Insets(
+        0, std::max(inside_border_insets_.left(), first.margins().left()), 0,
+        std::max(inside_border_insets_.right(), last.margins().right()));
+  } else {
+    return gfx::Insets(
+        std::max(inside_border_insets_.top(), first.margins().top()), 0,
+        std::max(inside_border_insets_.bottom(), last.margins().bottom()), 0);
+  }
+}
+
+gfx::Insets BoxLayout::CrossAxisOuterMargin() const {
+  int top_or_left;
+  int bottom_or_right;
+  bool is_vertical = orientation_ == Orientation::kVertical;
+  top_or_left =
+      is_vertical ? inside_border_insets_.left() : inside_border_insets_.top();
+  bottom_or_right = is_vertical ? inside_border_insets_.right()
+                                : inside_border_insets_.bottom();
+  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, is_vertical ? child.margins().left()
+                                                    : child.margins().top());
+    bottom_or_right =
+        std::max(bottom_or_right, is_vertical ? child.margins().right()
+                                              : child.margins().bottom());
+  }
+  if (is_vertical)
+    return gfx::Insets(0, top_or_left, 0, bottom_or_right);
+  else
+    return gfx::Insets(top_or_left, 0, bottom_or_right, 0);
+}
+
+void BoxLayout::AdjustMainAxisForMargin(gfx::Rect* rect) const {
+  rect->Inset(MainAxisOuterMargin());
+}
+
+void BoxLayout::AdjustCrossAxisForMargin(gfx::Rect* rect) const {
+  rect->Inset(CrossAxisOuterMargin());
 }
 
-int BoxLayout::CrossAxisSizeForView(const View* view) const {
+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());
 }
 
 gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
                                                    int child_area_width) const {
+  DCHECK_EQ(host, host_);
   gfx::Rect child_area_bounds;
 
   if (orientation_ == kHorizontal) {
@@ -272,34 +382,36 @@ gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
     // 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));
+      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;
     }
 
@@ -307,15 +419,45 @@ gfx::Size BoxLayout::GetPreferredSizeForChildWidth(const View* host,
     child_area_bounds.set_height(height);
   }
 
-  gfx::Size non_child_size = NonChildSize(host);
+  gfx::Size non_child_size = NonChildSize(host_);
   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());
+  } else {
+    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