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..872cd4e1ea79f7755fed6cb8e74afff423bd189d 100644
--- a/ui/views/layout/box_layout.cc
+++ b/ui/views/layout/box_layout.cc
@@ -8,13 +8,84 @@
 
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/view.h"
+#include "ui/views/view_properties.h"
 
 namespace views {
 
+namespace {
+
+// Returns the maximum of the given insets along the given |axis|.
+// NOTE: |axis| is different from |orientation_|; it specifies the actual
+// desired axis.
+enum Axis { HORIZONTAL_AXIS, VERTICAL_AXIS };
+
+gfx::Insets MaxAxisInsets(Axis axis,
+                          const gfx::Insets& leading1,
+                          const gfx::Insets& leading2,
+                          const gfx::Insets& trailing1,
+                          const gfx::Insets& trailing2) {
+  if (axis == HORIZONTAL_AXIS)

[sky, 2017/06/05 21:04:10]

use {}

[kylix_rd, 2017/06/06 18:22:34]

Done.

+    return gfx::Insets(0, std::max(leading1.left(), leading2.left()), 0,
+                       std::max(trailing1.right(), trailing2.right()));
+  return gfx::Insets(std::max(leading1.top(), leading2.top()), 0,
+                     std::max(trailing1.bottom(), trailing2.bottom()), 0);
+}
+}

[sky, 2017/06/05 21:04:10]

Newline between 32/33 and // namespace (with two spaces before the '//'). See
last three lines in this file for what this should look like.

[kylix_rd, 2017/06/06 18:22:35]

Done.

+
+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_)

[sky, 2017/06/05 21:04:10]

This is subtle and worth a comment. In particular *why* does this not include
the margins?

[kylix_rd, 2017/06/06 18:22:34]

Done.

+    return view_->GetHeightForWidth(width);
+  if (orientation_ == Orientation::kHorizontal) {
+    return view_->GetHeightForWidth(std::max(0, width - margins_.width())) +
+           margins_.height();
+  } else {

[sky, 2017/06/05 21:04:10]

no else after early return.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+    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) {
+      new_bounds.set_x(bounds.x() + margins_.left());
+      new_bounds.set_width(std::max(0, bounds.width() - margins_.width()));
+    }
+    if (orientation_ == Orientation::kVertical) {

[sky, 2017/06/05 21:04:10]

Replace this if with an else.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+      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),
@@ -23,8 +94,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 +120,30 @@ 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());
+
+  AdjustMainAxisForMargin(&child_area);
+  gfx::Insets max_cross_axis_margin;

[sky, 2017/06/05 21:04:10]

Early out if child_area empty?

[kylix_rd, 2017/06/06 18:22:34]

Done.

+  if (!collapse_margins_spacing_) {
+    AdjustCrossAxisForInsets(&child_area);
+    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())
+    const ViewWrapper child(this, host->child_at(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, ViewWrapper(this, NextVisibleView(i)));
     ++num_visible;
     flex_sum += flex;
   }
@@ -106,8 +184,8 @@ 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));
+    if (!child.visible())
       continue;
 
     // TODO(bruthig): Fix this. The main axis should be calculated before
@@ -116,23 +194,54 @@ void BoxLayout::Layout(View* host) {
 
     // Calculate cross axis size.
     gfx::Rect bounds(child_area);
+    gfx::Rect min_child_area(child_area);
+    gfx::Insets child_margins;
+    if (collapse_margins_spacing_)
+      child_margins = MaxAxisInsets(

[sky, 2017/06/05 21:04:10]

{}

[kylix_rd, 2017/06/06 18:22:35]

Done.

+          orientation_ == kVertical ? HORIZONTAL_AXIS : VERTICAL_AXIS,
+          child.margins(), inside_border_insets_, child.margins(),
+          inside_border_insets_);
+    else
+      child_margins = child.margins();
+    if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_STRETCH ||
+        cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
+      InsetCrossAxis(&min_child_area, CrossAxisLeadingInset(child_margins),
+                     CrossAxisTrailingInset(child_margins));
+    }
+
     SetMainAxisPosition(main_position, &bounds);
     if (cross_axis_alignment_ != CROSS_AXIS_ALIGNMENT_STRETCH) {
-      int free_space = CrossAxisSize(bounds) - CrossAxisSizeForView(child);
+      int cross_axis_margin_size = CrossAxisMarginSizeForView(child);
+      int view_cross_axis_size =
+          CrossAxisSizeForView(child) - cross_axis_margin_size;
+      int free_space = CrossAxisSize(bounds) - view_cross_axis_size;
       int position = CrossAxisPosition(bounds);
       if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_CENTER) {
+        if (view_cross_axis_size > CrossAxisSize(min_child_area))
+          view_cross_axis_size = CrossAxisSize(min_child_area);
         position += free_space / 2;
+        position = std::max(position, CrossAxisLeadingEdge(min_child_area));
       } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
