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 94ceb2df046e8d7b2ed65e5350b4d6b45ea1db2e..41abb1e6d3f7aae4e6271a57e6b5f676554521aa 100644
--- a/ui/chromeos/touch_exploration_controller.cc
+++ b/ui/chromeos/touch_exploration_controller.cc
@@ -7,14 +7,17 @@
 #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),
+        state_(NO_FINGERS_DOWN) {
   CHECK(root_window);
   root_window->GetHost()->GetEventSource()->AddEventRewriter(this);
 }
@@ -34,115 +37,252 @@ 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.
-    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();
 
-    if (num_fingers_remaining == 0) {
-      *rewritten_event = CreateMouseMoveEvent(location, flags);
-      return ui::EVENT_REWRITE_REWRITTEN;
-    }
-
-    // If we are left with one finger - enter the mouse move mode.
-    const bool enter_mouse_move_mode = num_fingers_remaining == 1;
-
-    if (!enter_mouse_move_mode && !first_finger_released) {
-      // No special handling needed.
+    // Can happen if touch exploration is enabled while fingers were down.
+    if (it == touch_ids_.end())
       return ui::EVENT_REWRITE_CONTINUE;
-    }
 
-    // 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));
-    }
-
-    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;
-    }
-    return ui::EVENT_REWRITE_REWRITTEN;
+    touch_ids_.erase(it);
+    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);
-    // 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;
+
     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();
-      return ui::EVENT_REWRITE_REWRITTEN;
+  }
+
+  // The rest of the processing depends on what state we're in.

[mfomitchev, 2014/05/28 17:47:45]

I really like how you've broken it up into multiple methods.

[dmazzoni, 2014/05/31 06:53:24]

Thanks!

+  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);
+  }
+
+  NOTREACHED();
+  return ui::EVENT_REWRITE_CONTINUE;
+}
+
+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));

[mfomitchev, 2014/05/28 17:47:45]

Why create a new event instead of using the one we have?

[dmazzoni, 2014/05/31 06:53:24]

We don't have ownership of the event passed in, we need to make our own copy -
right?

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

Done.

+    tap_timer_.Start(FROM_HERE,

[mfomitchev, 2014/05/28 17:47:45]

We should probably Stop() the timer as we transition through states. It's not
quite clear to me what happens if you start the timer which has already been
started - does it update both the delay and the task or only the task?

[dmazzoni, 2014/05/31 06:53:24]

Starting the timer again does what it should - it updates both the delay and the
task.

I could call Stop, but I actually think that would complicate things for no good
reason and could potentially lead to bugs. I think it's simpler that when the
timer fires, it checks for one specific state - and if it's not in the state
that's waiting on a timer to fire, it exits. What do you think?

If we did add Stop, we'd have to add it to each transition to:
TOUCH_EXPLORATION
DOUBLE_TAP_PRESSED
PASSTHROUGH_MINUS_ONE

That's potentially several transitions. My worry is that future changes would
require updating this code.

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

Yes, makes sense.

+                     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) {
+    // Can happen if touch exploration is enabled while fingers were down.
+    return ui::EVENT_REWRITE_DISCARD;

[mfomitchev, 2014/05/28 17:47:45]

I don't think we can get here since we'd have no touch id in touch_ids_ and we'd
bail out earlier - in RewriteEvent(). If somehow we do get here, I think this
should return a continue, not a discard. Or at least it should be consistent
with what we return in RewriteEvent() for this case.

[dmazzoni, 2014/05/31 06:53:24]

I agree that it shouldn't happen, so I added a NOTREACHED.

+  } else if (type == ui::ET_TOUCH_MOVED) {
+    // Can happen if touch exploration is enabled while fingers were down.
+    return ui::EVENT_REWRITE_DISCARD;

[mfomitchev, 2014/05/28 17:47:45]

Same as above

[dmazzoni, 2014/05/31 06:53:24]

Done.

+  }
+  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() >
+          gesture_detector_config_.double_tap_timeout) {
+    EnterTouchToMouseMode();
+    state_ = TOUCH_EXPLORATION;

[mfomitchev, 2014/05/28 17:47:45]

I think you need to somehow dispatch the mouse move corresponding to
initial_press_ here. This is relevant if event is a release for example.

[dmazzoni, 2014/05/31 06:53:24]

If this is a mouse move, calling OnTouchExploration will generate the mouse
move.

I modified OnTouchExploration so that a release does get rewritten as a mouse
move event, just in case.

+    return OnTouchExploration(event, rewritten_event);
+  }
+
+  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) {
+    if (touch_ids_.size() == 0)

[mfomitchev, 2014/05/28 17:47:45]

Isn't this if superflous? Should this be a DCHECK() instead?

[dmazzoni, 2014/05/31 06:53:24]

Agreed.

+      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

[mfomitchev, 2014/05/28 17:47:45]

When we write the code to determine if this is a swipe or a slow move, we should
try to leverage GestureProviderAura as much as we can so that we don't duplicate
the gesture recognition logic here.

[dmazzoni, 2014/05/31 06:53:24]

Sounds good.

+    // the "slop" region, jump to the touch exploration mode early. Note that
+    // 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);
     }
