[go: up one dir, main page]

Ensure spat nav doesn't use an enter key press that was already used.

Some elements click on down or press, and spat nav clicks on up, so if
the down or press for an enter key event were handled, we shouldn't also
handle the up as a click.

(cherry picked from commit cdd0902ffcf59a4ecea78dc81d5a7751c37e30c7)

Bug: 973520
Change-Id: I2017da39b97d205e5dab4fa39c5b05e666e8677a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1656014
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#669314}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1662495
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/branch-heads/3809@{#348}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}
diff --git a/third_party/blink/renderer/core/dom/events/event_dispatcher.cc b/third_party/blink/renderer/core/dom/events/event_dispatcher.cc
index 90c8500..385c6f1 100644
--- a/third_party/blink/renderer/core/dom/events/event_dispatcher.cc
+++ b/third_party/blink/renderer/core/dom/events/event_dispatcher.cc
@@ -36,6 +36,7 @@
 #include "third_party/blink/renderer/core/dom/events/event_path.h"
 #include "third_party/blink/renderer/core/dom/events/scoped_event_queue.h"
 #include "third_party/blink/renderer/core/dom/events/window_event_context.h"
+#include "third_party/blink/renderer/core/events/keyboard_event.h"
 #include "third_party/blink/renderer/core/events/mouse_event.h"
 #include "third_party/blink/renderer/core/frame/ad_tracker.h"
 #include "third_party/blink/renderer/core/frame/deprecation.h"
@@ -45,6 +46,8 @@
 #include "third_party/blink/renderer/core/frame/use_counter.h"
 #include "third_party/blink/renderer/core/html/html_element.h"
 #include "third_party/blink/renderer/core/inspector/inspector_trace_events.h"
+#include "third_party/blink/renderer/core/page/page.h"
+#include "third_party/blink/renderer/core/page/spatial_navigation_controller.h"
 #include "third_party/blink/renderer/core/timing/event_timing.h"
 #include "third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h"
 
@@ -362,6 +365,15 @@
     }
   }
 
+  if (Page* page = node_->GetDocument().GetPage()) {
+    if (page->GetSettings().GetSpatialNavigationEnabled() &&
+        is_trusted_or_click && event_->IsKeyboardEvent() &&
+        ToKeyboardEvent(*event_).key() == "Enter" &&
+        event_->type() == event_type_names::kKeyup) {
+      page->GetSpatialNavigationController().ResetEnterKeyState();
+    }
+  }
+
   // Track the usage of sending a mousedown event to a select element to force
   // it to open. This measures a possible breakage of not allowing untrusted
   // events to open select boxes.
diff --git a/third_party/blink/renderer/core/html/forms/radio_input_type.cc b/third_party/blink/renderer/core/html/forms/radio_input_type.cc
index 730db85..f4f691d 100644
--- a/third_party/blink/renderer/core/html/forms/radio_input_type.cc
+++ b/third_party/blink/renderer/core/html/forms/radio_input_type.cc
@@ -151,8 +151,6 @@
        event.key() == "Enter")) {
     DispatchSimulatedClickIfActive(event);
   }
-
-  DispatchSimulatedClickIfActive(event);
 }
 
 bool RadioInputType::IsKeyboardFocusable() const {
diff --git a/third_party/blink/renderer/core/html/html_element.cc b/third_party/blink/renderer/core/html/html_element.cc
index a68349f6..f85f8a2 100644
--- a/third_party/blink/renderer/core/html/html_element.cc
+++ b/third_party/blink/renderer/core/html/html_element.cc
@@ -1333,6 +1333,8 @@
 void HTMLElement::HandleKeypressEvent(KeyboardEvent& event) {
   if (!IsSpatialNavigationEnabled(GetDocument().GetFrame()) || !SupportsFocus())
     return;
+  if (RuntimeEnabledFeatures::FocuslessSpatialNavigationEnabled())
+    return;
   GetDocument().UpdateStyleAndLayoutTree();
   // if the element is a text form control (like <input type=text> or
   // <textarea>) or has contentEditable attribute on, we should enter a space or
diff --git a/third_party/blink/renderer/core/input/keyboard_event_manager.cc b/third_party/blink/renderer/core/input/keyboard_event_manager.cc
index 06c313c3..100e7a4d 100644
--- a/third_party/blink/renderer/core/input/keyboard_event_manager.cc
+++ b/third_party/blink/renderer/core/input/keyboard_event_manager.cc
@@ -352,16 +352,19 @@
     frame_->GetEditor().HandleKeyboardEvent(event);
     if (event->DefaultHandled())
       return;
-    if (event->charCode() == ' ')
-      DefaultSpaceEventHandler(event, possible_focused_node);
-  } else if (event->type() == event_type_names::kKeyup) {
     if (event->key() == "Enter") {
       DefaultEnterEventHandler(event);
-      return;
+    } else if (event->charCode() == ' ') {
+      DefaultSpaceEventHandler(event, possible_focused_node);
     }
-
-    if (event->keyCode() == kVKeySpatNavBack)
+  } else if (event->type() == event_type_names::kKeyup) {
+    if (event->DefaultHandled())
+      return;
+    if (event->key() == "Enter") {
+      DefaultEnterEventHandler(event);
+    } else if (event->keyCode() == kVKeySpatNavBack) {
       DefaultSpatNavBackEventHandler(event);
+    }
   }
 }
 
