Make PWA titlebar and icons flex and animate properly.

Nested layouts now allocate available space properly to their
descendents. Also, we consistently use flex layout in the PWA titlebar.

Removed a couple of invalid assumptions that were encoded as DCHECKs.

There are still some inkdrop issues but they are purely cosmetic, see
crbug.com/1006162 for more detail.

Bug: 1048061
Change-Id: I403b71b5e2bbcb2d461e1ed6c543c76f91d4c652
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036264
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738874}

chrome/browser/ui/views/extensions/extensions_toolbar_container.cc

@@ -36,26 +36,41 @@ ExtensionsToolbarContainer::DropInfo::DropInfo(
     size_t index)
     : action_id(action_id), index(index) {}
 
-ExtensionsToolbarContainer::ExtensionsToolbarContainer(Browser* browser)
+ExtensionsToolbarContainer::ExtensionsToolbarContainer(Browser* browser,
+                                                       DisplayMode display_mode)
     : ToolbarIconContainerView(/*uses_highlight=*/true),
       browser_(browser),
       model_(ToolbarActionsModel::Get(browser_->profile())),
       model_observer_(this),
-      extensions_button_(new ExtensionsToolbarButton(browser_, this)) {
+      extensions_button_(new ExtensionsToolbarButton(browser_, this)),
+      display_mode_(display_mode) {
   model_observer_.Add(model_);
   // Do not flip the Extensions icon in RTL.
   extensions_button_->EnableCanvasFlippingForRTLUI(false);
-  extensions_button_->SetProperty(views::kFlexBehaviorKey,
-                                  views::FlexSpecification());
+
+  const views::FlexSpecification hide_icon_flex_specification =
+      views::FlexSpecification(views::MinimumFlexSizeRule::kPreferredSnapToZero,
+                               views::MaximumFlexSizeRule::kPreferred)
+          .WithWeight(0);
+  switch (display_mode) {
+    case DisplayMode::kNormal:
+      // In normal mode, the menu icon is always shown.
+      extensions_button_->SetProperty(views::kFlexBehaviorKey,
+                                      views::FlexSpecification());
+      break;
+    case DisplayMode::kCompact:
+      // In compact mode, the menu icon can be hidden but has the highest
+      // priority.
+      extensions_button_->SetProperty(
+          views::kFlexBehaviorKey, hide_icon_flex_specification.WithOrder(1));
+      break;
+  }
   extensions_button_->SetID(VIEW_ID_EXTENSIONS_MENU_BUTTON);
   AddMainButton(extensions_button_);
   target_layout_manager()
       ->SetFlexAllocationOrder(views::FlexAllocationOrder::kReverse)
       .SetDefault(views::kFlexBehaviorKey,
-                  views::FlexSpecification(
-                      views::MinimumFlexSizeRule::kPreferredSnapToZero,
-                      views::MaximumFlexSizeRule::kPreferred)
-                      .WithWeight(0));
+                  hide_icon_flex_specification.WithOrder(3));
   CreateActions();
 
   // TODO(pbos): Consider splitting out tab-strip observing into another class.
@@ -136,11 +151,28 @@ void ExtensionsToolbarContainer::UpdateIconVisibility(
   // Popped out action uses a flex rule that causes it to always be visible
   // regardless of space; default for actions is to drop out when there is
   // insufficient space. So if an action is being forced visible, it should have
-  // the always-show flex rule, and if it not, it should not.
+  // a rule that gives it higher priority, and if it does not, it should use the
+  // default.
   const bool must_show = ShouldForceVisibility(extension_id);
   if (must_show) {
-    action_view->SetProperty(views::kFlexBehaviorKey,
-                             views::FlexSpecification());
+    switch (display_mode_) {
+      case DisplayMode::kNormal:
+        // In normal mode, the icon's visibility is forced.
+        action_view->SetProperty(views::kFlexBehaviorKey,
+                                 views::FlexSpecification());
+        break;
+      case DisplayMode::kCompact:
+        // In compact mode, the icon can still drop out, but receives precedence
+        // over other actions.
+        action_view->SetProperty(
+            views::kFlexBehaviorKey,
+            views::FlexSpecification(
+                views::MinimumFlexSizeRule::kPreferredSnapToZero,
+                views::MaximumFlexSizeRule::kPreferred)
+                .WithWeight(0)
+                .WithOrder(2));
+        break;
+    }
   } else {
     action_view->ClearProperty(views::kFlexBehaviorKey);
   }

chrome/browser/ui/views/extensions/extensions_toolbar_container.h

@@ -38,7 +38,22 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   using ToolbarIconMap = std::map<ToolbarActionsModel::ActionId,
                                   std::unique_ptr<ToolbarActionView>>;
 
-  explicit ExtensionsToolbarContainer(Browser* browser);
+  // Determines how the container displays - specifically whether the menu and
+  // popped out action can be hidden.
+  enum class DisplayMode {
+    // In normal mode, the menu icon and popped-out action is always visible.
+    // Normal mode is used for the main toolbar and in windows where there is
+    // always enough space to show at least two icons.
+    kNormal,
+    // In compact mode, one or both of the menu icon and popped-out action may
+    // be hidden. Compact mode is used in smaller windows (e.g. webapps) where
+    // there may not be enough space to display the buttons.
+    kCompact,
+  };
+
+  explicit ExtensionsToolbarContainer(
+      Browser* browser,
+      DisplayMode display_mode = DisplayMode::kNormal);
   ~ExtensionsToolbarContainer() override;
 
   ExtensionsToolbarButton* extensions_button() const {
@@ -185,6 +200,7 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   ScopedObserver<ToolbarActionsModel, ToolbarActionsModel::Observer>
       model_observer_;
   ExtensionsToolbarButton* const extensions_button_;
+  DisplayMode display_mode_;
 
   // TODO(pbos): Create actions and icons only for pinned pinned / popped out
   // actions (lazily). Currently code expects GetActionForId() to return

chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

@@ -841,8 +841,8 @@ class WebAppNonClientFrameViewAshTest
   }
 
   PageActionIconView* GetPageActionIcon(PageActionIconType type) {
-    return browser_view_->toolbar_button_provider()
-        ->GetPageActionIconView(type);
+    return browser_view_->toolbar_button_provider()->GetPageActionIconView(
+        type);
   }
 
   ContentSettingImageView* GrantGeolocationPermission() {
@@ -1044,6 +1044,10 @@ IN_PROC_BROWSER_TEST_P(WebAppNonClientFrameViewAshTest,
   SetUpWebApp();
   ContentSettingImageView* geolocation_icon = GrantGeolocationPermission();
 
+  // In order to receive focus, the geo icon must be laid out (and be both
+  // visible and nonzero size).
+  web_app_frame_toolbar_->Layout();
+
   EXPECT_FALSE(web_app_menu_button_->HasFocus());
   EXPECT_FALSE(geolocation_icon->HasFocus());
 

chrome/browser/ui/views/toolbar/browser_actions_container.cc

@@ -437,7 +437,6 @@ int BrowserActionsContainer::GetHeightForWidth(int width) const {
 }
 
 gfx::Size BrowserActionsContainer::GetMinimumSize() const {
-  DCHECK(interactive_);
   return gfx::Size(GetResizeAreaWidth(),
                    toolbar_actions_bar_->GetViewSize().height());
 }

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_browsertest.cc

@@ -127,14 +127,16 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
   // Resize the WebAppFrameToolbarView just enough to clip out the page action
   // icons (and toolbar contents left of them).
   const int original_toolbar_width = web_app_frame_toolbar()->width();
-  web_app_frame_toolbar()->SetSize(
-      gfx::Size(toolbar_right_container->width() -
-                    page_action_icon_container->bounds().right(),
-                web_app_frame_toolbar()->height()));
-  frame_view()->SetSize(gfx::Size(frame_view()->width() -
-                                      original_toolbar_width +
-                                      web_app_frame_toolbar()->width(),
-                                  frame_view()->height()));
+  const int new_toolbar_width = toolbar_right_container->width() -
+                                page_action_icon_container->bounds().right();
+  const int new_frame_width =
+      frame_view()->width() - original_toolbar_width + new_toolbar_width;
+
+  // The best cross-platform way to resize the app window is to set the size
+  // directly on the widget.
+  views::Widget* const widget = frame_view()->GetWidget();
+  widget->SetSize(gfx::Size(new_frame_width, frame_view()->height()));
+  widget->GetRootView()->Layout();
 
   // The left container (containing Back and Reload) should be hidden.
   EXPECT_FALSE(toolbar_left_container->GetVisible());
@@ -144,9 +146,9 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
   EXPECT_EQ(window_title->width(), 0);
 #endif
 
-  // The page action icons should be clipped to 0 width while the app menu
-  // button retains its full width.
-  EXPECT_EQ(page_action_icon_container->width(), 0);
+  // The page action icons should be hidden while the app menu button retains
+  // its full width.
+  EXPECT_FALSE(page_action_icon_container->GetVisible());
   EXPECT_EQ(menu_button->width(), original_menu_button_width);
 }
 

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.cc

@@ -548,15 +548,22 @@ WebAppFrameToolbarView::ToolbarButtonContainer::ToolbarButtonContainer(
     views::Widget* widget,
     BrowserView* browser_view)
     : browser_view_(browser_view) {
-  views::BoxLayout& layout =
-      *SetLayoutManager(std::make_unique<views::BoxLayout>(
-          views::BoxLayout::Orientation::kHorizontal,
-          gfx::Insets(0, WebAppFrameRightMargin()),
-          HorizontalPaddingBetweenPageActionsAndAppMenuButtons()));
-  // Right align to clip the leftmost items first when not enough space.
-  layout.set_main_axis_alignment(views::BoxLayout::MainAxisAlignment::kEnd);
-  layout.set_cross_axis_alignment(
-      views::BoxLayout::CrossAxisAlignment::kCenter);
+  views::FlexLayout* const layout =
+      SetLayoutManager(std::make_unique<views::FlexLayout>());
+  layout->SetOrientation(views::LayoutOrientation::kHorizontal)
+      .SetInteriorMargin(gfx::Insets(0, WebAppFrameRightMargin()))
+      .SetDefault(
+          views::kMarginsKey,
+          gfx::Insets(0,
+                      HorizontalPaddingBetweenPageActionsAndAppMenuButtons()))
+      .SetCollapseMargins(true)
+      .SetCrossAxisAlignment(views::LayoutAlignment::kCenter)
+      .SetDefault(views::kFlexBehaviorKey,
+                  views::FlexSpecification(
+                      views::MinimumFlexSizeRule::kPreferredSnapToZero,
+                      views::MaximumFlexSizeRule::kPreferred)
+                      .WithWeight(0))
+      .SetFlexAllocationOrder(views::FlexAllocationOrder::kReverse);
 
   const auto* app_controller = browser_view_->browser()->app_controller();
 
@@ -596,8 +603,20 @@ WebAppFrameToolbarView::ToolbarButtonContainer::ToolbarButtonContainer(
                              static_cast<int>(HTCLIENT));
 
   if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu)) {
-    extensions_container_ = AddChildView(
-        std::make_unique<ExtensionsToolbarContainer>(browser_view_->browser()));
+    // Extensions toolbar area with pinned extensions is lower priority than,
+    // for example, the menu button or other toolbar buttons, and pinned
+    // extensions should hide before other toolbar buttons.
+    constexpr int kLowPriorityFlexOrder = 2;
+    extensions_container_ =
+        AddChildView(std::make_unique<ExtensionsToolbarContainer>(
+            browser_view_->browser(),
+            ExtensionsToolbarContainer::DisplayMode::kCompact));
+    extensions_container_->SetProperty(
+        views::kFlexBehaviorKey,
+        views::FlexSpecification(
+            extensions_container_->animating_layout_manager()
+                ->GetDefaultFlexRule())
+            .WithOrder(kLowPriorityFlexOrder));
     views::SetHitTestComponent(extensions_container_,
                                static_cast<int>(HTCLIENT));
   } else {

ui/views/layout/animating_layout_manager.cc

@@ -468,7 +468,6 @@ void AnimatingLayoutManager::LayoutImpl() {
         GetMainAxis(orientation(), current_layout_.host_size);
     if (current_main > host_main ||
         (bounds_main && current_main > *bounds_main)) {
-      DCHECK(!bounds_main || *bounds_main >= host_main);
       last_available_host_size_ = available_size;
       ResetLayoutToSize(host_size);
     } else if (available_size != last_available_host_size_) {

ui/views/layout/fill_layout.cc

@@ -42,7 +42,8 @@ ProposedLayout FillLayout::CalculateProposedLayout(
   for (View* child : host_view()->children()) {
     if (ShouldIncludeChild(child)) {
       layout.child_layouts.push_back(
-          {child, child->GetVisible(), contents_bounds});
+          ChildLayout{child, child->GetVisible(), contents_bounds,
+                      SizeBounds(contents_bounds.size())});
     }
   }
 

ui/views/layout/layout_manager_base.cc

@@ -12,6 +12,29 @@
 
 namespace views {
 
+namespace {
+
+// Adjusts |child_available_size| by adding the difference between the host
+// view's size and the size available to it.
+SizeBounds AdjustAvailableSizeForParentAvailableSize(
+    const View* host,
+    const SizeBounds& child_available_size) {
+  if (!host || !host->parent() || child_available_size == SizeBounds())
+    return child_available_size;
+
+  SizeBounds host_additional_size = host->parent()->GetAvailableSize(host);
+  host_additional_size.Enlarge(-host->width(), -host->height());
+  return SizeBounds(
+      child_available_size.width() && host_additional_size.width()
+          ? *child_available_size.width() + *host_additional_size.width()
+          : child_available_size.width(),
+      child_available_size.height() && host_additional_size.height()
+          ? *child_available_size.height() + *host_additional_size.height()
+          : child_available_size.height());
+}
+
+}  // anonymous namespace
+
 LayoutManagerBase::~LayoutManagerBase() = default;
 
 gfx::Size LayoutManagerBase::GetPreferredSize(const View* host) const {
@@ -44,8 +67,10 @@ SizeBounds LayoutManagerBase::GetAvailableSize(const View* host,
   DCHECK_EQ(host_view_, host);
   if (cached_layout_size_) {
     for (const auto& child_layout : cached_layout_.child_layouts)
-      if (child_layout.child_view == view)
-        return child_layout.available_size;
+      if (child_layout.child_view == view) {
+        return AdjustAvailableSizeForParentAvailableSize(
+            host, child_layout.available_size);
+      }
   }
   return SizeBounds();
 }

ui/views/layout/layout_manager_base_unittest.cc

@@ -36,9 +36,17 @@ class TestLayoutManagerBase : public LayoutManagerBase {
     return included;
   }
 
+  void OverrideProposedLayout(const ProposedLayout& forced_layout) {
+    forced_layout_ = forced_layout;
+    InvalidateHost(true);
+  }
+
   // LayoutManagerBase:
   ProposedLayout CalculateProposedLayout(
       const SizeBounds& size_bounds) const override {
+    if (forced_layout_)
+      return *forced_layout_;
+
     ProposedLayout layout;
     layout.host_size.set_width(
         std::max(kMinimumSize.width(),
@@ -48,6 +56,10 @@ class TestLayoutManagerBase : public LayoutManagerBase {
                  size_bounds.height().value_or(kPreferredSize.height())));
     return layout;
   }
+
+ private:
+  // If specified, will always return this layout.
+  base::Optional<ProposedLayout> forced_layout_;
 };
 
 // This layout layout lays out included child views in the upper-left of the
@@ -632,17 +644,12 @@ class LayoutManagerBaseAvailableSizeTest : public testing::Test {
         view_->SetLayoutManager(std::make_unique<TestLayoutManagerBase>());
   }
 
-  void SetCachedLayout(const ProposedLayout& layout) {
-    layout_->set_cached_layout_size(layout.host_size);
-    layout_->set_cached_layout(layout);
-  }
-
   View* view() { return view_.get(); }
-  LayoutManagerBase* layout() { return layout_; }
+  TestLayoutManagerBase* layout() { return layout_; }
 
  private:
   std::unique_ptr<View> view_;
-  LayoutManagerBase* layout_;
+  TestLayoutManagerBase* layout_;
 };
 
 TEST_F(LayoutManagerBaseAvailableSizeTest, ReturnsCorrectValues) {
@@ -652,13 +659,98 @@ TEST_F(LayoutManagerBaseAvailableSizeTest, ReturnsCorrectValues) {
   View* const child2 = view()->AddChildView(std::make_unique<View>());
   View not_a_child;
 
-  SetCachedLayout({{10, 10},
-                   {{child1, true, {1, 1, 1, 1}, kChild1Bounds},
-                    {child2, true, {2, 2, 2, 2}, kChild2Bounds}}});
+  layout()->OverrideProposedLayout(
+      {{10, 10},
+       {{child1, true, {1, 1, 1, 1}, kChild1Bounds},
+        {child2, true, {2, 2, 2, 2}, kChild2Bounds}}});
+  view()->SizeToPreferredSize();
 
   EXPECT_EQ(kChild1Bounds, view()->GetAvailableSize(child1));
   EXPECT_EQ(kChild2Bounds, view()->GetAvailableSize(child2));
   EXPECT_EQ(SizeBounds(), view()->GetAvailableSize(&not_a_child));
 }
 
+TEST_F(LayoutManagerBaseAvailableSizeTest, AvailableSizesInNestedValuesAdd) {
+  View* const child = view()->AddChildView(std::make_unique<View>());
+  View* const grandchild = child->AddChildView(std::make_unique<View>());
+  auto* const child_layout =
+      child->SetLayoutManager(std::make_unique<TestLayoutManagerBase>());
+
+  constexpr gfx::Size kViewSize(18, 17);
+  constexpr SizeBounds kChildAvailableSize(16, 15);
+  constexpr gfx::Size kChildSize(13, 12);
+  constexpr SizeBounds kGrandchildAvailableSize(10, 9);
+  constexpr gfx::Size kGrandchildSize(3, 2);
+  layout()->OverrideProposedLayout(
+      {kViewSize, {{child, true, {{3, 3}, kChildSize}, kChildAvailableSize}}});
+  child_layout->OverrideProposedLayout({kChildSize,
+                                        {{grandchild,
+                                          true,
+                                          {{2, 2}, kGrandchildSize},
+                                          kGrandchildAvailableSize}}});
+  view()->SizeToPreferredSize();
+
+  EXPECT_EQ(kChildAvailableSize, view()->GetAvailableSize(child));
+  SizeBounds expected;
+  expected.set_width(*kGrandchildAvailableSize.width() +
+                     *kChildAvailableSize.width() - kChildSize.width());
+  expected.set_height(*kGrandchildAvailableSize.height() +
+                      *kChildAvailableSize.height() - kChildSize.height());
+  EXPECT_EQ(expected, child->GetAvailableSize(grandchild));
+}
+
+TEST_F(LayoutManagerBaseAvailableSizeTest,
+       PartiallySpecifiedAvailableSizesInNestedLayoutsAddPartially) {
+  View* const child = view()->AddChildView(std::make_unique<View>());
+  View* const grandchild = child->AddChildView(std::make_unique<View>());
+  auto* const child_layout =
+      child->SetLayoutManager(std::make_unique<TestLayoutManagerBase>());
+
+  constexpr gfx::Size kViewSize(18, 17);
+  constexpr SizeBounds kChildAvailableSize(16, base::nullopt);
+  constexpr gfx::Size kChildSize(13, 12);
+  constexpr SizeBounds kGrandchildAvailableSize(10, 9);
+  constexpr gfx::Size kGrandchildSize(3, 2);
+  layout()->OverrideProposedLayout(
+      {kViewSize, {{child, true, {{3, 3}, kChildSize}, kChildAvailableSize}}});
+  child_layout->OverrideProposedLayout({kChildSize,
+                                        {{grandchild,
+                                          true,
+                                          {{2, 2}, kGrandchildSize},
+                                          kGrandchildAvailableSize}}});
+  view()->SizeToPreferredSize();
+
+  EXPECT_EQ(kChildAvailableSize, view()->GetAvailableSize(child));
+  SizeBounds expected;
+  expected.set_width(*kGrandchildAvailableSize.width() +
+                     *kChildAvailableSize.width() - kChildSize.width());
+  expected.set_height(*kGrandchildAvailableSize.height());
+  EXPECT_EQ(expected, child->GetAvailableSize(grandchild));
+}
+
+TEST_F(LayoutManagerBaseAvailableSizeTest,
+       MismatchedAvailableSizesInNestedLayoutsDoNotAdd) {
+  View* const child = view()->AddChildView(std::make_unique<View>());
+  View* const grandchild = child->AddChildView(std::make_unique<View>());
+  auto* const child_layout =
+      child->SetLayoutManager(std::make_unique<TestLayoutManagerBase>());
+
+  constexpr gfx::Size kViewSize(18, 17);
+  constexpr SizeBounds kChildAvailableSize(16, base::nullopt);
+  constexpr gfx::Size kChildSize(13, 12);
+  constexpr SizeBounds kGrandchildAvailableSize(base::nullopt, 9);
+  constexpr gfx::Size kGrandchildSize(3, 2);
+  layout()->OverrideProposedLayout(
+      {kViewSize, {{child, true, {{3, 3}, kChildSize}, kChildAvailableSize}}});
+  child_layout->OverrideProposedLayout({kChildSize,
+                                        {{grandchild,
+                                          true,
+                                          {{2, 2}, kGrandchildSize},
+                                          kGrandchildAvailableSize}}});
+  view()->SizeToPreferredSize();
+
+  EXPECT_EQ(kChildAvailableSize, view()->GetAvailableSize(child));
+  EXPECT_EQ(kGrandchildAvailableSize, child->GetAvailableSize(grandchild));
+}
+
 }  // namespace views