-        position += free_space;
+        position += free_space - CrossAxisTrailingInset(max_cross_axis_margin);
+        if (!collapse_margins_spacing_)
+          InsetCrossAxis(&min_child_area,
+                         CrossAxisLeadingInset(child.margins()),
+                         CrossAxisTrailingInset(max_cross_axis_margin));
+      } else {
+        position += CrossAxisLeadingInset(max_cross_axis_margin);
+        if (!collapse_margins_spacing_)
+          InsetCrossAxis(&min_child_area,
+                         CrossAxisLeadingInset(max_cross_axis_margin),
+                         CrossAxisTrailingInset(child.margins()));
       }
       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;
@@ -147,12 +256,14 @@ 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, ViewWrapper(this, NextVisibleView(i)));
 
     // Clamp child view bounds to |child_area|.
-    bounds.Intersect(child_area);
-    child->SetBoundsRect(bounds);
+    bounds.Intersect(min_child_area);
+    child.SetBoundsRect(bounds);
   }
 
   // Flex views should have grown/shrunk to consume all free space.
@@ -165,14 +276,41 @@ 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())
+    int leading = 0;

[sky, 2017/06/05 21:04:10]

This whole block could use a comment.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+    int trailing = 0;
+    gfx::Rect child_view_area;
+    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.view()->GetPreferredSize();

[sky, 2017/06/05 21:04:10]

This is worth a comment as to why you don't use the wrapper's GetPreferredSize
function.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+      gfx::Insets child_margins;
+      if (collapse_margins_spacing_)

[sky, 2017/06/05 21:04:10]

use {} as body spans multiple lines.

[kylix_rd, 2017/06/06 18:22:34]

Done.

+        child_margins = MaxAxisInsets(HORIZONTAL_AXIS, child.margins(),
+                                      inside_border_insets_, child.margins(),
+                                      inside_border_insets_);
+      else
+        child_margins = child.margins();
+      if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_START) {
+        leading = std::max(leading, CrossAxisLeadingInset(child_margins));
+        width = std::max(
+            width, child_size.width() + CrossAxisTrailingInset(child_margins));
+      } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
+        trailing = std::max(trailing, CrossAxisTrailingInset(child_margins));
+        width = std::max(
+            width, child_size.width() + CrossAxisLeadingInset(child_margins));
+      } else {
+        gfx::Rect child_bounds =
+            gfx::Rect(-(child_size.width() / 2), 0, child_size.width(),

[sky, 2017/06/05 21:04:10]

Shouldn't you use the margins in this code path?

[kylix_rd, 2017/06/06 18:22:34]

No. This code is "centering" the view horizontally by using 0 as the
center-point. Since we don't have a rectangle which can be used to calculate a
common center point, a single known point (0) is used.

That's OK here, because we're not interested in the actual position of the view.
We're only interested in the overall width including the margins.

Comment added to that effect.

+                      child_size.height());
+        child_bounds.Inset(-child.margins().left(), 0, -child.margins().right(),
+                           0);
+        child_view_area.Union(child_bounds);
+        width = std::max(width, child_view_area.width());
+      }
     }
-    width = std::max(width, minimum_cross_axis_size_);
+    width = std::max(width + leading + trailing, minimum_cross_axis_size_);
   }
 
   return GetPreferredSizeForChildWidth(host, width);
