Minor visual improvements to text selection UI

ui/views/touchui/touch_selection_controller_impl.cc

 // Copyright (c) 2013 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 "ui/views/touchui/touch_selection_controller_impl.h"
 
 #include "base/time/time.h"
 #include "ui/aura/client/cursor_client.h"
 #include "ui/aura/env.h"
 #include "ui/aura/window.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/path.h"
 #include "ui/gfx/rect.h"
 #include "ui/gfx/screen.h"
 #include "ui/gfx/size.h"
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/views/widget/widget.h"
 #include "ui/wm/core/coordinate_conversion.h"
 #include "ui/wm/core/masked_window_targeter.h"
 
 namespace {
 
 // Constants defining the visual attributes of selection handles
 
-// Controls the width of the cursor line associated with the selection handle.
-const int kSelectionHandleLineWidth = 2;
-
-const SkColor kSelectionHandleLineColor =
-    SkColorSetRGB(0x42, 0x81, 0xf4);
+// The distance by which a handle image is offset from the bottom of the
+// selection/text baseline.
+const int kSelectionHandleVerticalVisualOffset = 2;
 
 // When a handle is dragged, the drag position reported to the client view is
 // offset vertically to represent the cursor position. This constant specifies
-// the offset in  pixels above the "O" (see pic below). This is required because
-// say if this is zero, that means the drag position we report is the point
-// right above the "O" or the bottom most point of the cursor "|". In that case,
-// a vertical movement of even one pixel will make the handle jump to the line
-// below it. So when the user just starts dragging, the handle will jump to the
-// next line if the user makes any vertical movement. It is correct but
-// looks/feels weird. So we have this non-zero offset to prevent this jumping.
+// the offset in  pixels above the bottom of the selection (see pic below). This

[mohsen, 2014/12/02 22:34:57]

nit: Please also remove the extra space in "in  pixels"

[mfomitchev, 2014/12/03 23:57:19]

Done.

+// is required because say if this is zero, that means the drag position we
+// report is right on the text baseline. In that case, a vertical movement of
+// even one pixel will make the handle jump to the line below it. So when the
+// user just starts dragging, the handle will jump to the next line if the user
+// makes any vertical movement. So we have this non-zero offset to prevent this
+// jumping.
 //
 // Editing handle widget showing the padding and difference between the position
 // of the ET_GESTURE_SCROLL_UPDATE event and the drag position reported to the
 // client:
-//                                  _____
-//                                 |  |<-|---- Drag position reported to client
+//                            ___________

[mohsen, 2014/12/02 22:34:56]

nit: Apparently, this line has trailing spaces. Can we get rid of them?

[mfomitchev, 2014/12/03 23:57:20]

I don't see any. There's a bunch of underscores.

[mohsen, 2014/12/04 00:34:56]

Yep, you're right.

+//    Selection Highlight --->_____|__|<-|---- Drag position reported to client
 //                              _  |  O  |
 //          Vertical Padding __|   |   <-|---- ET_GESTURE_SCROLL_UPDATE position
 //                             |_  |_____|<--- Editing handle widget
 //
 //                                 | |
 //                                  T
 //                          Horizontal Padding
 //
 const int kSelectionHandleVerticalDragOffset = 5;
 
@@ skipping 63 matching lines @@
     case ui::SelectionBound::CENTER:
       return GetCenterHandleImage();
     case ui::SelectionBound::RIGHT:
       return GetRightHandleImage();
     default:
       NOTREACHED() << "Invalid touch handle bound type.";
       return nullptr;
   };
 }
 
