Initial patch set to implement improved touch support for screen magnification

ash/magnifier/magnification_controller.cc

Index: ash/magnifier/magnification_controller.cc
diff --git a/ash/magnifier/magnification_controller.cc b/ash/magnifier/magnification_controller.cc
index c677caae5c56b2b741f3a8ae9c87dffc785470b0..59c17d71b66844335bd40b66ba20f0a4d6c7c938 100644
--- a/ash/magnifier/magnification_controller.cc
+++ b/ash/magnifier/magnification_controller.cc
@@ -20,6 +20,7 @@
 #include "base/command_line.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/timer/timer.h"
+#include "chromeos/chromeos_switches.h"
 #include "ui/aura/client/cursor_client.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_tree_host.h"
@@ -34,6 +35,7 @@
 #include "ui/display/screen.h"
 #include "ui/events/event.h"
 #include "ui/events/event_handler.h"
+#include "ui/events/gesture_event_details.h"
 #include "ui/gfx/geometry/point3_f.h"
 #include "ui/gfx/geometry/point_conversions.h"
 #include "ui/gfx/geometry/point_f.h"
@@ -191,6 +193,7 @@ class MagnificationControllerImpl : public MagnificationController,
   void OnMouseEvent(ui::MouseEvent* event) override;
   void OnScrollEvent(ui::ScrollEvent* event) override;
   void OnTouchEvent(ui::TouchEvent* event) override;
+  void OnGestureEvent(ui::GestureEvent* event) override;
 
   // Moves the view port when |point| is located within
   // |x_panning_margin| and |y_pannin_margin| to the edge of the visible
@@ -261,6 +264,9 @@ class MagnificationControllerImpl : public MagnificationController,
   // mode.
   bool disable_move_magnifier_delay_;
 
+  // Flag whether user is currently changing zoom level by touch or not.
+  bool is_changing_zoom_level_by_touch_;
+
   DISALLOW_COPY_AND_ASSIGN(MagnificationControllerImpl);
 };
 
@@ -275,7 +281,8 @@ MagnificationControllerImpl::MagnificationControllerImpl()
       move_cursor_after_animation_(false),
       scale_(kNonMagnifiedScale),
       scroll_direction_(SCROLL_NONE),
-      disable_move_magnifier_delay_(false) {
+      disable_move_magnifier_delay_(false),
+      is_changing_zoom_level_by_touch_(false) {
   Shell::GetInstance()->AddPreTargetHandler(this);
   root_window_->AddObserver(this);
   point_of_interest_ = root_window_->bounds().CenterPoint();
@@ -690,6 +697,47 @@ void MagnificationControllerImpl::OnTouchEvent(ui::TouchEvent* event) {
   }
 }
 
+void MagnificationControllerImpl::OnGestureEvent(ui::GestureEvent* event) {
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kEnableTouchSupportInScreenMagnification)) {
+    return;
+  }
+
+  const ui::GestureEventDetails& details = event->details();
+  if (details.type() == ui::ET_GESTURE_SCROLL_UPDATE &&
+      details.touch_points() == 2 && !is_changing_zoom_level_by_touch_) {
+    gfx::Rect viewport_rect_in_dip = GetViewportRect();
+    viewport_rect_in_dip.Offset(details.scroll_x() * -1,
+                                details.scroll_y() * -1);
+    const gfx::Rect& viewport_rect_in_pixel =
+        ui::ConvertRectToPixel(root_window_->layer(), viewport_rect_in_dip);
+    MoveWindow(viewport_rect_in_pixel.origin(), false);
+    event->SetHandled();
+  } else if (details.type() == ui::ET_GESTURE_PINCH_BEGIN &&

[yoshiki, 2017/01/19 06:30:20]

As talked offline, could you check if this logic works when the number of
touched fingers are increased (or decreased) during pinch?

[yawano, 2017/01/19 06:45:55]

I've tested this and we get gesture end event and begin event when user increase
or decrease number of fingers. But it may be safer not to check number of touch
points for toggling the flag. I'll upload new patch set to fix this.

[yawano, 2017/01/19 07:54:25]

Done.

[yoshiki, 2017/01/20 05:26:33]

If "we get gesture end event and begin event when user increase
or decrease number of fingers" is correct, I think we can remove the
"is_performing_pinch_gesture_" flag entirely, because the pinch uses 3 fingers
and scroll uses 2 fingers, right?

And could you add comments about the gesture event behavior to prevent feature
us from having same question like current us.

[yawano, 2017/02/06 06:49:04]

Removed is_performing_pinch_gesture_ flag. We can distinguish them by number of
fingers.

The logic is very simple now. If user is performing scroll with 2 fingers, we
will scroll the screen. If user is performing pinch gesture with 3 fingers, we
will change zoom level.

Do we still need to add comment about the behavior of gesture events, especially
about begin and end events? We no longer handle gesture begin and end events
here.

+             details.touch_points() == 3) {
+    // Set this flag to prevent scrolling while user is zooming.
+    is_changing_zoom_level_by_touch_ = true;
+    event->SetHandled();
+  } else if (details.type() == ui::ET_GESTURE_PINCH_END &&
+             details.touch_points() == 3) {
+    is_changing_zoom_level_by_touch_ = false;
+    event->SetHandled();
+  } else if (details.type() == ui::ET_GESTURE_PINCH_UPDATE &&
+             details.touch_points() == 3) {
+    float scale = GetScale() * details.scale();
+
+    if (scale < kMinMagnifiedScaleThreshold) {
+      scale = kMinMagnifiedScaleThreshold;
+    } else if (scale > kMaxMagnifiedScaleThreshold) {
+      scale = kMaxMagnifiedScaleThreshold;
+    }
+
+    point_of_interest_ = event->root_location();
+    SetScale(scale, false);
+    event->SetHandled();
+  }
+}
+
 void MagnificationControllerImpl::MoveMagnifierWindowFollowPoint(
     const gfx::Point& point,
     int x_panning_margin,