@@ -194,7 +332,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 +383,118 @@ 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::MainAxisLeadingInset(const gfx::Insets& insets) const {
+  return orientation_ == kHorizontal ? insets.left() : insets.top();
+}
+
+int BoxLayout::MainAxisTrailingInset(const gfx::Insets& insets) const {
+  return orientation_ == kHorizontal ? insets.right() : insets.bottom();
+}
+
+int BoxLayout::CrossAxisLeadingEdge(const gfx::Rect& rect) const {
+  return orientation_ == kVertical ? rect.x() : rect.y();
+}
+
+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& leading,
+                                          const ViewWrapper& trailing) const {
+  if (!collapse_margins_spacing_ || !leading.view() || !trailing.view())
+    return between_child_spacing_;
+  return std::max(between_child_spacing_,
+                  std::max(MainAxisTrailingInset(leading.margins()),
+                           MainAxisLeadingInset(trailing.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 leading = 0;
+  int trailing = 0;
+  for (int i = 0; i < host_->child_count(); ++i) {
+    const ViewWrapper child(this, host_->child_at(i));
+    if (!child.visible())
+      continue;
+    leading = std::max(leading, CrossAxisLeadingInset(child.margins()));
+    trailing = std::max(trailing, CrossAxisTrailingInset(child.margins()));
+  }
+  if (orientation_ == Orientation::kVertical)
+    return gfx::Insets(0, leading, 0, trailing);
+  return gfx::Insets(leading, 0, trailing, 0);
+}
+
+void BoxLayout::AdjustMainAxisForMargin(gfx::Rect* rect) const {
+  rect->Inset(MainAxisOuterMargin());
 }
 
-int BoxLayout::CrossAxisSizeForView(const View* view) const {
+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::CrossAxisLeadingMarginForView(const ViewWrapper& view) const {
+  return collapse_margins_spacing_ ? 0 : CrossAxisLeadingInset(view.margins());
+}
+
+void BoxLayout::InsetCrossAxis(gfx::Rect* rect,
+                               int leading,
+                               int trailing) const {
+  if (orientation_ == kVertical)
+    rect->Inset(leading, 0, trailing, 0);
+  else
+    rect->Inset(0, leading, 0, trailing);
 }
 
 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 +502,64 @@ 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())
+    gfx::Insets max_margins = CrossAxisMaxViewMargin();
+    for (int i = 0; i < host_->child_count(); ++i) {
+      const ViewWrapper child(this, host_->child_at(i));
+      if (!child.visible())
         continue;
 
-      gfx::Size size(child->GetPreferredSize());
+      gfx::Size size(child.view()->GetPreferredSize());
       if (size.IsEmpty())
         continue;
 
-      gfx::Rect child_bounds(position, 0, size.width(), size.height());
+      gfx::Rect child_bounds = gfx::Rect(
+          position, 0,
+          size.width() +
+              (!collapse_margins_spacing_ ? child.margins().width() : 0),
+          size.height());
+      gfx::Insets child_margins;
+      if (collapse_margins_spacing_)
+        child_margins =
+            MaxAxisInsets(VERTICAL_AXIS, child.margins(), inside_border_insets_,
+                          child.margins(), inside_border_insets_);
+      else
+        child_margins = child.margins();
+
+      if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_START) {
+        child_bounds.Inset(0, -CrossAxisLeadingInset(max_margins), 0,
+                           -child_margins.bottom());
+        child_bounds.set_origin(gfx::Point(position, 0));
+      } else if (cross_axis_alignment_ == CROSS_AXIS_ALIGNMENT_END) {
+        child_bounds.Inset(0, -child_margins.top(), 0,
+                           -CrossAxisTrailingInset(max_margins));
+        child_bounds.set_origin(gfx::Point(position, 0));
+      } else {
+        child_bounds.set_origin(
+            gfx::Point(position, -(child_bounds.height() / 2)));
+        child_bounds.Inset(0, -child_margins.top(), 0, -child_margins.bottom());
+      }
+
       child_area_bounds.Union(child_bounds);
-      position += size.width() + between_child_spacing_;
+      position += child_bounds.width() +
+                  MainAxisMarginBetweenViews(
+                      child, ViewWrapper(this, NextVisibleView(i)));

[sky, 2017/06/05 21:04:10]

optional: I have to admit I'm mildly bothered that NextVisibleView iterates, and
then the for loop iterates (this comment applies to other for loops that share
the same pattern).

[kylix_rd, 2017/06/06 18:22:34]

Understandable. It could be incorporated into the loop itself, but I fear it may
make the code more dense.

     }
     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));
+      if (!child.visible())
         continue;
 
+      const ViewWrapper next(this, NextVisibleView(i));
       // 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 +567,42 @@ 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());
+  gfx::Insets main_axis = MainAxisOuterMargin();
+  gfx::Insets cross_axis = inside_border_insets_;
+  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