-enum Alignment {ALIGN_LEFT, ALIGN_CENTER, ALIGN_RIGHT};
-
-// Determine the cursor line alignment to the handle image based on the bound
-// type. Depends on the visual shapes of the handle image assets.
-Alignment GetCursorAlignment(ui::SelectionBound::Type bound_type) {
-  switch (bound_type) {
-    case ui::SelectionBound::LEFT:
-      return ALIGN_RIGHT;
-    case ui::SelectionBound::RIGHT:
-      return ALIGN_LEFT;
-    case ui::SelectionBound::CENTER:
-      return ALIGN_CENTER;
-    default:
-      NOTREACHED() << "Undefined bound type for cursor alignment.";
-      return ALIGN_LEFT;
-  };
-}
-
 // Calculates the bounds of the widget containing the selection handle based
 // on the SelectionBound's type and location
 gfx::Rect GetSelectionWidgetBounds(const ui::SelectionBound& bound) {
-  Alignment cursor_alignment = GetCursorAlignment(bound.type);
   gfx::Size image_size = GetHandleImage(bound.type)->Size();
-  int widget_width =  image_size.width() + 2 * kSelectionHandleHorizPadding;
+  int widget_width = image_size.width() + 2 * kSelectionHandleHorizPadding;
   int widget_height = bound.GetHeight() + image_size.height() +
-      kSelectionHandleVertPadding;
+      kSelectionHandleVerticalVisualOffset + kSelectionHandleVertPadding;
+  // Due to the shape of the handle images, the widget is aligned differently to
+  // the selection bound depending on the type of the bound.
   int widget_left = 0;
-  switch (cursor_alignment) {
-    case ALIGN_LEFT:
-      widget_left = bound.edge_top.x() - kSelectionHandleHorizPadding;
-      break;
-    case ALIGN_RIGHT:
+  switch (bound.type) {
+    case ui::SelectionBound::LEFT:
       widget_left = bound.edge_top.x() - image_size.width() -
           kSelectionHandleHorizPadding;
       break;
-    case ALIGN_CENTER:
+    case ui::SelectionBound::RIGHT:
+      widget_left = bound.edge_top.x() - kSelectionHandleHorizPadding;
+      break;
+    case ui::SelectionBound::CENTER:
       widget_left = bound.edge_top.x() - widget_width / 2;
       break;
+    default:
+      NOTREACHED() << "Undefined bound type.";
+      break;
   };
   return gfx::Rect(
       widget_left, bound.edge_top.y(), widget_width, widget_height);
 }
 
 gfx::Size GetMaxHandleImageSize() {
   gfx::Rect center_rect = gfx::Rect(GetCenterHandleImage()->Size());
   gfx::Rect left_rect = gfx::Rect(GetLeftHandleImage()->Size());
   gfx::Rect right_rect = gfx::Rect(GetRightHandleImage()->Size());
   gfx::Rect union_rect = center_rect;
@@ skipping 81 matching lines @@
 
   ~EditingHandleView() override { SetWidgetVisible(false, false); }
 
   // Overridden from views::WidgetDelegateView:
   bool WidgetHasHitTestMask() const override { return true; }
 
   void GetWidgetHitTestMask(gfx::Path* mask) const override {
     gfx::Size image_size = image_->Size();
     mask->addRect(
         SkIntToScalar(0),
-        SkIntToScalar(selection_bound_.GetHeight()),
+        SkIntToScalar(selection_bound_.GetHeight() +
+                      kSelectionHandleVerticalVisualOffset),
         SkIntToScalar(image_size.width()) + 2 * kSelectionHandleHorizPadding,
-        SkIntToScalar(selection_bound_.GetHeight() + image_size.height() +
-                      kSelectionHandleVertPadding));
+        SkIntToScalar(selection_bound_.GetHeight() +
+                      kSelectionHandleVerticalVisualOffset +
+                      image_size.height() + kSelectionHandleVertPadding));
   }
 
   void DeleteDelegate() override {
     // We are owned and deleted by TouchSelectionController.
   }
 
   // Overridden from views::View:
   void OnPaint(gfx::Canvas* canvas) override {
     if (draw_invisible_)
       return;
 
-    Alignment cursor_alignment = GetCursorAlignment(selection_bound_.type);
-    int cursor_x = 0;
-    switch (cursor_alignment) {
-      case ALIGN_RIGHT:
-        cursor_x =
-            selection_bound_.edge_top.x() - kSelectionHandleLineWidth + 1;
-        break;
-      case ALIGN_LEFT:
-        cursor_x = selection_bound_.edge_top.x() - 1;
-        break;
-      case ALIGN_CENTER:
-        cursor_x =
-            selection_bound_.edge_top.x() - kSelectionHandleLineWidth / 2;
-        break;
-    };
-    // Draw the cursor line.
-    canvas->FillRect(gfx::Rect(cursor_x,
-                               0,
-                               kSelectionHandleLineWidth,
-                               selection_bound_.GetHeight()),
-                     kSelectionHandleLineColor);
     // Draw the handle image.
-    canvas->DrawImageInt(*image_->ToImageSkia(),
-        kSelectionHandleHorizPadding, selection_bound_.GetHeight());
+    canvas->DrawImageInt(
+        *image_->ToImageSkia(),
+        kSelectionHandleHorizPadding,
+        selection_bound_.GetHeight() + kSelectionHandleVerticalVisualOffset);
   }
 
   void OnGestureEvent(ui::GestureEvent* event) override {
     event->SetHandled();
     switch (event->type()) {
       case ui::ET_GESTURE_SCROLL_BEGIN: {
         widget_->SetCapture(this);
         controller_->SetDraggingHandle(this);
         // Distance from the point which is |kSelectionHandleVerticalDragOffset|
-        // pixels above the bottom of the handle's cursor line to the event
+        // pixels above the bottom of the selection bound edge to the event
         // location (aka the touch-drag point).
         drag_offset_ = selection_bound_.edge_bottom -
                        gfx::Vector2d(0, kSelectionHandleVerticalDragOffset) -
                        event->location();
         break;
       }
       case ui::ET_GESTURE_SCROLL_UPDATE: {
         controller_->SelectionHandleDragged(event->location() + drag_offset_);
         break;
       }
       case ui::ET_GESTURE_SCROLL_END:
       case ui::ET_SCROLL_FLING_START:
         widget_->ReleaseCapture();
         controller_->SetDraggingHandle(nullptr);
         break;
       default:
         break;
     }
   }
 
   gfx::Size GetPreferredSize() const override {
-    gfx::Size image_size = image_->Size();
-    return gfx::Size(image_size.width() + 2 * kSelectionHandleHorizPadding,
-                     image_size.height() + selection_bound_.GetHeight() +
-                         kSelectionHandleVertPadding);
+    return GetSelectionWidgetBounds(selection_bound_).size();

[mohsen, 2014/12/02 22:34:56]

What about adding a helper function that calculates the size and use it both
here and in GetSelectionWidgetBounds()? (I don't like the fact that
GetSelectionWidgetBounds() does some calculations that is thrown away here!)

[mfomitchev, 2014/12/03 23:57:20]

This would be worthwhile if this was called a bunch, but that's not the case. In
fact I put a log statement in GetPreferredSize and it never hit. Maybe it gets
called under some conditions, but I couldn't hit it.
Also, if we add this helper function (lets say GetSelectionWidgetSize),
presumably we would want to call it from GetSelectionWidgetBounds to avoid code
duplication. Both GetSelectionWidgetBounds and GetSelectionWidgetSize need to
calculate the image size, so we'd be calculating the image size twice when we
call GetSelectionWidgetBounds. There's also a minor increase in code size.

[mohsen, 2014/12/04 00:34:56]

I see. OK.

   }
 
   bool IsWidgetVisible() const {
     return widget_->IsVisible();
   }
 
   void SetWidgetVisible(bool visible, bool quick) {
     if (widget_->IsVisible() == visible)
       return;
     widget_->SetVisibilityAnimationDuration(
@@ skipping 405 matching lines @@
 
 views::WidgetDelegateView* TouchSelectionControllerImpl::GetHandle1View() {
   return selection_handle_1_.get();
 }
 
 views::WidgetDelegateView* TouchSelectionControllerImpl::GetHandle2View() {
   return selection_handle_2_.get();
 }
 
 }  // namespace views