-    if (touch_id == touch_ids_.front()) {
-      // Touch moves of the first finger are discarded when there's more than
-      // one finger touching.
+
+    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

[mfomitchev, 2014/05/28 17:47:45]

That's not how Android works. I guess we are okay with this?

[dmazzoni, 2014/05/31 06:53:24]

I think I prefer this for now, but we'll do some user testing this summer. I
think it's enough of a corner case that I'd rather implement the most
straightforward solution rather than copy Android.

+    // mode. Later, we should support "split-tap".
+    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;
       return ui::EVENT_REWRITE_DISCARD;

[mfomitchev, 2014/05/28 17:47:45]

We used to generate a mouse move for the release in this spot. Maybe this
doesn't matter - in practice I never saw a release without a prior move to the
exact location of the release. If it can happen though, dispatching a mouse move
here would be relevant.

[dmazzoni, 2014/05/31 06:53:24]

Agreed, I changed the logic to generate a mouse move here.

     }
+  } else if (type == ui::ET_TOUCH_MOVED) {
+    // Rewrite as a mouse-move event.
+    *rewritten_event = CreateMouseMoveEvent(event.location(), event.flags());
+    last_touch_exploration_location_ = event.location();
+    return ui::EVENT_REWRITE_REWRITTEN;

[mfomitchev, 2014/05/28 17:47:45]

This would get a bit nuts if you are moving multiple fingers on the screen. I
guess that's okay. However if we are dispatching mouse events for all moves
regardless of the finger, perhaps we should be dispatching moves for all presses
regardless of the finger as well for consistency's sake?

[dmazzoni, 2014/05/31 06:53:24]

I'm tempted to not worry about this case for now because it will be moot once we
implement split-tap.

+  }
+  NOTREACHED() << "Unexpected event type received.";
+  return ui::EVENT_REWRITE_CONTINUE;
+}
+
+ui::EventRewriteStatus TouchExplorationController::OnSingleTapPending(
+    const ui::TouchEvent& event, scoped_ptr<ui::Event>* rewritten_event) {
+  const ui::EventType type = event.type();
+  if (type == ui::ET_TOUCH_PRESSED) {
+    // Rewrite at location of last touch exploration.
+    ui::TouchEvent* rewritten_release_event = new ui::TouchEvent(
+        ui::ET_TOUCH_PRESSED,
+        last_touch_exploration_location_,
+        event.touch_id(),
+        event.time_stamp());
+    rewritten_release_event->set_flags(event.flags());
+    rewritten_event->reset(rewritten_release_event);
+    state_ = DOUBLE_TAP_PRESSED;
+    return ui::EVENT_REWRITE_REWRITTEN;
+  } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {
+    // Can happen if touch exploration is enabled while fingers were down.

[mfomitchev, 2014/05/28 17:47:45]

I think this is NOTREACHED() as we have code to catch this in RewriteEvent()

[dmazzoni, 2014/05/31 06:53:24]

Done.

+    return ui::EVENT_REWRITE_CONTINUE;
+  } else if (type == ui::ET_TOUCH_MOVED) {
+    // Can happen if touch exploration is enabled while fingers were down.
     return ui::EVENT_REWRITE_CONTINUE;

[mfomitchev, 2014/05/28 17:47:45]

Same

[dmazzoni, 2014/05/31 06:53:24]

Done.

   }
   NOTREACHED() << "Unexpected event type received.";
   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())
