Reorders EventHandlers by priority across all EventTargets.

This is phase 3 of a refactor that allows Select-to-Speak to work
with Sticky Keys, see go/sts-with-sticky-keys for more on the
background for this change.

Not only are EventHandlers ordered by priority across all EventTargets,
but also the later an EventHandler was added to its EventTarget, the
earlier it will appear in the resulting list of EventHandlers. This is
a change for kDefault handlers, but kAccessibility and kSystem already
had this behavior. This is for consistency across all types and requires
a large change to event_dispatcher_unittest.

Also adds an additional debugging function to TestEventHandler
to get that handler's name. This was useful in writing the unittests
for this change and may be useful to others as well.

Bug: 819860
Change-Id: I4c5002fdc2cd26f0df5e94600183c3cc0cc2ff56
Reviewed-on: https://chromium-review.googlesource.com/996456Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Katie Dektar <katie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568557}

chrome/browser/chromeos/accessibility/select_to_speak_event_handler.cc

@@ -49,9 +49,6 @@ gfx::PointF GetScreenLocationFromEvent(const ui::LocatedEvent& event) {
 const ui::KeyboardCode kSpeakSelectionKey = ui::VKEY_S;
 
 SelectToSpeakEventHandler::SelectToSpeakEventHandler() {
-  // Add this to the root level EventTarget so that it is called first.
-  // TODO(katie): instead of using the root level EventTarget, just add this
-  // handler to ash::Shell::Get()->GetPrimaryRootWindow().
   if (aura::Env::GetInstanceDontCreate())
     aura::Env::GetInstanceDontCreate()->AddPreTargetHandler(
         this, ui::EventTarget::Priority::kAccessibility);

ui/events/event_target.cc

@@ -28,26 +28,10 @@ void EventTarget::AddPreTargetHandler(EventHandler* handler,
   PrioritizedHandler prioritized = PrioritizedHandler();
   prioritized.handler = handler;
   prioritized.priority = priority;
-  EventHandlerPriorityList::iterator it;
-  switch (priority) {
-    case Priority::kDefault:
-      pre_target_list_.push_back(prioritized);
-      return;
-    case Priority::kSystem:
-      // Find the beginning of the kSystem part of the list and prepend
-      // this new handler to that section.
-      // TODO(katie): We are adding this to the front of the list because
-      // previously the function PrependPreTargetHandler added items to the
-      // front in this way. See if we can simply put each item in the list and
-      // sort, or insert each item the same way, in a later change.
-      it = std::lower_bound(pre_target_list_.begin(), pre_target_list_.end(),
-                            prioritized);
-      pre_target_list_.insert(it, prioritized);
-      return;
-    case Priority::kAccessibility:
-      pre_target_list_.insert(pre_target_list_.begin(), prioritized);
-      return;
-  }
+  if (priority == Priority::kDefault)
+    pre_target_list_.push_back(prioritized);
+  else
+    pre_target_list_.insert(pre_target_list_.begin(), prioritized);
 }
 
 void EventTarget::RemovePreTargetHandler(EventHandler* handler) {
@@ -85,21 +69,24 @@ EventHandler* EventTarget::SetTargetHandler(EventHandler* target_handler) {
   return original_target_handler;
 }
 
-// TODO(katie): trigger all kAccessibility handlers in the tree first,
-// then kSystem and finally kDefault, rather than doing each set per
-// parent level.
 void EventTarget::GetPreTargetHandlers(EventHandlerList* list) {
   EventTarget* target = this;
+  EventHandlerPriorityList temp;
   while (target) {
-    EventHandlerPriorityList::reverse_iterator it, rend;
-    for (it = target->pre_target_list_.rbegin(),
-            rend = target->pre_target_list_.rend();
-        it != rend;
-        ++it) {
-      list->insert(list->begin(), it->handler);
-    }
+    // Build a composite list of EventHandlers from targets.
+    temp.insert(temp.begin(), target->pre_target_list_.begin(),
+                target->pre_target_list_.end());
     target = target->GetParentTarget();
   }
+
+  // Sort the list, keeping relative order, but making sure the
+  // accessibility handlers always go first before system, which will
+  // go before default, at all levels of EventTarget.
+  std::stable_sort(temp.begin(), temp.end());
+
+  // Add the sorted handlers to the result list, in order.
+  for (size_t i = 0; i < temp.size(); ++i)
+    list->insert(list->end(), temp[i].handler);
 }
 
 void EventTarget::GetPostTargetHandlers(EventHandlerList* list) {

ui/events/event_target_unittest.cc

@@ -76,27 +76,89 @@ TEST(EventTargetTest, HandlerOrderingComplex) {
 
   // Adding a new system or accessibility handler will insert it before others
   // of its type. Adding a new default handler puts it at the end of the list,
-  // but this will change in a later patch set.
+  // for historical reasons. Re-arranging default handlers causes test failures
+  // in many unittests and may also cause real-life bugs, so for now default
+  // still is at the end of the list.
   target.AddPreTargetHandler(&system_handler_3, EventTarget::Priority::kSystem);
-  target.AddPreTargetHandler(&default_handler_2,
+  target.AddPreTargetHandler(&default_handler_1,
                              EventTarget::Priority::kDefault);
   target.AddPreTargetHandler(&system_handler_2, EventTarget::Priority::kSystem);
   target.AddPreTargetHandler(&a11y_handler_2,
                              EventTarget::Priority::kAccessibility);
   target.AddPreTargetHandler(&system_handler_1, EventTarget::Priority::kSystem);
-  target.AddPreTargetHandler(&default_handler_1,
+  target.AddPreTargetHandler(&default_handler_2,
                              EventTarget::Priority::kDefault);
   target.AddPreTargetHandler(&a11y_handler_1,
                              EventTarget::Priority::kAccessibility);
   list = test_api.GetPreTargetHandlers();
+
   ASSERT_EQ(7u, list.size());
   EXPECT_EQ(list[0], &a11y_handler_1);
   EXPECT_EQ(list[1], &a11y_handler_2);
   EXPECT_EQ(list[2], &system_handler_1);
   EXPECT_EQ(list[3], &system_handler_2);
   EXPECT_EQ(list[4], &system_handler_3);
-  EXPECT_EQ(list[5], &default_handler_2);
+  EXPECT_EQ(list[5], &default_handler_1);
+  EXPECT_EQ(list[6], &default_handler_2);
+}
+
+TEST(EventTargetTest, HandlerOrderingAcrossEventTargets) {
+  // Child needs to be a unique pointer so that TestEventTarget::AddChild works.
+  std::unique_ptr<test::TestEventTarget> child =
+      std::make_unique<test::TestEventTarget>();
+  test::TestEventTarget parent;
+  test::TestEventHandler default_handler_1;
+  test::TestEventHandler default_handler_2;
+  test::TestEventHandler default_handler_3;
+  test::TestEventHandler system_handler_1;
+  test::TestEventHandler system_handler_2;
+  test::TestEventHandler system_handler_3;
+  test::TestEventHandler a11y_handler_1;
+  test::TestEventHandler a11y_handler_2;
+  test::TestEventHandler a11y_handler_3;
+
+  // Parent handlers should be called before children handlers.
+  parent.AddPreTargetHandler(&default_handler_1,
+                             EventTarget::Priority::kDefault);
+  parent.AddPreTargetHandler(&system_handler_2, EventTarget::Priority::kSystem);
+  parent.AddPreTargetHandler(&a11y_handler_2,
+                             EventTarget::Priority::kAccessibility);
+
+  child->AddPreTargetHandler(&default_handler_3,
+                             EventTarget::Priority::kDefault);
+  child->AddPreTargetHandler(&a11y_handler_3,
+                             EventTarget::Priority::kAccessibility);
+  child->AddPreTargetHandler(&system_handler_3, EventTarget::Priority::kSystem);
+
+  parent.AddPreTargetHandler(&system_handler_1, EventTarget::Priority::kSystem);
+  parent.AddPreTargetHandler(&default_handler_2,
+                             EventTarget::Priority::kDefault);
+  parent.AddPreTargetHandler(&a11y_handler_1,
+                             EventTarget::Priority::kAccessibility);
+
+  // Connect the parent and child in a EventTargetTestAPI.
+  EventTargetTestApi test_api(child.get());
+  parent.AddChild(std::move(child));
+
+  EventHandlerList list;
+  list = test_api.GetPreTargetHandlers();
+
+  ASSERT_EQ(9u, list.size());
+  // Parent handlers are called before child handlers, so a11y_handler_1 and
+  // 2 should be called before a11y_handler3, and similarly all the system and
+  // default handlers added to the parent should be called before those added
+  // to the child.
+  // In addition, all a11y handlers should be called before all system handlers,
+  // which should be called before all default handlers.
+  EXPECT_EQ(list[0], &a11y_handler_1);
+  EXPECT_EQ(list[1], &a11y_handler_2);
+  EXPECT_EQ(list[2], &a11y_handler_3);
+  EXPECT_EQ(list[3], &system_handler_1);
+  EXPECT_EQ(list[4], &system_handler_2);
+  EXPECT_EQ(list[5], &system_handler_3);
   EXPECT_EQ(list[6], &default_handler_1);
+  EXPECT_EQ(list[7], &default_handler_2);
+  EXPECT_EQ(list[8], &default_handler_3);
 }
 
 }  // namespace

ui/events/test/test_event_handler.h

@@ -38,6 +38,7 @@ class TestEventHandler : public EventHandler {
   void set_handler_name(const std::string& handler_name) {
     handler_name_ = handler_name;
   }
+  const std::string& handler_name() const { return handler_name_; }
 
   // EventHandler overrides:
   void OnKeyEvent(KeyEvent* event) override;