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
+// 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
+//                            ___________
+//    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;

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

At least, line 144 has the same indentation issue.

[mfomitchev, 2014/12/09 22:32:26]

Done.

+  // 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();
   }
 
   bool IsWidgetVisible() const {
     return widget_->IsVisible();
   }
 
   void SetWidgetVisible(bool visible, bool quick) {
     if (widget_->IsVisible() == visible)
       return;
     widget_->SetVisibilityAnimationDuration(
@@ skipping 334 matching lines @@
   gfx::Rect menu_anchor;
   if (ShouldShowHandleFor(b1) && ShouldShowHandleFor(b2))
     menu_anchor = ui::RectBetweenSelectionBounds(b1_in_screen, b2_in_screen);
   else if (ShouldShowHandleFor(b1))
     menu_anchor = BoundToRect(b1_in_screen);
   else if (ShouldShowHandleFor(b2))
     menu_anchor = BoundToRect(b2_in_screen);
   else
     return;
 
+  // Enlarge the anchor rect so that the menu is offset from the text at least
+  // by the same distance the handles are offset from the text.
+  menu_anchor.Inset(0, -kSelectionHandleVerticalVisualOffset,
+                    0, -kSelectionHandleVerticalVisualOffset);

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

I think you can use Rect::Inset(int horizontal, int vertical) overload.

[mfomitchev, 2014/12/09 22:32:26]

Done.

   DCHECK(!context_menu_);
   context_menu_ = TouchEditingMenuView::Create(this, menu_anchor,
                                                GetMaxHandleImageSize(),
                                                client_view_->GetNativeView());
 }
 
 void TouchSelectionControllerImpl::StartContextMenuTimer() {
   if (context_menu_timer_.IsRunning())
     return;
   context_menu_timer_.Start(
@@ skipping 51 matching lines @@
 
 views::WidgetDelegateView* TouchSelectionControllerImpl::GetHandle1View() {
   return selection_handle_1_.get();
 }
 
 views::WidgetDelegateView* TouchSelectionControllerImpl::GetHandle2View() {
   return selection_handle_2_.get();
 }
 
 }  // namespace views