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

ash/common/system/tray/tray_details_view.cc

 // Copyright (c) 2012 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/tray/tray_details_view.h"
 
 #include "ash/common/ash_view_ids.h"
 #include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/system/tray/fixed_sized_scroll_view.h"
 #include "ash/common/system/tray/system_menu_button.h"
@@ skipping 130 matching lines @@
   const int kShadowOffsetY = 2;
   const int kShadowBlur = 2;
   // TODO(fukino): Remove this constant once we stop maintaining pre-MD design.
   // crbug.com/614453.
   const int kContentsBetweenChildSpacingNonMd = 1;
 
   // A structure that keeps the original offset of each header between the
   // 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;
 
     // 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
   // top of the visible viewport.
   void PositionHeaderRows() {
     const int scroll_offset = -y();
     Header* previous_header = nullptr;
     for (auto& header : base::Reversed(headers_)) {
       views::View* header_view = header.view;
       if (header.natural_offset >= scroll_offset) {
+        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;
-        header_view->SetY(header.natural_offset);
         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();
       }
       break;
     }
   }
 
   // Paints a separator for a header view. The separator can be a horizontal
   // rule or a horizontal shadow, depending on whether the header is sticking to
   // the top of the scroll viewport. The return value indicates whether a shadow
   // 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()));
     return true;
   }
 
   // Draws a drop shadow below |shadowed_area|.
   void DrawShadow(const ui::PaintContext& context,
                   const gfx::Rect& shadowed_area) {
     ui::PaintRecorder recorder(context, size());
@@ skipping 340 matching lines @@
     if (index < child_count() - 1 && child_at(index + 1) != title_row_)
       scroll_border_->set_visible(true);
     else
       scroll_border_->set_visible(false);
   }
 
   views::View::OnPaintBorder(canvas);
 }
 
 }  // namespace ash