Layout integration tests, plus some spot-fixes for corner cases.

These issues could conceivably cause some graphical glitches in certain
cases in e.g. the extensions container, which we have seen some reports
of.

Still to do:
- Tests which use a widget to simulate being minimized, maximized, etc.
  mid-animation

Change-Id: I54249b8de85dae74d45327ea8f80501981800c3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2335652
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794900}

ui/views/BUILD.gn

@@ -1097,6 +1097,7 @@ test("views_unittests") {
     "focus/focus_traversal_unittest.cc",
     "layout/animating_layout_manager_unittest.cc",
     "layout/box_layout_unittest.cc",
+    "layout/composite_layout_tests.cc",
     "layout/fill_layout_unittest.cc",
     "layout/flex_layout_unittest.cc",
     "layout/grid_layout_unittest.cc",

ui/views/layout/animating_layout_manager.cc

@@ -54,6 +54,27 @@ enum class LayoutFadeType {
   kContinuingFade
 };
 
+// Makes a copy of the given layout with only visible child views (non-visible
+// children are omitted).
+ProposedLayout WithOnlyVisibleViews(const ProposedLayout layout) {
+  ProposedLayout result;
+  result.host_size = layout.host_size;
+  std::copy_if(
+      layout.child_layouts.begin(), layout.child_layouts.end(),
+      std::back_inserter(result.child_layouts),
+      [](const ChildLayout& child_layout) { return child_layout.visible; });
+  return result;
+}
+
+// Returns true if the two proposed layouts have the same visible views, with
+// the same parameters, in the same order.
+bool HaveSameVisibleViews(const ProposedLayout& l1, const ProposedLayout& l2) {
+  // There is an approach that uses nested loops and dual iterators that is more
+  // efficient than copying, but since this method is only currently called when
+  // views are added to the layout, clarity is more important than speed.
+  return WithOnlyVisibleViews(l1) == WithOnlyVisibleViews(l2);
+}
+
 }  // namespace
 
 // Holds data about a view that is fading in or out as part of an animation.
@@ -107,6 +128,11 @@ class AnimatingLayoutManager::AnimationDelegate
   // invalidating the host to make sure the layout is up to date.
   void MakeReadyForAnimation();
 
+  // Overrides the default animation container with |container|.
+  void SetAnimationContainerForTesting(gfx::AnimationContainer* container) {
+    animation_->SetContainer(container);
+  }
+
  private:
   // Observer used to watch for the host view being parented to a widget.
   class ViewWidgetObserver : public ViewObserver {
@@ -273,6 +299,17 @@ void AnimatingLayoutManager::FadeOut(View* child_view) {
     return;
   }
 
+  // This handles a case where we are in the middle of an animation where we
+  // would have hidden the target view, but haven't hit Layout() yet, so haven't
+  // actually hidden it yet. Because we plan fade-outs off of the current layout
+  // if the view the child view is visible it will not get a proper fade-out and
+  // will remain visible but not properly laid out. We remedy this by hiding the
+  // view immediately.
+  const ChildLayout* const current_layout =
+      FindChildViewInLayout(current_layout_, child_view);
+  if ((!current_layout || !current_layout->visible) && child_view->GetVisible())
+    SetViewVisibility(child_view, false);
+
   // Indicate that the view should become hidden in the layout without
   // immediately changing its visibility. Instead, this triggers an animation
   // which results in the view being hidden.
@@ -462,6 +499,24 @@ void AnimatingLayoutManager::OnInstalled(View* host) {
   animation_delegate_ = std::make_unique<AnimationDelegate>(this);
 }
 
+bool AnimatingLayoutManager::OnViewAdded(View* host, View* view) {
+  // Handle a case where we add a visible view that shouldn't be visible in the
+  // layout. In this case, there is no animation, no invalidation, and we just
+  // set the view to not be visible.
+  if (view->GetVisible() && cached_layout_size() && !is_animating_) {
+    const gfx::Size target_size = GetAvailableTargetLayoutSize();
+    ProposedLayout proposed_layout =
+        target_layout_manager()->GetProposedLayout(target_size);
+    if (HaveSameVisibleViews(current_layout_, proposed_layout)) {
+      SetViewVisibility(view, false);
+      current_layout_ = target_layout_ = proposed_layout;
+      return false;
+    }
+  }
+
+  return RecalculateTarget();
+}
+
 void AnimatingLayoutManager::OnLayoutChanged() {
   // This replaces the normal behavior of clearing cached layouts.
   RecalculateTarget();
@@ -585,6 +640,7 @@ bool AnimatingLayoutManager::RecalculateTarget() {
   // start or update an animation.
   const ProposedLayout proposed_layout =
       target_layout_manager()->GetProposedLayout(target_size);
+
   if (target_layout_ == proposed_layout)
     return false;
 
@@ -605,6 +661,11 @@ bool AnimatingLayoutManager::RecalculateTarget() {
     // child views' visibility changing.)
     starting_layout_ = current_layout_;
     starting_offset_ = current_offset_;
+  } else if (starting_layout_ == target_layout_) {
+    // If we initiated but did not show any frames of an animation, and we are
+    // redirected to our starting layout then just reset the layout.
+    ResetLayoutToSize(target_size);
+    return false;
   }
   CalculateFadeInfos();
   UpdateCurrentLayout(0.0);

ui/views/layout/animating_layout_manager.h

@@ -202,11 +202,20 @@ class VIEWS_EXPORT AnimatingLayoutManager : public LayoutManagerBase {
   // widget).
   void EnableAnimationForTesting();
 
+  const ProposedLayout& starting_layout_for_testing() const {
+    return starting_layout_;
+  }
+
+  const ProposedLayout& target_layout_for_testing() const {
+    return target_layout_;
+  }
+
  protected:
   // LayoutManagerBase:
   ProposedLayout CalculateProposedLayout(
       const SizeBounds& size_bounds) const override;
   void OnInstalled(View* host) override;
+  bool OnViewAdded(View* host, View* view) override;
   void OnLayoutChanged() override;
   void LayoutImpl() override;
 

ui/views/layout/layout_manager_base.cc

@@ -175,6 +175,8 @@ void LayoutManagerBase::ApplyLayout(const ProposedLayout& layout) {
     // events that wouldn't do anything useful).
     if (new_available_size != cached_available_size_ || child_layout.visible ||
         !child_layout.bounds.IsEmpty()) {
+      const bool size_changed =
+          child_view->bounds().size() != child_layout.bounds.size();
       if (child_view->bounds() != child_layout.bounds)
         child_view->SetBoundsRect(child_layout.bounds);
       // Child layouts which are not invalid will not be laid out by the default
@@ -182,7 +184,7 @@ void LayoutManagerBase::ApplyLayout(const ProposedLayout& layout) {
       // constraint it's important that the child view be laid out. So we'll do
       // it here.
       // TODO(dfried): figure out a better way to handle this.
-      else if (child_layout.available_size != SizeBounds())
+      if (!size_changed && child_layout.available_size != SizeBounds())
         child_view->Layout();
     }
   }