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

ash/common/system/tray/tray_details_view.cc

Index: ash/common/system/tray/tray_details_view.cc
diff --git a/ash/common/system/tray/tray_details_view.cc b/ash/common/system/tray/tray_details_view.cc
index 30df1395b64fdaca8f5d0d2a53df868a6f28cb71..96f44a40769a14e4fb91f69f6c5f1516edc73c67 100644
--- a/ash/common/system/tray/tray_details_view.cc
+++ b/ash/common/system/tray/tray_details_view.cc
@@ -148,7 +148,7 @@ class ScrollContentsView : public views::View,
   // calls to Layout() to allow keeping track of which view should be sticky.
   struct Header {
     explicit Header(views::View* view)
-        : view(view), natural_offset(view->y()) {}
+        : view(view), natural_offset(view->y()), separator(false) {}
 
     // A header View that can be decorated as sticky.
     views::View* view;
@@ -156,6 +156,10 @@ class ScrollContentsView : public views::View,
     // Offset from the top of ScrollContentsView to |view|'s original vertical
     // position.
     int natural_offset;
+
+    // True when a separator needs to be painted below the header when another
+    // header is pushing |this| header up.
+    bool separator;

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

I suggest renaming to |draw_separator_below| or similar for readability at call
sites. Alternatively, consider a name that represents what the variable is
actually tracking, such as |joined_to_previous_header|.

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

Done.

   };
 
   // Adjusts y-position of header rows allowing one or two rows to stick to the
@@ -166,16 +170,19 @@ class ScrollContentsView : public views::View,
     for (auto& header : base::Reversed(headers_)) {
       views::View* header_view = header.view;
       if (header.natural_offset >= scroll_offset) {
-        previous_header = &header;
+        header.separator = false;

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

Consider moving this line directly above line 172 (outside the if) and then
deleting line 185.

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

Done.

         header_view->SetY(header.natural_offset);

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

Is there a reason for moving this line before previous_header = &header?

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

Just seemed nicer with the now moved assignment. Moved back.

+        previous_header = &header;
         continue;
       }
       if (previous_header &&
-          previous_header->view->y() < scroll_offset + header_view->height()) {
+          previous_header->view->y() <= scroll_offset + header_view->height()) {

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

Double checking my understanding: you changed this to <= so that a separator is
drawn at the time when the previous header and this header first come into
contact, but the current header hasn't started to be displaced from its sticky
position?

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

Yes, exactly. Otherwise I would get a flash of no separator when the rows are
exactly touching.

         // Lower header displacing the header above.
+        header.separator = true;
         header_view->SetY(previous_header->view->y() - header_view->height());
       } else {
         // A header becomes sticky.
+        header.separator = false;
         header_view->SetY(scroll_offset);
         header_view->Layout();
         header_view->SchedulePaint();
@@ -190,28 +197,23 @@ class ScrollContentsView : public views::View,
   // was drawn.
   bool PaintDelineation(const Header& header, const ui::PaintContext& context) {
     const View* view = header.view;
-    const bool at_top = view->y() == -y();
 
-    // If the header is where it normally belongs, draw a separator above.
-    if (view->y() == header.natural_offset) {
-      // But if the header is at the very top of the viewport, draw nothing.
-      if (at_top)
-        return false;
+    // If the header is where it normally belongs, draw nothing.
+    if (view->y() == header.natural_offset)
+      return false;
 
+    // If the header is pushed by a header directly below it, draw a separator.
+    if (header.separator) {
       // TODO(estade): look better at 1.5x scale.
       ui::PaintRecorder recorder(context, size());
       gfx::Canvas* canvas = recorder.canvas();
       gfx::Rect separator = view->bounds();
+      separator.set_y(separator.bottom() - kSeparatorWidth);
       separator.set_height(kSeparatorWidth);
       canvas->FillRect(separator, kSeparatorColor);
       return false;
     }
 
-    // If the header is displaced but is not at the top of the viewport, it's
-    // being pushed out by another header. Draw nothing.
-    if (!at_top)
-      return false;
-
     // Otherwise, draw a shadow below.
     DrawShadow(context,
                gfx::Rect(0, 0, view->width(), view->bounds().bottom()));