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

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/common/system/chromeos/network/vpn_list_view.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 92 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(VPNListProviderEntry);
 };
 
 // A list entry that represents a VPN provider with Material Design.
 class VPNListProviderEntryMd : public views::ButtonListener,
                                public views::View {
  public:
   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());
     views::ImageButton* add_vpn_button =
         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:
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override {
     parent_->OnViewClicked(this);
   }
 
  private:
   // Our parent to handle events.
   ViewClickListener* parent_;
 
+  // Container that owns the child controls.
+  TriView* tri_view_;
+
   DISALLOW_COPY_AND_ASSIGN(VPNListProviderEntryMd);
 };
 
 // A list entry that represents a network. If the network is currently
 // connecting, the icon shown by this list entry will be animated. If the
 // network is currently connected, a disconnect button will be shown next to its
 // name.
 class VPNListNetworkEntry : public VPNListEntryBase,
                             public network_icon::AnimationObserver {
  public:
@@ skipping 96 matching lines @@
 
 void VPNListNetworkEntry::DisconnectButton::OnBoundsChanged(
     const gfx::Rect& previous_bounds) {
   TrayPopupLabelButton::OnBoundsChanged(previous_bounds);
   if (IsMouseHovered()) {
     SetState(STATE_HOVERED);
     parent_->SetHoverHighlight(false);
   }
 }
 
 void VPNListNetworkEntry::UpdateFromNetworkState(

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

Please revert your changes to UpdateFromNetworkState(). Yi has a CL in progress
that will be addressing the internal layout of rows which contain an
MdTextButton, which includes changes to this function
(https://chromiumcodereview.appspot.com/2512883002/) - I think they more
properly belong within the scope of her CL, and removing them here makes your CL
smaller.

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

Done.

     const chromeos::NetworkState* network) {
   if (network && network->IsConnectingState())
     network_icon::NetworkIconAnimation::GetInstance()->AddObserver(this);
   else
     network_icon::NetworkIconAnimation::GetInstance()->RemoveObserver(this);
 
   if (!network) {
     // This is a transient state where the network has been removed already but
     // the network list in the UI has not been updated yet.
     return;
   }
 
   RemoveAllChildViews(true);
   disconnect_button_ = nullptr;
 
   AddIconAndLabel(
       network_icon::GetImageForNetwork(network, network_icon::ICON_TYPE_LIST),
       network_icon::GetLabelForNetwork(network, network_icon::ICON_TYPE_LIST),
       IsConnectedOrConnecting(network));
   if (IsConnectedOrConnecting(network)) {
     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()) {
     // The icon and the disconnect button are always set to their preferred
     // size. All remaining space is used for the network name.
     views::BoxLayout* layout = new views::BoxLayout(
         views::BoxLayout::kHorizontal, 0, 3, kTrayPopupPaddingBetweenItems);
     SetLayoutManager(layout);
     layout->SetDefaultFlex(0);
     layout->SetFlexForView(text_label(), 1);
@@ skipping 133 matching lines @@
   views::View* entry(new VPNListNetworkEntry(this, network));
   container()->AddChildView(entry);
   network_view_service_path_map_[entry] = network->path();
   list_empty_ = false;
 }
 
 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) {
     if (VpnProviderMatchesNetwork(vpn_provider, *network))
       AddNetwork(network);
   }
 }
 
 void VPNListView::AddProvidersAndNetworks(
     const chromeos::NetworkStateHandler::NetworkStateList& networks) {
@@ skipping 15 matching lines @@
     }
   }
 
   // Add providers without any configured networks, in the order that the
   // providers were returned by the extensions system.
   for (const VPNProvider& provider : providers)
     AddProviderAndNetworks(provider, networks);
 }
 
 }  // namespace ash