Bug 872397: Work around inconsistencies in layout word movement to ensure that a11y word start boundaries are consistent around punctuation and white space. r=MarcoZ

Differential Revision: https://phabricator.services.mozilla.com/D90654

accessible/generic/HyperTextAccessible.cpp

@@ -38,6 +38,7 @@
 #include "mozilla/HTMLEditor.h"
 #include "mozilla/MathAlgorithms.h"
 #include "mozilla/PresShell.h"
+#include "mozilla/StaticPrefs_layout.h"
 #include "mozilla/TextEditor.h"
 #include "mozilla/dom/Element.h"
 #include "mozilla/dom/HTMLBRElement.h"
@@ -501,6 +502,101 @@ uint32_t HyperTextAccessible::FindOffset(uint32_t aOffset,
   return hyperTextOffset;
 }
 
+uint32_t HyperTextAccessible::FindWordBoundary(
+    uint32_t aOffset, nsDirection aDirection,
+    EWordMovementType aWordMovementType) {
+  uint32_t orig =
+      FindOffset(aOffset, aDirection, eSelectWord, aWordMovementType);
+  if (aWordMovementType != eStartWord) {
+    return orig;
+  }
+  if (aDirection == eDirPrevious) {
+    // When layout.word_select.stop_at_punctuation is true (the default),
+    // for a word beginning with punctuation, layout treats the punctuation
+    // as the start of the word when moving next. However, when moving
+    // previous, layout stops *after* the punctuation. We want to be
+    // consistent regardless of movement direction and always treat punctuation
+    // as the start of a word.
+    if (!StaticPrefs::layout_word_select_stop_at_punctuation()) {
+      return orig;
+    }
+    // Case 1: Example: "a @"
+    // If aOffset is 2 or 3, orig will be 0, but it should be 2. That is,
+    // previous word moved back too far.
+    Accessible* child = GetChildAtOffset(orig);
+    if (child && child->IsHyperText()) {
+      // For a multi-word embedded object, previous word correctly goes back
+      // to the start of the word (the embedded object). Next word (below)
+      // incorrectly stops after the embedded object in this case, so return
+      // the already correct result.
+      // Example: "a x y b", where "x y" is an embedded link
+      // If aOffset is 4, orig will be 2, which is correct.
+      // If we get the next word (below), we'll end up returning 3 instead.
+      return orig;
+    }
+    uint32_t next = FindOffset(orig, eDirNext, eSelectWord, eStartWord);
+    if (next < aOffset) {
+      // Next word stopped on punctuation.
+      return next;
+    }
+    // case 2: example: "a @@b"
+    // If aOffset is 2, 3 or 4, orig will be 4, but it should be 2. That is,
+    // previous word didn't go back far enough.
+    if (orig == 0) {
+      return orig;
+    }
+    // Walk backwards by offset, getting the next word.
+    // In the loop, o is unsigned, so o >= 0 will always be true and won't
+    // prevent us from decrementing at 0. Instead, we check that o doesn't
+    // wrap around.
+    for (uint32_t o = orig - 1; o < orig; --o) {
+      next = FindOffset(o, eDirNext, eSelectWord, eStartWord);
+      if (next == orig) {
+        // Next word and previous word were consistent. This
+        // punctuation problem isn't applicable here.
+        break;
+      }
+      if (next < orig) {
+        // Next word stopped on punctuation.
+        return next;
+      }
+    }
+  } else {
+    // When layout.word_select.stop_at_punctuation is true (the default),
+    // when positioned on punctuation in the middle of a word, next word skips
+    // the rest of the word. However, when positioned before the punctuation,
+    // next word moves just after the punctuation. We want to be consistent
+    // regardless of starting position and always stop just after the
+    // punctuation.
+    // Next word can move too far when positioned on white space too.
+    // Example: "a b@c"
+    // If aOffset is 3, orig will be 5, but it should be 4. That is, next word
+    // moved too far.
+    if (aOffset == 0) {
+      return orig;
+    }
+    uint32_t prev = FindOffset(orig, eDirPrevious, eSelectWord, eStartWord);
+    if (prev <= aOffset) {
+      // orig definitely isn't too far forward.
+      return orig;
+    }
+    // Walk backwards by offset, getting the next word.
+    // In the loop, o is unsigned, so o >= 0 will always be true and won't
+    // prevent us from decrementing at 0. Instead, we check that o doesn't
+    // wrap around.
+    for (uint32_t o = aOffset - 1; o < aOffset; --o) {
+      uint32_t next = FindOffset(o, eDirNext, eSelectWord, eStartWord);
+      if (next > aOffset && next < orig) {
+        return next;
+      }
+      if (next <= aOffset) {
+        break;
+      }
+    }
+  }
+  return orig;
+}
+
 uint32_t HyperTextAccessible::FindLineBoundary(
     uint32_t aOffset, EWhichLineBoundary aWhichLineBoundary) {
   // Note: empty last line doesn't have own frame (a previous line contains '\n'

accessible/generic/HyperTextAccessible.h

@@ -445,9 +445,7 @@ class HyperTextAccessible : public AccessibleWrap {
    * Return an offset of the found word boundary.
    */
   uint32_t FindWordBoundary(uint32_t aOffset, nsDirection aDirection,
-                            EWordMovementType aWordMovementType) {
-    return FindOffset(aOffset, aDirection, eSelectWord, aWordMovementType);
-  }
+                            EWordMovementType aWordMovementType);
 
   /**
    * Used to get begin/end of previous/this/next line. Note: end of line

accessible/tests/mochitest/text/test_wordboundary.html

@@ -208,11 +208,7 @@
                            ] );
 
       testTextAtOffset(ids, BOUNDARY_WORD_START,
-                       [ [ 0, 8, "oneword\n\n", 0, 9,
-                           [ [ 7, "ml_divbr1", kTodo, kTodo, kTodo ],
-                             [ 7, "ml_edivbr1", kTodo, kTodo, kTodo ],
-                             [ 8, "ml_divbr1", kTodo, kTodo, kTodo ],
-                             [ 8, "ml_edivbr1", kTodo, kTodo, kTodo ] ] ],
+                       [ [ 0, 8, "oneword\n\n", 0, 9 ],
                          [ 9, 12, "two ", 9, 13 ],
                          [ 13, 19, "words\n", 13, 19 ] ]);
       testTextAtOffset(ids, BOUNDARY_WORD_END,
@@ -226,11 +222,7 @@
                              [ 19, "ml_edivbr1", kTodo, kTodo, kOk ] ] ] ]);
 
       testTextAfterOffset(ids, BOUNDARY_WORD_START,
-                           [ [ 0, 8, "two ", 9, 13,
-                               [ [ 7, "ml_divbr1", kTodo, kTodo, kTodo ],
-                                 [ 7, "ml_edivbr1", kTodo, kTodo, kTodo ],
-                                 [ 8, "ml_divbr1", kTodo, kTodo, kTodo ],
-                                 [ 8, "ml_edivbr1", kTodo, kTodo, kTodo ] ] ],
+                           [ [ 0, 8, "two ", 9, 13 ],
                              [ 9, 12, "words\n", 13, 19 ],
                              [ 13, 19, "", 19, 19 ] ]);
       testTextAfterOffset(ids, BOUNDARY_WORD_END,
@@ -254,6 +246,45 @@
                            [ [ 0, 1, kEmbedChar, 2, 3 ],
                              [ 2, 3, "", 3, 3 ] ]);
 
+      // Punctuation tests.
+      testTextAtOffset("punc_alone", BOUNDARY_WORD_START, [
+        [ 0, 1, "a ", 0, 2 ],
+        [ 2, 4, "@@ ", 2, 5 ],
+        [ 5, 6, "b", 5, 6 ]
+      ]);
+      testTextAtOffset("punc_begin", BOUNDARY_WORD_START, [
+        [ 0, 1, "a ", 0, 2 ],
+        [ 2, 5, "@@b ", 2, 6 ],
+        [ 6, 7, "c", 6, 7 ]
+      ]);
+      testTextAtOffset("punc_end", BOUNDARY_WORD_START, [
+        [ 0, 1, "a ", 0, 2 ],
+        [ 2, 5, "b@@ ", 2, 6 ],
+        [ 6, 7, "c", 6, 7 ]
+      ]);
+      testTextAtOffset("punc_middle", BOUNDARY_WORD_START, [
+        [ 0, 1, "a ", 0, 2 ],
+        [ 2, 4, "b@@", 2, 5 ],
+        [ 5, 6, "c ", 5, 7 ],
+        [ 7, 8, "d", 7, 8 ]
+      ]);
+      testTextAtOffset("punc_everywhere", BOUNDARY_WORD_START, [
+        [ 0, 1, "a ", 0, 2 ],
+        [ 2, 6, "@@b@@", 2, 7 ],
+        [ 7, 10, "c@@ ", 7, 11 ],
+        [ 11, 12, "d", 11, 12 ]
+      ]);
+
+      // Multi-word embedded object test.
+      testTextAtOffset("multiword_embed", BOUNDARY_WORD_START, [
+        [ 0, 1, "a ", 0, 2 ],
+        [ 2, 3, `${kEmbedChar} `, 2, 4, [
+          // Word at offset 2 returns end offset 3, should be 4.
+          [ 2, "multiword_embed", kTodo, kOk, kTodo ]
+        ] ],
+        [ 4, 5, "b", 4, 5 ]
+      ]);
+
       SimpleTest.finish();
     }
 
@@ -318,5 +349,13 @@ two words
   </pre>
 
   <div id="cntr_1">a <a href="#">b</a></div>
+
+  <p id="punc_alone">a @@ b</p>
+  <p id="punc_begin">a @@b c</p>
+  <p id="punc_end">a b@@ c</p>
+  <p id="punc_middle">a b@@c d</p>
+  <p id="punc_everywhere">a @@b@@c@@ d</p>
+
+  <p id="multiword_embed">a <a href="#">x y</a> b</p>
 </body>
 </html>