diff --git a/third_party/blink/renderer/core/page/spatial_navigation_controller.cc b/third_party/blink/renderer/core/page/spatial_navigation_controller.cc
index 7a2921c..4990b457 100644
--- a/third_party/blink/renderer/core/page/spatial_navigation_controller.cc
+++ b/third_party/blink/renderer/core/page/spatial_navigation_controller.cc
@@ -163,10 +163,18 @@
     return false;
 
   if (event->type() == event_type_names::kKeydown) {
+    enter_key_down_seen_ = true;
     interest_element->SetActive(true);
+  } else if (event->type() == event_type_names::kKeypress) {
+    enter_key_press_seen_ = true;
   } else if (event->type() == event_type_names::kKeyup) {
     interest_element->SetActive(false);
-    if (RuntimeEnabledFeatures::FocuslessSpatialNavigationEnabled()) {
+
+    // Ensure that the enter key has not already been handled by something else,
+    // or we can end up clicking elements multiple times. Some elements already
+    // convert the Enter key into click on down and press (and up) events.
+    if (RuntimeEnabledFeatures::FocuslessSpatialNavigationEnabled() &&
+        enter_key_down_seen_ && enter_key_press_seen_) {
       interest_element->focus(FocusParams(SelectionBehaviorOnFocus::kReset,
                                           kWebFocusTypeSpatialNavigation,
                                           nullptr));
@@ -179,6 +187,11 @@
   return true;
 }
 
+void SpatialNavigationController::ResetEnterKeyState() {
+  enter_key_down_seen_ = false;
+  enter_key_press_seen_ = false;
+}
+
 bool SpatialNavigationController::HandleImeSubmitKeyboardEvent(
     KeyboardEvent* event) {
   DCHECK(page_->GetSettings().GetSpatialNavigationEnabled());
diff --git a/third_party/blink/renderer/core/page/spatial_navigation_controller.h b/third_party/blink/renderer/core/page/spatial_navigation_controller.h
index faee8c69..98b709d 100644
--- a/third_party/blink/renderer/core/page/spatial_navigation_controller.h
+++ b/third_party/blink/renderer/core/page/spatial_navigation_controller.h
@@ -32,6 +32,10 @@
   bool HandleEscapeKeyboardEvent(KeyboardEvent* event);
   bool HandleImeSubmitKeyboardEvent(KeyboardEvent* event);
 
+  // Called when the enter key is released to clear local state because we don't
+  // get a consistent event stream when the Enter key is partially handled.
+  void ResetEnterKeyState();
+
   // Returns the element that's currently interested. i.e. the Element that's
   // currently indicated to the user.
   Element* GetInterestedElement() const;
@@ -107,6 +111,11 @@
   WeakMember<Element> interest_element_;
   Member<Page> page_;
 
+  // We need to track whether the enter key has been handled in down or press to
+  // know whether to generate a click on the up.
+  bool enter_key_down_seen_ = false;
+  bool enter_key_press_seen_ = false;
+
   mojom::blink::SpatialNavigationStatePtr spatial_navigation_state_;
   mojom::blink::SpatialNavigationHostPtr spatial_navigation_host_;
 };
diff --git a/third_party/blink/web_tests/virtual/focusless-spat-nav/fast/spatial-navigation/focusless/snav-focusless-click-once.html b/third_party/blink/web_tests/virtual/focusless-spat-nav/fast/spatial-navigation/focusless/snav-focusless-click-once.html
new file mode 100644
index 0000000..7e01c704
--- /dev/null
+++ b/third_party/blink/web_tests/virtual/focusless-spat-nav/fast/spatial-navigation/focusless/snav-focusless-click-once.html
@@ -0,0 +1,63 @@
+<!DOCTYPE html>
+<script src="../../../../../resources/testharness.js"></script>
+<script src="../../../../../resources/testharnessreport.js"></script>
+<script src="file:///gen/layout_test_data/mojo/public/js/mojo_bindings.js"></script>
+<script src="file:///gen/third_party/blink/public/mojom/page/spatial_navigation.mojom.js"></script>
+<script src="../../../../../fast/spatial-navigation/resources/mock-snav-service.js"></script>
+<script src="../../../../../fast/spatial-navigation/resources/snav-testharness.js"></script>
+
+<button type="button" id="button" autofocus> button </button>
+
+<svg height="100" width="100" id="svg" tabindex="0">
+  <circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" />
+</svg>
+
+<img src="" height="100" width="100" id="img" tabindex="1" />
+
+<script>
+  let button = document.getElementById("button");
+  let svg = document.getElementById("svg");
+  let img = document.getElementById("img");
+  let buttonClicked = 0;
+  let svgClicked = 0;
+  let imgClicked = 0;
+
+  button.addEventListener('click', function() {
+    console.log('wtf');
+    buttonClicked++;
+  })
+
+  svg.addEventListener('click', function() {
+    svgClicked++;
+  })
+
+  img.addEventListener('keydown', function(e) {
+    e.preventDefault();
+  })
+  img.addEventListener('click', function() {
+    imgClicked++;
+  })
+
+  promise_test(async () => {
+    await snav.rAF();
+    eventSender.keyDown('Enter');
+    await snav.rAF();
+    assert_equals(buttonClicked,
+                  1,
+                  "button should have been clicked once.");
+
+    svg.focus();
+    eventSender.keyDown('Enter');
+    await snav.rAF();
+    assert_equals(svgClicked,
+                  1,
+                  "svg should have been clicked once.");
+
+    img.focus();
+    eventSender.keyDown('Enter');
+    await snav.rAF();
+    assert_equals(imgClicked,
+                  0,
+                  "img should not have been clicked.");
+  }, 'Spat Nav doesn\'t click multiple times.');
+</script>