Reland "If the caret is at the boundary of a misspelling, getting > > If the...

Reland "If the caret is at the boundary of a misspelling, getting > > If the caret is at the boundary of a misspelling, getting suggestions via context menu should work

If a screen reader user tries to get spelling suggestions for a misspelled word, they usually first move to the beginning of the word using Ctrl+Left/Right and then invoke the context menu.
On Windows, Ctrl+Left/Right always moves to the start of the word. On other platforms, Ctrl or Cmd + Left moves to the start whilst Ctrl / Cmd + Right moves to the end of the word.
When any of the above keystrokes are pressed, the user hears the whole word.
It is not reasonable to expect the user to first press Ctrl+Left/Right to navigate through the line until they find the spelling mistake and then have to additionally press cursor right, (or cursor left when using Cmd-Right on Mac), so that the caret is within the word before invoking the context menu.
The result is that many screen reader users might have the mistaken believe that our spell checker is broken.
Tested: Manually, unit tests, layout tests
Bug: 790828

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I847419f299e1a56c4e32a7d00368f663f66072ff
Reviewed-on: https://chromium-review.googlesource.com/c/1252247Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597166}

third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html

@@ -97,31 +97,18 @@ spellcheck_test(
       title: 'Context click at boundary of misspelled word.',
       callback: sample => test(() => {
         const container = sample.document.querySelector('div');
-        const shouldSelect = isMac(navigator.platform);
-        if (shouldSelect) {
-          // On Mac, right-clicking immediately before a word will select it.
-          assertContextClickSelection(
+        assertContextClickSelection(
             container, 0, 'wellcome',
             'Context clicking immediately before "wellcome" selects it');
 
-          // De-select "wellcome"; otherwise, context-clicking immediately after "wellcome" will
-          // not change the selection (since it would select a space and not another word)
-          container.ownerDocument.getSelection().removeAllRanges();
-
-          // On Mac, right-clicking immediately after a word will select the space after it.
-          assertContextClickSelection(
-            container, 8, ' ',
-            'Context clicking immediately after "wellcome" selects the space after it');
-          return;
-        }
-
-        assertContextClickSelection(
-            container, 0, '',
-            'Context clicking immediately before "wellcome" does not select it');
+        // De-select "wellcome" to ensure that context-clicking immediately after
+        // "wellcome" will select it again.
+        container.ownerDocument.getSelection().removeAllRanges();
 
+        // Right-clicking immediately after a word will select the whole word.
         assertContextClickSelection(
-            container, 8, '',
-            'Context clicking immediately after "wellcome" does not select it');
+            container, 8, 'wellcome',
+            'Context clicking immediately after "wellcome" selects it');
       },
       'Context clicking immediately before or after misspelled word in editable DIV does not select it.',
       {sample: sample, blockHeldTest: true})

third_party/blink/renderer/core/editing/ephemeral_range.h

@@ -107,7 +107,7 @@ class EphemeralRangeTemplate final {
 
   Node* CommonAncestorContainer() const;
 
-  // Returns true if |m_startPositoin| == |m_endPosition| or |isNull()|.
+  // Returns true if |m_startPosition| == |m_endPosition| or |isNull()|.
   bool IsCollapsed() const;
   bool IsNull() const {
     DCHECK(IsValid());

third_party/blink/renderer/core/editing/markers/document_marker_controller.cc

@@ -48,6 +48,8 @@
 #include "third_party/blink/renderer/core/editing/markers/suggestion_marker_list_impl.h"
 #include "third_party/blink/renderer/core/editing/markers/text_match_marker.h"
 #include "third_party/blink/renderer/core/editing/markers/text_match_marker_list_impl.h"
+#include "third_party/blink/renderer/core/editing/position.h"
+#include "third_party/blink/renderer/core/editing/visible_position.h"
 #include "third_party/blink/renderer/core/editing/visible_units.h"
 #include "third_party/blink/renderer/core/frame/local_frame_view.h"
 #include "third_party/blink/renderer/core/layout/layout_object.h"
@@ -389,6 +391,79 @@ void DocumentMarkerController::RemoveMarkersInternal(
   InvalidatePaintForNode(text);
 }
 
+DocumentMarker* DocumentMarkerController::FirstMarkerAroundPosition(
+    const PositionInFlatTree& position,
+    DocumentMarker::MarkerTypes types) {
+  if (position.IsNull())
+    return nullptr;
+
+  const PositionInFlatTree start_of_word_or_null =
+      StartOfWord(CreateVisiblePosition(position), kPreviousWordIfOnBoundary)
+          .DeepEquivalent();
+  const PositionInFlatTree start =
+      start_of_word_or_null.IsNotNull() ? start_of_word_or_null : position;
+  const PositionInFlatTree end_of_word_or_null =
+      EndOfWord(CreateVisiblePosition(position), kNextWordIfOnBoundary)
+          .DeepEquivalent();
+  const PositionInFlatTree end =
+      end_of_word_or_null.IsNotNull() ? end_of_word_or_null : position;
+
+  DCHECK_LE(start, end) << "|start| should be before |end|.";
+
+  const Node* const start_node = start.ComputeContainerNode();
+  const unsigned start_offset = start.ComputeOffsetInContainerNode();
+  const Node* const end_node = end.ComputeContainerNode();
+  const unsigned end_offset = end.ComputeOffsetInContainerNode();
+
+  for (const Node& node : EphemeralRangeInFlatTree(start, end).Nodes()) {
+    if (!node.IsTextNode())
+      continue;
+
+    const unsigned start_range_offset = node == start_node ? start_offset : 0;
+    const unsigned end_range_offset =
+        node == end_node ? end_offset : ToText(node).length();
+
+    DocumentMarker* const found_marker = FirstMarkerIntersectingOffsetRange(
+        ToText(node), start_range_offset, end_range_offset, types);
+    if (found_marker)
+      return found_marker;
+  }
+
+  return nullptr;
+}
+
+DocumentMarker* DocumentMarkerController::FirstMarkerIntersectingEphemeralRange(
+    const EphemeralRange& range,
+    DocumentMarker::MarkerTypes types) {
+  if (range.IsNull())
+    return nullptr;
+
+  if (range.IsCollapsed()) {
+    return FirstMarkerAroundPosition(
+        ToPositionInFlatTree(range.StartPosition()), types);
+  }
+
+  const Node* const start_container =
+      range.StartPosition().ComputeContainerNode();
+  const Node* const end_container = range.EndPosition().ComputeContainerNode();
+
+  // We don't currently support the case where a marker spans multiple nodes.
+  // See crbug.com/720065
+  if (start_container != end_container)
+    return nullptr;
+
+  if (!start_container->IsTextNode())
+    return nullptr;
+
+  const unsigned start_offset =
+      range.StartPosition().ComputeOffsetInContainerNode();
+  const unsigned end_offset =
+      range.EndPosition().ComputeOffsetInContainerNode();
+
+  return FirstMarkerIntersectingOffsetRange(ToText(*start_container),
+                                            start_offset, end_offset, types);
+}
+
 DocumentMarker* DocumentMarkerController::FirstMarkerIntersectingOffsetRange(
     const Text& node,
     unsigned start_offset,

third_party/blink/renderer/core/editing/markers/document_marker_controller.h

@@ -29,9 +29,12 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_EDITING_MARKERS_DOCUMENT_MARKER_CONTROLLER_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_EDITING_MARKERS_DOCUMENT_MARKER_CONTROLLER_H_
 
+#include <utility>
+
 #include "base/macros.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/dom/synchronous_mutation_observer.h"
+#include "third_party/blink/renderer/core/editing/forward.h"
 #include "third_party/blink/renderer/core/editing/iterators/text_iterator.h"
 #include "third_party/blink/renderer/core/editing/markers/composition_marker.h"
 #include "third_party/blink/renderer/core/editing/markers/document_marker.h"
@@ -97,6 +100,24 @@ class CORE_EXPORT DocumentMarkerController final
   // TODO(rlanday): can these methods for retrieving markers be consolidated
   // without hurting efficiency?
 
+  // If the given position is either at the boundary or inside a word, expands
+  // the position to the surrounding word and then looks for a marker having the
+  // specified type. If the position is neither at the boundary or inside a
+  // word, expands the position to cover the space between the end of the
+  // previous and the start of the next words. If such a marker exists, this
+  // method will return one of them (no guarantees are provided as to which
+  // one). Otherwise, this method will return null.
+  DocumentMarker* FirstMarkerAroundPosition(const PositionInFlatTree&,
+                                            DocumentMarker::MarkerTypes);
+  // Looks for a marker in the specified EphemeralRange of the specified type
+  // whose interior has non-empty overlap with the bounds of the range.
+  // If the range is collapsed, it uses FirstMarkerAroundPosition to expand the
+  // range to the surrounding word.
+  // If such a marker exists, this method will return one of them (no guarantees
+  // are provided as to which one). Otherwise, this method will return null.
+  DocumentMarker* FirstMarkerIntersectingEphemeralRange(
+      const EphemeralRange&,
+      DocumentMarker::MarkerTypes);
   // Looks for a marker in the specified node of the specified type whose
   // interior has non-empty overlap with the range [start_offset, end_offset].
   // If the range is collapsed, this looks for a marker containing the offset of

third_party/blink/renderer/core/editing/selection_controller.cc

@@ -131,15 +131,9 @@ VisiblePositionInFlatTree VisiblePositionOfHitTestResult(
 
 DocumentMarker* SpellCheckMarkerAtPosition(
     DocumentMarkerController& document_marker_controller,
-    const Position& position) {
-  const Node* const node = position.ComputeContainerNode();
-  if (!node->IsTextNode())
-    return nullptr;
-
-  const unsigned offset = position.ComputeOffsetInContainerNode();
-  return document_marker_controller.FirstMarkerIntersectingOffsetRange(
-      *ToText(node), offset, offset,
-      DocumentMarker::MarkerTypes::Misspelling());
+    const PositionInFlatTree& position) {
+  return document_marker_controller.FirstMarkerAroundPosition(
+      position, DocumentMarker::MarkerTypes::Misspelling());
 }
 
 }  // namespace
@@ -674,9 +668,8 @@ void SelectionController::SelectClosestMisspellingFromHitTestResult(
 
   const PositionInFlatTree& marker_position =
       pos.DeepEquivalent().ParentAnchoredEquivalent();
-  const DocumentMarker* const marker =
-      SpellCheckMarkerAtPosition(inner_node->GetDocument().Markers(),
-                                 ToPositionInDOMTree(marker_position));
+  const DocumentMarker* const marker = SpellCheckMarkerAtPosition(
+      inner_node->GetDocument().Markers(), marker_position);
   if (!marker) {
     UpdateSelectionForMouseDownDispatchingSelectStart(
         inner_node, SelectionInFlatTree(),
@@ -1168,8 +1161,8 @@ static bool HitTestResultIsMisspelled(const HitTestResult& result) {
       inner_node->GetLayoutObject()->PositionForPoint(result.LocalPoint()));
   if (pos.IsNull())
     return false;
-  const Position& marker_position =
-      pos.DeepEquivalent().ParentAnchoredEquivalent();
+  const PositionInFlatTree& marker_position =
+      ToPositionInFlatTree(pos.DeepEquivalent().ParentAnchoredEquivalent());
   return SpellCheckMarkerAtPosition(inner_node->GetDocument().Markers(),
                                     marker_position);
 }

third_party/blink/renderer/core/editing/spellcheck/spell_checker.cc

@@ -434,32 +434,14 @@ SpellChecker::GetSpellCheckMarkerUnderSelection() const {
   // Caret and range selections always return valid normalized ranges.
   const EphemeralRange& selection_range = FirstEphemeralRangeOf(selection);
 
-  Node* const selection_start_container =
-      selection_range.StartPosition().ComputeContainerNode();
-  Node* const selection_end_container =
-      selection_range.EndPosition().ComputeContainerNode();
-
-  // We don't currently support the case where a misspelling spans multiple
-  // nodes. See crbug.com/720065
-  if (selection_start_container != selection_end_container)
-    return {};
-
-  if (!selection_start_container->IsTextNode())
-    return {};
-
-  const unsigned selection_start_offset =
-      selection_range.StartPosition().ComputeOffsetInContainerNode();
-  const unsigned selection_end_offset =
-      selection_range.EndPosition().ComputeOffsetInContainerNode();
-
   DocumentMarker* const marker =
-      GetFrame().GetDocument()->Markers().FirstMarkerIntersectingOffsetRange(
-          ToText(*selection_start_container), selection_start_offset,
-          selection_end_offset, DocumentMarker::MarkerTypes::Misspelling());
+      GetFrame().GetDocument()->Markers().FirstMarkerIntersectingEphemeralRange(
+          selection_range, DocumentMarker::MarkerTypes::Misspelling());
   if (!marker)
     return {};
 
-  return std::make_pair(selection_start_container, ToSpellCheckMarker(marker));
+  return std::make_pair(selection_range.StartPosition().ComputeContainerNode(),
+                        ToSpellCheckMarker(marker));
 }
 
 std::pair<String, String> SpellChecker::SelectMisspellingAsync() {

third_party/blink/renderer/core/editing/spellcheck/spell_checker_test.cc

@@ -149,7 +149,10 @@ TEST_F(SpellCheckerTest, GetSpellCheckMarkerUnderSelection_FirstCharSelected) {
           .GetFrame()
           ->GetSpellChecker()
           .GetSpellCheckMarkerUnderSelection();
-  EXPECT_NE(nullptr, result.first);
+  EXPECT_EQ(text, result.first);
+  ASSERT_NE(nullptr, result.second);
+  EXPECT_EQ(0u, result.second->StartOffset());
+  EXPECT_EQ(8u, result.second->EndOffset());
 }
 
 TEST_F(SpellCheckerTest, GetSpellCheckMarkerUnderSelection_LastCharSelected) {
@@ -173,7 +176,10 @@ TEST_F(SpellCheckerTest, GetSpellCheckMarkerUnderSelection_LastCharSelected) {
           .GetFrame()
           ->GetSpellChecker()
           .GetSpellCheckMarkerUnderSelection();
-  EXPECT_NE(nullptr, result.first);
+  EXPECT_EQ(text, result.first);
+  ASSERT_NE(nullptr, result.second);
+  EXPECT_EQ(0u, result.second->StartOffset());
+  EXPECT_EQ(8u, result.second->EndOffset());
 }
 
 TEST_F(SpellCheckerTest,
@@ -198,7 +204,10 @@ TEST_F(SpellCheckerTest,
           .GetFrame()
           ->GetSpellChecker()
           .GetSpellCheckMarkerUnderSelection();
-  EXPECT_NE(nullptr, result.first);
+  EXPECT_EQ(text, result.first);
+  ASSERT_NE(nullptr, result.second);
+  EXPECT_EQ(0u, result.second->StartOffset());
+  EXPECT_EQ(1u, result.second->EndOffset());
 }
 
 TEST_F(SpellCheckerTest,
@@ -223,7 +232,10 @@ TEST_F(SpellCheckerTest,
           .GetFrame()
           ->GetSpellChecker()
           .GetSpellCheckMarkerUnderSelection();
-  EXPECT_EQ(nullptr, result.first);
+  EXPECT_EQ(text, result.first);
+  ASSERT_NE(nullptr, result.second);
+  EXPECT_EQ(0u, result.second->StartOffset());
+  EXPECT_EQ(1u, result.second->EndOffset());
 }
 
 TEST_F(SpellCheckerTest,
@@ -248,7 +260,10 @@ TEST_F(SpellCheckerTest,
           .GetFrame()
           ->GetSpellChecker()
           .GetSpellCheckMarkerUnderSelection();
-  EXPECT_EQ(nullptr, result.first);
+  EXPECT_EQ(text, result.first);
+  ASSERT_NE(nullptr, result.second);
+  EXPECT_EQ(0u, result.second->StartOffset());
+  EXPECT_EQ(1u, result.second->EndOffset());
 }
 
 TEST_F(SpellCheckerTest,
@@ -273,7 +288,10 @@ TEST_F(SpellCheckerTest,
           .GetFrame()
           ->GetSpellChecker()
           .GetSpellCheckMarkerUnderSelection();
-  EXPECT_EQ(nullptr, result.first);
+  EXPECT_EQ(text, result.first);
+  ASSERT_NE(nullptr, result.second);
+  EXPECT_EQ(0u, result.second->StartOffset());
+  EXPECT_EQ(8u, result.second->EndOffset());
 }
 
 TEST_F(SpellCheckerTest,
@@ -298,7 +316,10 @@ TEST_F(SpellCheckerTest,
           .GetFrame()
           ->GetSpellChecker()
           .GetSpellCheckMarkerUnderSelection();
-  EXPECT_EQ(nullptr, result.first);
+  EXPECT_EQ(text, result.first);
+  ASSERT_NE(nullptr, result.second);
+  EXPECT_EQ(0u, result.second->StartOffset());
+  EXPECT_EQ(8u, result.second->EndOffset());
 }
 
 TEST_F(SpellCheckerTest, GetSpellCheckMarkerUnderSelection_CaretMiddleOfWord) {
@@ -322,7 +343,10 @@ TEST_F(SpellCheckerTest, GetSpellCheckMarkerUnderSelection_CaretMiddleOfWord) {
           .GetFrame()
           ->GetSpellChecker()
           .GetSpellCheckMarkerUnderSelection();
-  EXPECT_NE(nullptr, result.first);
+  EXPECT_EQ(text, result.first);
+  ASSERT_NE(nullptr, result.second);
+  EXPECT_EQ(0u, result.second->StartOffset());
+  EXPECT_EQ(8u, result.second->EndOffset());
 }
 
 TEST_F(SpellCheckerTest,