[ash-md] Adjusts layout of lists with sticky header rows to match specs

ash/common/system/chromeos/network/vpn_list_view.cc

Index: ash/common/system/chromeos/network/vpn_list_view.cc
diff --git a/ash/common/system/chromeos/network/vpn_list_view.cc b/ash/common/system/chromeos/network/vpn_list_view.cc
index ba923fe7f5b0d5fa36463caddf20915e0d225b61..92ff273396510aa56d8b0d93c9ec028cb82e28b5 100644
--- a/ash/common/system/chromeos/network/vpn_list_view.cc
+++ b/ash/common/system/chromeos/network/vpn_list_view.cc
@@ -110,17 +110,26 @@ class VPNListProviderEntryMd : public views::ButtonListener,
   VPNListProviderEntryMd(ViewClickListener* parent,
                          const std::string& name,
                          int button_accessible_name_id)
-      : parent_(parent) {
+      : parent_(parent), tri_view_(nullptr) {
+    // TODO(varkha): Make this a sticky section header.
     SetLayoutManager(new views::FillLayout);
-    TriView* tri_view = TrayPopupUtils::CreateDefaultRowView();
-    tri_view->SetContainerVisible(TriView::Container::START, false);
-    AddChildView(tri_view);
+    tri_view_ = TrayPopupUtils::CreateDefaultRowView();
+    tri_view_->SetContainerVisible(TriView::Container::START, false);
+    AddChildView(tri_view_);
 
     views::Label* label = TrayPopupUtils::CreateDefaultLabel();
     TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::SUB_HEADER);
     style.SetupLabel(label);
     label->SetText(base::ASCIIToUTF16(name));
-    tri_view->AddView(TriView::Container::CENTER, label);
+    tri_view_->AddView(TriView::Container::CENTER, label);
+    auto box_layout = base::MakeUnique<views::BoxLayout>(

[tdanderson, 2016/11/24 23:37:53]

Please add a TODO to consider making a factory function for this type of row
instead of modifying the "CreateDefaultRowView()" at creation site. Though it
may be worth double-checking with Ben; if he has any changes planned in 56 for
how CreateDefaultRowView() behaves then it may inadvertently impact the layout
here.

[tdanderson, 2016/11/24 23:59:52]

Please also make sure Ben's change at
https://chromiumcodereview.appspot.com/2531713002/ (which will land before this
one) does not mess up the layout in this CL.

[bruthig, 2016/11/25 00:08:54]

+1 to adding a factory function like TPU::CreateSubHeaderRow() that returns a
correctly configured TriView.

[varkha, 2016/11/28 17:14:39]

Acknowledged.

[varkha, 2016/11/28 17:14:39]

Acknowledged.

[varkha, 2016/11/28 17:14:39]

Acknowledged.

[varkha, 2016/11/28 21:24:49]

Done. See if this is what you had in mind.

+        views::BoxLayout::kHorizontal, 0, 0, 0);
+    box_layout->set_main_axis_alignment(
+        views::BoxLayout::MAIN_AXIS_ALIGNMENT_START);
+    box_layout->set_cross_axis_alignment(
+        views::BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER);
+    tri_view_->SetContainerLayout(TriView::Container::CENTER,
+                                  std::move(box_layout));
 
     gfx::ImageSkia icon = gfx::CreateVectorIcon(kSystemMenuAddConnectionIcon,
                                                 style.GetIconColor());
@@ -128,7 +137,20 @@ class VPNListProviderEntryMd : public views::ButtonListener,
         new SystemMenuButton(this, TrayPopupInkDropStyle::HOST_CENTERED, icon,
                              icon, button_accessible_name_id);
     add_vpn_button->SetEnabled(true);
-    tri_view->AddView(TriView::Container::END, add_vpn_button);
+    tri_view_->AddView(TriView::Container::END, add_vpn_button);
+  }
+
+  // Sets up the border. When the provider header is the first item in the
+  // list (i.e. when the list was empty before adding the provider row) there is
+  // already |kMenuSeparatorVerticalPadding| padding at the top of the scroll
+  // contents, so only add that padding when the list was not |empty|.

[tdanderson, 2016/11/24 23:37:53]

So just to be sure, you are not doing any of this special handling to suppress
the extra 4px when the header is the first thing in network_list_md.cc *because*
the network headers are sticky and thus are able to handle that already?

[varkha, 2016/11/28 17:14:39]

Yes, if configured as sticky this would not be necessary. See
https://cs.chromium.org/chromium/src/ash/common/system/tray/tray_details_view...