-    EnterTouchToMouseMode();
+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) {
+    return ui::EVENT_REWRITE_DISCARD;
+  } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {
+    if (touch_ids_.size() != 0)
+      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();
+  if (type == ui::ET_TOUCH_PRESSED) {
+    // As each new finger is pressed, we map its touch id to itself.
+    // Any finger not in this map will be ignored.
+    touch_id_map_[event.touch_id()] = event.touch_id();
+  } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) {
+    if (touch_ids_.size() == 0)
+      state_ = NO_FINGERS_DOWN;
+
+    // If removing a finger that was passed through, and another finger

[mfomitchev, 2014/05/28 17:47:45]

Ok, so this means you can release any finger, not necessarily the "first" finger
and continue interacting w/o "minus one". 

[dmazzoni, 2014/05/31 06:53:24]

Yes, that's the idea

+    // was not being passed through, remap the not-passed-through finger
+    // to the id of the finger that's just being released from now on.
+    int releasing_id = event.touch_id();

[mfomitchev, 2014/05/28 17:47:45]

I think this code would become a lot simpler if we kept track of the "minus one"
finger instead of keeping track of all the other fingers. We wouldn't need to do
all these searches. touch_id_map_ would  have zero entries in "minus one" mode
and one entry in "non-minus one" mode after a finger has been lifted.

[dmazzoni, 2014/05/31 06:53:24]

Good idea. We don't even need a map now, just the id to remap the first finger
to.

+    if (touch_id_map_.find(releasing_id) != touch_id_map_.end()) {
+      for (size_t i = 0; i < touch_ids_.size(); ++i) {
+        int still_down_id = touch_ids_[i];
+        if (touch_id_map_.find(still_down_id) == touch_id_map_.end()) {
+          touch_id_map_[still_down_id] = touch_id_map_[releasing_id];
+          type = ui::ET_TOUCH_MOVED;

[mfomitchev, 2014/05/28 17:47:45]

We are changing the type here, so I assume we want the move dispatched? I think
that's right, but:

- It looks like we will in the if check below and return DISCARD

- I think we also need to change the location - instead of dispatching the move
event at the release point we want to dispatch the event at the location of
still_down_id, i.e. the new effective finger location? 

[dmazzoni, 2014/05/31 06:53:24]

Good catch, done.

+          break;
+        }
+      }
+    }
+    touch_id_map_.erase(releasing_id);
+  } else if (type != ui::ET_TOUCH_MOVED) {
+    NOTREACHED() << "Unexpected event type received.";
+    return ui::EVENT_REWRITE_CONTINUE;
+  }
+
+  if (touch_id_map_.find(event.touch_id()) == touch_id_map_.end())
+    return ui::EVENT_REWRITE_DISCARD;

[mfomitchev, 2014/05/28 17:47:45]

I don't realluy understand why is this needed - see above

+
+  ui::TouchEvent* rewritten_passthrough_event = new ui::TouchEvent(

[mfomitchev, 2014/05/28 17:47:45]

It seems suboptimal to rewrite the event every time. In many cases we can just
let the event pass through. If we make the changes to tracking "minus one"
finger as I suggested above, it should be easy to deduce when to return rewrite
vs. continue.

[dmazzoni, 2014/05/31 06:53:24]

Done.

+      type,
+      event.location(),
+      touch_id_map_[event.touch_id()],
+      event.time_stamp());
+  rewritten_passthrough_event->set_flags(event.flags());
+  rewritten_event->reset(rewritten_passthrough_event);
   return ui::EVENT_REWRITE_REWRITTEN;
 }
 
+void TouchExplorationController::OnTapTimerFired() {
+  if (state_ != SINGLE_TAP_PENDING)

[mfomitchev, 2014/05/28 17:47:45]

Don't we need to dispatch a mouse move if we are in GRACE_PERIOD state?

[dmazzoni, 2014/05/31 06:53:24]

Done.

+    return;
+
+  ui::MouseEvent mouse_move(ui::ET_MOUSE_MOVED,

[mfomitchev, 2014/05/28 17:47:45]

Why not use CreateMouseMoveEvent() here?

[dmazzoni, 2014/05/31 06:53:24]

Done.

+                            initial_press_->location(),
+                            initial_press_->location(),
+                            ui::EF_IS_SYNTHESIZED,
+                            0);
+  ui::EventDispatchDetails result ALLOW_UNUSED =
+      root_window_->GetHost()->dispatcher()->OnEventFromSource(&mouse_move);
+  last_touch_exploration_location_ = initial_press_->location();
+  state_ = NO_FINGERS_DOWN;
+}
+
 scoped_ptr<ui::Event> TouchExplorationController::CreateMouseMoveEvent(
     const gfx::PointF& location,
     int flags) {
@@ -150,7 +290,7 @@ scoped_ptr<ui::Event> TouchExplorationController::CreateMouseMoveEvent(
       new ui::MouseEvent(ui::ET_MOUSE_MOVED,
                          location,
                          location,
-                         flags | ui::EF_IS_SYNTHESIZED | ui::EF_FROM_TOUCH,
+                         flags | ui::EF_IS_SYNTHESIZED,
                          0));
 }