Support double-tap to click in touch accessibility controller.

ui/chromeos/touch_exploration_controller.cc

Index: ui/chromeos/touch_exploration_controller.cc
diff --git a/ui/chromeos/touch_exploration_controller.cc b/ui/chromeos/touch_exploration_controller.cc
index 57457697f3c294a28dd161b1668c580c3fd7d358..5cbe80636a1936d8bd801bab158d3d098bfb88f5 100644
--- a/ui/chromeos/touch_exploration_controller.cc
+++ b/ui/chromeos/touch_exploration_controller.cc
@@ -7,14 +7,19 @@
 #include "base/logging.h"
 #include "ui/aura/client/cursor_client.h"
 #include "ui/aura/window.h"
+#include "ui/aura/window_event_dispatcher.h"
 #include "ui/aura/window_tree_host.h"
 #include "ui/events/event.h"
+#include "ui/events/event_processor.h"
 
 namespace ui {
 
 TouchExplorationController::TouchExplorationController(
     aura::Window* root_window)
-        : root_window_(root_window) {
+      : root_window_(root_window),
+        initial_touch_id_passthrough_mapping_(0),
+        state_(NO_FINGERS_DOWN),
+        event_handler_for_testing_(NULL) {
   CHECK(root_window);
   root_window->GetHost()->GetEventSource()->AddEventRewriter(this);
 }
@@ -23,6 +28,17 @@ TouchExplorationController::~TouchExplorationController() {
   root_window_->GetHost()->GetEventSource()->RemoveEventRewriter(this);
 }
 
+void TouchExplorationController::CallTapTimerNowForTesting() {
+  CHECK(tap_timer_.IsRunning());
+  tap_timer_.Stop();
+  OnTapTimerFired();
+}
+
+void TouchExplorationController::SetEventHandlerForTesting(
+    ui::EventHandler* event_handler_for_testing) {
+  event_handler_for_testing_ = event_handler_for_testing;
+}
+
 ui::EventRewriteStatus TouchExplorationController::RewriteEvent(
     const ui::Event& event,
     scoped_ptr<ui::Event>* rewritten_event) {
@@ -34,113 +50,269 @@ ui::EventRewriteStatus TouchExplorationController::RewriteEvent(
   const ui::EventType type = touch_event.type();
   const gfx::PointF& location = touch_event.location_f();
   const int touch_id = touch_event.touch_id();
-  const int flags = touch_event.flags();
 
+  // Always update touch ids and touch locations, so we can use those
+  // no matter what state we're in.
   if (type == ui::ET_TOUCH_PRESSED) {
     touch_ids_.push_back(touch_id);
     touch_locations_.insert(std::pair<int, gfx::PointF>(touch_id, location));
-    // If this is the first and only finger touching - rewrite the touch as a
-    // mouse move. Otherwise let the it go through as is.
-    if (touch_ids_.size() == 1) {
-      *rewritten_event = CreateMouseMoveEvent(location, flags);
-      EnterTouchToMouseMode();
-      return ui::EVENT_REWRITE_REWRITTEN;
-    }
-    return ui::EVENT_REWRITE_CONTINUE;
   } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {
     std::vector<int>::iterator it =
         std::find(touch_ids_.begin(), touch_ids_.end(), touch_id);
-    // We may fail to find the finger if the exploration mode was turned on
-    // while the user had some fingers touching the screen. We simply ignore
-    // those fingers for the purposes of event transformation.
+
+    // Can happen if touch exploration is enabled while fingers were down.
     if (it == touch_ids_.end())
       return ui::EVENT_REWRITE_CONTINUE;
-    const bool first_finger_released = it == touch_ids_.begin();
+
     touch_ids_.erase(it);
-    int num_erased = touch_locations_.erase(touch_id);
-    DCHECK_EQ(num_erased, 1);
-    const int num_fingers_remaining = touch_ids_.size();
+    touch_locations_.erase(touch_id);
+  } else if (type == ui::ET_TOUCH_MOVED) {
+    std::vector<int>::iterator it =
+        std::find(touch_ids_.begin(), touch_ids_.end(), touch_id);
 
-    if (num_fingers_remaining == 0) {
-      *rewritten_event = CreateMouseMoveEvent(location, flags);
-      return ui::EVENT_REWRITE_REWRITTEN;
-    }
+    // Can happen if touch exploration is enabled while fingers were down.
+    if (it == touch_ids_.end())
+      return ui::EVENT_REWRITE_CONTINUE;
 
-    // If we are left with one finger - enter the mouse move mode.
-    const bool enter_mouse_move_mode = num_fingers_remaining == 1;
+    touch_locations_[*it] = location;
+  }
 
-    if (!enter_mouse_move_mode && !first_finger_released) {
-      // No special handling needed.
-      return ui::EVENT_REWRITE_CONTINUE;
-    }
+  // The rest of the processing depends on what state we're in.
+  switch(state_) {
+    case NO_FINGERS_DOWN:
+      return OnNoFingersDown(touch_event, rewritten_event);
+    case GRACE_PERIOD:
+      return OnGracePeriod(touch_event, rewritten_event);
+    case TOUCH_EXPLORATION:
+      return OnTouchExploration(touch_event, rewritten_event);
+    case SINGLE_TAP_PENDING:
+      return OnSingleTapPending(touch_event, rewritten_event);
+    case DOUBLE_TAP_PRESSED:
+      return OnDoubleTapPressed(touch_event, rewritten_event);
+    case PASSTHROUGH_MINUS_ONE:
+      return OnPassthroughMinusOne(touch_event, rewritten_event);
+  }
 
-    // If the finger which was released was the first one, - we need to rewrite
-    // the release event as a release of the was second / now first finger.
-    // This is the finger which will now be getting "substracted".
-    if (first_finger_released) {
-      int rewritten_release_id = touch_ids_[0];
-      const gfx::PointF& rewritten_release_location =
-          touch_locations_[rewritten_release_id];
-      ui::TouchEvent* rewritten_release_event = new ui::TouchEvent(
-          ui::ET_TOUCH_RELEASED,
-          rewritten_release_location,
-          rewritten_release_id,
-          event.time_stamp());
-      rewritten_release_event->set_flags(touch_event.flags());
-      rewritten_event->reset(rewritten_release_event);
-    } else if (enter_mouse_move_mode) {
-      // Dispatch the release event as is.
-      // TODO(mfomitchev): We can get rid of this clause once we have
-      // EVENT_REWRITE_DISPATCH_ANOTHER working without having to set
-      // rewritten_event.
-      rewritten_event->reset(new ui::TouchEvent(touch_event));
-    }
+  NOTREACHED();
+  return ui::EVENT_REWRITE_CONTINUE;
+}
 
-    if (enter_mouse_move_mode) {
-      // Since we are entering the mouse move mode - also dispatch a mouse move
-      // event at the location of the one remaining finger. (num_fingers == 1)
-      const gfx::PointF& mouse_move_location = touch_locations_[touch_ids_[0]];
-      next_dispatch_event_ =
-          CreateMouseMoveEvent(mouse_move_location, flags).Pass();
-      return ui::EVENT_REWRITE_DISPATCH_ANOTHER;
+ui::EventRewriteStatus TouchExplorationController::NextDispatchEvent(
+    const ui::Event& last_event, scoped_ptr<ui::Event>* new_event) {
+  NOTREACHED();
+  return ui::EVENT_REWRITE_CONTINUE;
+}
+
+ui::EventRewriteStatus TouchExplorationController::OnNoFingersDown(
+    const ui::TouchEvent& event, scoped_ptr<ui::Event>* rewritten_event) {
+  const ui::EventType type = event.type();
+  if (type == ui::ET_TOUCH_PRESSED) {
+    initial_press_.reset(new TouchEvent(event));
+    tap_timer_.Start(FROM_HERE,
+                     gesture_detector_config_.double_tap_timeout,
+                     this,
+                     &TouchExplorationController::OnTapTimerFired);
+    state_ = GRACE_PERIOD;
+    return ui::EVENT_REWRITE_DISCARD;
+  } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {

[aboxhall, 2014/06/02 16:33:14]

What's the point of these last two if blocks? They both do the same thing, and
do essentially the same thing as the fall-through as well.

[dmazzoni, 2014/06/03 07:46:51]

You're right. Simplified.

+    NOTREACHED();
+    return ui::EVENT_REWRITE_CONTINUE;
+  } else if (type == ui::ET_TOUCH_MOVED) {
+    NOTREACHED();
+    return ui::EVENT_REWRITE_CONTINUE;
+  }
+  NOTREACHED() << "Unexpected event type received.";
+  return ui::EVENT_REWRITE_CONTINUE;
+}
+
+ui::EventRewriteStatus TouchExplorationController::OnGracePeriod(
+    const ui::TouchEvent& event, scoped_ptr<ui::Event>* rewritten_event) {
+  const ui::EventType type = event.type();
+  if (event.time_stamp() - initial_press_->time_stamp() >

[aboxhall, 2014/06/02 16:33:14]

Even though it's reasonably self-explanatory, it might be helpful to have a
brief comment here explaining the early return.

[dmazzoni, 2014/06/03 07:46:51]

Done.

+          gesture_detector_config_.double_tap_timeout) {
+    EnterTouchToMouseMode();
+    state_ = TOUCH_EXPLORATION;
+    return OnTouchExploration(event, rewritten_event);

[mfomitchev, 2014/06/02 23:12:18]

I think there still is a problem:

Suppose event is a press. We will call OnTouchExploration, which will exit
discarding the event. The state will be changed, so when the timer fires there
will be no mouse move dispatched. So we will be in touch exploration mode, but
we won't dispatch any mouse moves, which is wrong.

I think it wold be good to write a unit test for this case - the case where an
event arrives after the timeout expires but before the timer fires.

[dmazzoni, 2014/06/03 07:46:51]

Good catch.

I realized that we can handle this case up in the main function rather than
here. That way all we need to do is say, if the tap handler is pending and
should be called, but it hasn't been called yet, call it now - then we can do
the switch and handle the event based on the new state.

I added two new tests for this.

+  }
+
+  if (type == ui::ET_TOUCH_PRESSED) {
+    // Adding a second finger within the timeout period switches to
+    // passthrough.
+    state_ = PASSTHROUGH_MINUS_ONE;
+    return OnPassthroughMinusOne(event, rewritten_event);
+  } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {
+    DCHECK_EQ(0U, touch_ids_.size());
+    state_ = SINGLE_TAP_PENDING;
+    return EVENT_REWRITE_DISCARD;
+  } else if (type == ui::ET_TOUCH_MOVED) {
+    // If the user moves far enough from the initial touch location (outside
+    // the "slop" region, jump to the touch exploration mode early. Note that

[aboxhall, 2014/06/02 16:33:14]

Rephrase this note as an explicit TODO?

[dmazzoni, 2014/06/03 07:46:51]

Done.

+    // when we add gesture recognition, we should probably jump to gesture
+    // mode here if the velocity is high enough, and touch exploration if
+    // the velocity is lower.
+    float delta = (event.location() - initial_press_->location()).Length();
+    if (delta > gesture_detector_config_.touch_slop) {
+      EnterTouchToMouseMode();
+      state_ = TOUCH_EXPLORATION;
+      return OnTouchExploration(event, rewritten_event);
     }
+
+    return EVENT_REWRITE_DISCARD;
+  }
+  NOTREACHED() << "Unexpected event type received.";
+  return ui::EVENT_REWRITE_CONTINUE;
+}
+
+ui::EventRewriteStatus TouchExplorationController::OnTouchExploration(
+    const ui::TouchEvent& event, scoped_ptr<ui::Event>* rewritten_event) {
+  const ui::EventType type = event.type();
+  if (type == ui::ET_TOUCH_PRESSED) {
+    // Ignore any additional fingers when we're already in touch exploration
+    // mode. Later, we should support "split-tap".

[aboxhall, 2014/06/02 16:33:14]

TODO?

[dmazzoni, 2014/06/03 07:46:51]

Done.

+    return ui::EVENT_REWRITE_DISCARD;
+  } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {
+    if (touch_ids_.size() == 0)
+      state_ = NO_FINGERS_DOWN;
+  } else if (type != ui::ET_TOUCH_MOVED) {
+    NOTREACHED() << "Unexpected event type received.";
+    return ui::EVENT_REWRITE_CONTINUE;
+  }
+
+  // Rewrite as a mouse-move event.
+  *rewritten_event = CreateMouseMoveEvent(event.location(), event.flags());
+  last_touch_exploration_location_ = event.location();
+  return ui::EVENT_REWRITE_REWRITTEN;
+}
+
+ui::EventRewriteStatus TouchExplorationController::OnSingleTapPending(
+    const ui::TouchEvent& event, scoped_ptr<ui::Event>* rewritten_event) {
+  const ui::EventType type = event.type();

[mfomitchev, 2014/06/02 23:12:18]

Should we have a case here for 
(event.time_stamp() - initial_press_->time_stamp()
>gesture_detector_config_.double_tap_timeout) like we do in OnGracePeriod()?

[dmazzoni, 2014/06/03 07:46:51]

Yes, but I think it's cleaner to have it up above. Done.

+  if (type == ui::ET_TOUCH_PRESSED) {

[aboxhall, 2014/06/02 16:33:14]

Comment stating explicitly that this represents a (potential) double-tap?

[dmazzoni, 2014/06/03 07:46:51]

Done.

+    // Rewrite at location of last touch exploration.
+    ui::TouchEvent* rewritten_press_event = new ui::TouchEvent(
+        ui::ET_TOUCH_PRESSED,
+        last_touch_exploration_location_,
+        event.touch_id(),
+        event.time_stamp());
+    rewritten_press_event->set_flags(event.flags());
+    rewritten_event->reset(rewritten_press_event);
+    state_ = DOUBLE_TAP_PRESSED;
     return ui::EVENT_REWRITE_REWRITTEN;
+  } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {

[aboxhall, 2014/06/02 16:33:14]

Similarly, why are these else blocks here? Do they need TODOs?

[dmazzoni, 2014/06/03 07:46:51]

Done.

+    NOTREACHED();
+    return ui::EVENT_REWRITE_CONTINUE;
   } else if (type == ui::ET_TOUCH_MOVED) {
-    std::vector<int>::iterator it =
-        std::find(touch_ids_.begin(), touch_ids_.end(), touch_id);
-    // We may fail to find the finger if the exploration mode was turned on
-    // while the user had some fingers touching the screen. We simply ignore
-    // those fingers for the purposes of event transformation.
-    if (it == touch_ids_.end())
-      return ui::EVENT_REWRITE_CONTINUE;
-    touch_locations_[*it] = location;
-    if (touch_ids_.size() == 1) {
-      // Touch moves are rewritten as mouse moves when there's only one finger
-      // touching the screen.
-      *rewritten_event = CreateMouseMoveEvent(location, flags).Pass();
+    NOTREACHED();
+    return ui::EVENT_REWRITE_CONTINUE;
+  }
+  NOTREACHED() << "Unexpected event type received.";
+  return ui::EVENT_REWRITE_CONTINUE;
+}
+
+ui::EventRewriteStatus TouchExplorationController::OnDoubleTapPressed(
+    const ui::TouchEvent& event, scoped_ptr<ui::Event>* rewritten_event) {
+  const ui::EventType type = event.type();
+  if (type == ui::ET_TOUCH_PRESSED) {

[aboxhall, 2014/06/02 16:33:14]

How could we get a TOUCH_PRESSED directly after a TOUCH_PRESSED without a
TOUCH_RELEASED or TOUCH_CANCELLED in between?

[dmazzoni, 2014/06/03 07:46:51]

You have lots of fingers. :)

+    return ui::EVENT_REWRITE_DISCARD;
+  } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {
+    if (touch_ids_.size() != 0)

[aboxhall, 2014/06/02 16:33:14]

It took me quite a while to work out what was going on here. Perhaps touch_ids_
might be more clearly named active_touch_ids_ or something?

[dmazzoni, 2014/06/03 07:46:51]

Sure, how about current_touch_ids_

+      return EVENT_REWRITE_DISCARD;
+
+    // Rewrite at location of last touch exploration.
+    ui::TouchEvent* rewritten_release_event = new ui::TouchEvent(
+        ui::ET_TOUCH_RELEASED,
+        last_touch_exploration_location_,
+        event.touch_id(),
+        event.time_stamp());
+    rewritten_release_event->set_flags(event.flags());
+    rewritten_event->reset(rewritten_release_event);
+    state_ = NO_FINGERS_DOWN;
+    return ui::EVENT_REWRITE_REWRITTEN;
+  } else if (type == ui::ET_TOUCH_MOVED) {
+    return ui::EVENT_REWRITE_DISCARD;
+  }
+  NOTREACHED() << "Unexpected event type received.";
+  return ui::EVENT_REWRITE_CONTINUE;
+}
+
+ui::EventRewriteStatus TouchExplorationController::OnPassthroughMinusOne(
+    const ui::TouchEvent& event, scoped_ptr<ui::Event>* rewritten_event) {
+  ui::EventType type = event.type();
+  gfx::PointF location = event.location_f();
+
+  if (type == ui::ET_TOUCH_PRESSED) {
+    return ui::EVENT_REWRITE_CONTINUE;

[mfomitchev, 2014/06/02 23:12:18]

Nit: It might make sense to get rid of this if. The only special case is
release/cancel. The moves and the presses can be handled by the code after the
if. We can also get rid of the if for unrecognized types - just apply the same
rules to them as to moves/presses.. or add a DCHECK(). Might make the code
easier to rid if there's just one if instead of three if/else.

[dmazzoni, 2014/06/03 07:46:51]

Done.

+  } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {
+    if (touch_ids_.size() == 0)
+      state_ = NO_FINGERS_DOWN;

[mfomitchev, 2014/06/02 23:12:18]

We also need to set initial_touch_id_passthrough_mapping_ to 0 here. Might make
sense to create some sort of a Reset() method setting the state to
NO_FINGERS_DOWN and clearing all the data structures as needed and use that
method whenever we set state to NO_FINGERS_DOWN.

[dmazzoni, 2014/06/03 07:46:51]

Done.

+
+    bool first_finger_still_down =

[aboxhall, 2014/06/02 16:33:14]

Does the rest of this need to go inside this if block? I find it confusing that
half of the logic is in here and half is outside the if block. Given each other
branch early-returns, I think we could move it below.

[dmazzoni, 2014/06/03 07:46:51]

No, the ET_TOUCH_MOVED case doesn't early-return.

+        touch_ids_.size() >= 1 &&
+        touch_ids_[0] == initial_press_->touch_id();
+
+    if (first_finger_still_down &&
+        touch_ids_.size() == 1 &&

[mfomitchev, 2014/06/02 23:12:18]

Hmm.. why do we only do this when there's only one finger remaining now? In the
previous version we activated the mapping on any release regardless of the
number of fingers.

Also, I think we should set initial_touch_id_passthrough_mapping to -1 or
similar when the first finger is released. This is because I think the id may
get reused if this (or another) finger is pressed again after it is released.
Also if we do this, the condition code could be simplified: we can get rid of
first_finger_still_down_ and just have:

if (initial_touch_id_passthrough_mapping == 0) {
  if (event.touch_id() == initial_press_->touch_id()) {
    initial_touch_id_passthrough_mapping = -1;
    // Can return DISCARD here or let it fall through and return below
} else {
    // The current rewriting code
  }
}

[dmazzoni, 2014/06/03 07:46:51]

Sure, this is a bit simpler.

+        event.touch_id() != initial_press_->touch_id() &&
+        initial_touch_id_passthrough_mapping_ == 0) {
+      // If the only finger now remaining is the first finger,
+      // rewrite as a move to the location of the first finger.
+      initial_touch_id_passthrough_mapping_ = event.touch_id();
+      ui::TouchEvent* rewritten_passthrough_event = new ui::TouchEvent(
+          ui::ET_TOUCH_MOVED,
+          touch_locations_[initial_press_->touch_id()],
+          initial_touch_id_passthrough_mapping_,
+          event.time_stamp());
+      rewritten_passthrough_event->set_flags(event.flags());
+      rewritten_event->reset(rewritten_passthrough_event);
       return ui::EVENT_REWRITE_REWRITTEN;
     }
-    if (touch_id == touch_ids_.front()) {
-      // Touch moves of the first finger are discarded when there's more than
-      // one finger touching.
-      return ui::EVENT_REWRITE_DISCARD;
-    }
+  } else if (type != ui::ET_TOUCH_MOVED) {
+    NOTREACHED();
     return ui::EVENT_REWRITE_CONTINUE;
   }
-  NOTREACHED() << "Unexpected event type received.";
+
+  if (event.touch_id() == initial_press_->touch_id()) {
+    if (!initial_touch_id_passthrough_mapping_)

[mfomitchev, 2014/06/02 23:12:18]

This would need to be changed to <= 0 if we use -1.

[dmazzoni, 2014/06/03 07:46:51]

Done.

+      return ui::EVENT_REWRITE_DISCARD;
+
+    ui::TouchEvent* rewritten_passthrough_event = new ui::TouchEvent(
+        type,
+        location,
+        initial_touch_id_passthrough_mapping_,

[aboxhall, 2014/06/02 16:33:14]

I don't follow this either. It looks like we only ever pass through the original
touch ID? Do we only handle the two-finger case?

[dmazzoni, 2014/06/03 07:46:51]

When you press two fingers, the first finger is ignored and the second one is
passed through as-is (same touch id). The only tricky thing happens if you then
release the second finger - then the first finger gets rewritten as the second
finger's touch id from then on - that's what
initial_touch_id_passthrough_mapping_ gives us.

+        event.time_stamp());
+    rewritten_passthrough_event->set_flags(event.flags());
+    rewritten_event->reset(rewritten_passthrough_event);
+    return ui::EVENT_REWRITE_REWRITTEN;
+  }
+
   return ui::EVENT_REWRITE_CONTINUE;
 }
 
-ui::EventRewriteStatus TouchExplorationController::NextDispatchEvent(
-    const ui::Event& last_event,
-    scoped_ptr<ui::Event>* new_event) {
-  CHECK(next_dispatch_event_);
-  DCHECK(last_event.IsTouchEvent());
-  *new_event = next_dispatch_event_.Pass();
-  // Enter the mouse move mode if needed
-  if ((*new_event)->IsMouseEvent())
+void TouchExplorationController::OnTapTimerFired() {
+  if (state_ != SINGLE_TAP_PENDING && state_ != GRACE_PERIOD)
+    return;
+
+  if (state_ == SINGLE_TAP_PENDING) {
+    state_ = NO_FINGERS_DOWN;
+  } else {
     EnterTouchToMouseMode();
-  return ui::EVENT_REWRITE_REWRITTEN;
+    state_ = TOUCH_EXPLORATION;
+  }
+
+  scoped_ptr<ui::Event> mouse_move = CreateMouseMoveEvent(
+      initial_press_->location(), initial_press_->flags());
+  DispatchEvent(mouse_move.get());
+  last_touch_exploration_location_ = initial_press_->location();
+}
+
+void TouchExplorationController::DispatchEvent(ui::Event* event) {
+  if (event_handler_for_testing_) {
+    event_handler_for_testing_->OnEvent(event);
+    return;
+  }
+
+  ui::EventDispatchDetails result ALLOW_UNUSED =
+      root_window_->GetHost()->dispatcher()->OnEventFromSource(event);
 }
 
 scoped_ptr<ui::Event> TouchExplorationController::CreateMouseMoveEvent(