+  // TODO(varkha): Remove this special handling when this header becomes sticky.
+  void SetBorderForHeader(bool empty) {
+    tri_view_->SetBorder(views::CreateEmptyBorder(
+        empty ? 0 : kMenuSeparatorVerticalPadding,
+        kTrayPopupPaddingHorizontal -
+            GetTrayConstant(TRAY_POPUP_ITEM_LEFT_INSET),

[tdanderson, 2016/11/24 23:37:53]

So you're subtracting TRAY_POPUP_ITEM_LEFT_INSET from the left margin to
compensate for what TriView is doing?

[varkha, 2016/11/28 17:14:39]

Yes. Let me see if adding a factory would isolate this code there.

+        kMenuSeparatorVerticalPadding, 0));
   }
 
  protected:
@@ -141,6 +163,9 @@ class VPNListProviderEntryMd : public views::ButtonListener,
   // Our parent to handle events.
   ViewClickListener* parent_;
 
+  // Container that owns the child controls.
+  TriView* tri_view_;
+
   DISALLOW_COPY_AND_ASSIGN(VPNListProviderEntryMd);
 };
 
@@ -281,22 +306,20 @@ void VPNListNetworkEntry::UpdateFromNetworkState(
     if (UseMd()) {
       disconnect_button_ = TrayPopupUtils::CreateTrayPopupButton(
           this, l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_VPN_DISCONNECT));
-    } else {
-      disconnect_button_ = new DisconnectButton(this);
-    }
-    if (UseMd()) {
       DCHECK(tri_view());
       tri_view()->AddView(TriView::Container::END, disconnect_button_);
       tri_view()->SetContainerVisible(TriView::Container::END, true);
     } else {
+      disconnect_button_ = new DisconnectButton(this);
       AddChildView(disconnect_button_);
     }
 
     SetBorder(
-        views::CreateEmptyBorder(0, kTrayPopupPaddingHorizontal, 0,
-                                 UseMd() ? kTrayPopupButtonEndMargin : 3));
+        views::CreateEmptyBorder(0, UseMd() ? 0 : kTrayPopupPaddingHorizontal,
+                                 0, UseMd() ? kTrayPopupButtonEndMargin : 3));
   } else {
-    SetBorder(views::CreateEmptyBorder(0, kTrayPopupPaddingHorizontal, 0, 0));
+    SetBorder(views::CreateEmptyBorder(
+        0, UseMd() ? 0 : kTrayPopupPaddingHorizontal, 0, 0));
   }
 
   if (!UseMd()) {
@@ -450,29 +473,26 @@ void VPNListView::AddProviderAndNetworks(
     const VPNProvider& vpn_provider,
     const chromeos::NetworkStateHandler::NetworkStateList& networks) {
   // Add a visual separator, unless this is the topmost entry in the list.
-  if (!list_empty_) {
-    views::Separator* const separator =
-        new views::Separator(views::Separator::HORIZONTAL);
-    separator->SetColor(kBorderLightColor);
-    container()->AddChildView(separator);
-  } else {
-    list_empty_ = false;
-  }
+  if (!list_empty_)
+    container()->AddChildView(TrayPopupUtils::CreateListHeaderSeparator());
   std::string vpn_name =
       vpn_provider.third_party
           ? vpn_provider.third_party_provider_name
           : l10n_util::GetStringUTF8(IDS_ASH_STATUS_TRAY_VPN_BUILT_IN_PROVIDER);
 
   // Add a list entry for the VPN provider.
-  views::View* provider_view = nullptr;
+  views::View* view = nullptr;
   if (UseMd()) {
-    provider_view = new VPNListProviderEntryMd(
+    VPNListProviderEntryMd* provider_view = new VPNListProviderEntryMd(
         this, vpn_name, IDS_ASH_STATUS_TRAY_ADD_CONNECTION);
+    provider_view->SetBorderForHeader(list_empty_);
+    view = provider_view;
   } else {
-    provider_view = new VPNListProviderEntry(this, vpn_name);
+    view = new VPNListProviderEntry(this, vpn_name);
   }
-  container()->AddChildView(provider_view);
-  provider_view_map_[provider_view] = vpn_provider;
+  container()->AddChildView(view);
+  provider_view_map_[view] = vpn_provider;
+  list_empty_ = false;
   // Add the networks belonging to this provider, in the priority order returned
   // by shill.
   for (const chromeos::NetworkState* const& network : networks) {