Addressing text selection mode on Android and AccessibilityEvent discrepancies

This CL addresses various coupled bugs in both text selection and non-
default granularity navigation for input fields/contenteditables on
Android. It fixes various issues with improper announcements,
announcements at the wrong time, and text selection not functioning in
general. It also addresses AccessibilityEvent discrepancies under-the-
hood in cases where we appeared to have the correct functionality to
the user, but only by happenstance as we were passing bad data to
TalkBack.

This CL addresses:
Bug 1041649 - Text selection on Android broken/not functioning
Bug 1017466 - Contenteditable word granularity navigation not reading
correctly
Bug 1041587 - Beginning/End of field announcements being read at
wrong index

Bug: 1041649,1017466,1041587
Change-Id: I30a8888e99e90356398d14319fbdf225839f5dac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1998829
Commit-Queue: Mark Schillaci <mschillaci@google.com>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734171}

content/browser/accessibility/android_granularity_movement_browsertest.cc

@@ -85,8 +85,7 @@ class AndroidGranularityMovementBrowserTest : public ContentBrowserTest {
     int previous_end_index = -1;
     while (manager->NextAtGranularity(granularity, end_index, android_node,
                                       &start_index, &end_index)) {
-      int len =
-          (granularity == GRANULARITY_CHARACTER) ? 1 : end_index - start_index;
+      int len = end_index - start_index;
       base::string16 selection = text.substr(start_index, len);
       if (base::EndsWith(selection, base::ASCIIToUTF16("\n"),
                          base::CompareCase::INSENSITIVE_ASCII))
@@ -111,8 +110,7 @@ class AndroidGranularityMovementBrowserTest : public ContentBrowserTest {
     start_index = end_index;
     while (manager->PreviousAtGranularity(
         granularity, start_index, android_node, &start_index, &end_index)) {
-      int len =
-          (granularity == GRANULARITY_CHARACTER) ? 1 : end_index - start_index;
+      int len = end_index - start_index;
       base::string16 selection = text.substr(start_index, len);
       if (base::EndsWith(selection, base::ASCIIToUTF16("\n"),
                          base::CompareCase::INSENSITIVE_ASCII))

content/browser/accessibility/browser_accessibility_manager_android.cc

@@ -287,8 +287,9 @@ bool BrowserAccessibilityManagerAndroid::NextAtGranularity(
       base::i18n::UTF16CharIterator iter(text.data(), text.size());
       while (!iter.end() && iter.array_pos() <= cursor_index)
         iter.Advance();
-      *start_index = iter.array_pos();
       *end_index = iter.array_pos();
+      iter.Rewind();
+      *start_index = iter.array_pos();
       break;
     }
     case ANDROID_ACCESSIBILITY_NODE_INFO_MOVEMENT_GRANULARITY_WORD:
@@ -335,7 +336,7 @@ bool BrowserAccessibilityManagerAndroid::PreviousAtGranularity(
         iter.Advance();
       }
       *start_index = previous_index;
-      *end_index = previous_index;
+      *end_index = iter.array_pos();
       break;
     }
     case ANDROID_ACCESSIBILITY_NODE_INFO_MOVEMENT_GRANULARITY_WORD:

content/browser/accessibility/web_contents_accessibility_android.cc

@@ -1066,9 +1066,9 @@ jboolean WebContentsAccessibilityAndroid::NextAtGranularity(
   if (root_manager_->NextAtGranularity(granularity, cursor_index, node,
                                        &start_index, &end_index)) {
     base::string16 text = node->GetInnerText();
-    Java_WebContentsAccessibilityImpl_finishGranularityMove(
+    Java_WebContentsAccessibilityImpl_finishGranularityMoveNext(
         env, obj, base::android::ConvertUTF16ToJavaString(env, text),
-        extend_selection, start_index, end_index, true);
+        extend_selection, start_index, end_index);
     return true;
   }
   return false;
@@ -1132,10 +1132,10 @@ jboolean WebContentsAccessibilityAndroid::PreviousAtGranularity(
   int end_index = -1;
   if (root_manager_->PreviousAtGranularity(granularity, cursor_index, node,
                                            &start_index, &end_index)) {
-    Java_WebContentsAccessibilityImpl_finishGranularityMove(
+    Java_WebContentsAccessibilityImpl_finishGranularityMovePrevious(
         env, obj,
         base::android::ConvertUTF16ToJavaString(env, node->GetInnerText()),
-        extend_selection, start_index, end_index, false);
+        extend_selection, start_index, end_index);
     return true;
   }
   return false;

content/browser/accessibility/web_contents_accessibility_android.h

@@ -137,8 +137,8 @@ class CONTENT_EXPORT WebContentsAccessibilityAndroid
   // of our own selection in BrowserAccessibilityManager.java for static
   // text, but if this is an editable text node, updates the selected text
   // in Blink, too, and either way calls
-  // Java_BrowserAccessibilityManager_finishGranularityMove with the
-  // result.
+  // Java_BrowserAccessibilityManager_finishGranularityMove[NEXT/PREVIOUS]
+  // with the result.
   jboolean NextAtGranularity(JNIEnv* env,
                              const base::android::JavaParamRef<jobject>& obj,
                              jint granularity,

content/public/android/java/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityImpl.java

@@ -90,20 +90,25 @@ public class WebContentsAccessibilityImpl extends AccessibilityNodeProvider
     private boolean mIsHovering;
     private int mLastHoverId = View.NO_ID;
     protected int mCurrentRootId;
-    private int[] mTempLocation = new int[2];
     protected ViewGroup mView;
     private boolean mUserHasTouchExplored;
     private boolean mPendingScrollToMakeNodeVisible;
     private boolean mNotifyFrameInfoInitializedCalled;
     private boolean mAccessibilityEnabledForTesting;
     private int mSelectionGranularity;
-    private int mSelectionStartIndex;
-    private int mSelectionEndIndex;
     protected int mAccessibilityFocusId;
     protected int mSelectionNodeId;
     private Runnable mSendWindowContentChangedRunnable;
     private View mAutofillPopupView;
     private CaptioningController mCaptioningController;
+    private boolean mIsCurrentlyExtendingSelection;
+    private int mSelectionStart;
+    private int mCursorIndex;
+
+    // Whether or not the next selection event should be fired. We only want to sent one traverse
+    // and one selection event per granularity move, this ensures no double events while still
+    // sending events when the user is using other assistive technology (e.g. external keyboard)
+    private boolean mSuppressNextSelectionEvent;
 
     // Whether native accessibility is allowed.
     private boolean mNativeAccessibilityAllowed;
@@ -692,19 +697,20 @@ public class WebContentsAccessibilityImpl extends AccessibilityNodeProvider
 
     private void setGranularityAndUpdateSelection(int granularity) {
         mSelectionGranularity = granularity;
-        if (mSelectionGranularity == NO_GRANULARITY_SELECTED) {
-            mSelectionStartIndex = -1;
-            mSelectionEndIndex = -1;
-        }
+
         if (WebContentsAccessibilityImplJni.get().isEditableText(
                     mNativeObj, WebContentsAccessibilityImpl.this, mAccessibilityFocusId)
                 && WebContentsAccessibilityImplJni.get().isFocused(
                         mNativeObj, WebContentsAccessibilityImpl.this, mAccessibilityFocusId)) {
-            mSelectionStartIndex =
-                    WebContentsAccessibilityImplJni.get().getEditableTextSelectionStart(
-                            mNativeObj, WebContentsAccessibilityImpl.this, mAccessibilityFocusId);
-            mSelectionEndIndex = WebContentsAccessibilityImplJni.get().getEditableTextSelectionEnd(
-                    mNativeObj, WebContentsAccessibilityImpl.this, mAccessibilityFocusId);
+            // If selection/cursor are "unassigned" (e.g. first user swipe), then assign as needed
+            if (mSelectionStart == -1 || mCursorIndex == -1) {
+                mSelectionStart =
+                        WebContentsAccessibilityImplJni.get().getEditableTextSelectionStart(
+                                mNativeObj, WebContentsAccessibilityImpl.this,
+                                mAccessibilityFocusId);
+                mCursorIndex = WebContentsAccessibilityImplJni.get().getEditableTextSelectionEnd(
+                        mNativeObj, WebContentsAccessibilityImpl.this, mAccessibilityFocusId);
+            }
         }
     }
 
@@ -712,10 +718,10 @@ public class WebContentsAccessibilityImpl extends AccessibilityNodeProvider
         if (virtualViewId != mSelectionNodeId) return false;
         setGranularityAndUpdateSelection(granularity);
 
-        // This calls finishGranularityMove when it's done.
+        // This calls finishGranularityMoveNext when it's done.
         return WebContentsAccessibilityImplJni.get().nextAtGranularity(mNativeObj,
                 WebContentsAccessibilityImpl.this, mSelectionGranularity, extendSelection,
-                virtualViewId, mSelectionStartIndex);
+                virtualViewId, mCursorIndex);
     }
 
     private boolean previousAtGranularity(
@@ -723,19 +729,20 @@ public class WebContentsAccessibilityImpl extends AccessibilityNodeProvider
         if (virtualViewId != mSelectionNodeId) return false;
         setGranularityAndUpdateSelection(granularity);
 
-        // This calls finishGranularityMove when it's done.
+        // This calls finishGranularityMovePrevious when it's done.
         return WebContentsAccessibilityImplJni.get().previousAtGranularity(mNativeObj,
                 WebContentsAccessibilityImpl.this, mSelectionGranularity, extendSelection,
-                virtualViewId, mSelectionEndIndex);
+                virtualViewId, mCursorIndex);
     }
 
     @CalledByNative
-    private void finishGranularityMove(String text, boolean extendSelection, int itemStartIndex,
-            int itemEndIndex, boolean forwards) {
+    private void finishGranularityMoveNext(
+            String text, boolean extendSelection, int itemStartIndex, int itemEndIndex) {
         // Prepare to send both a selection and a traversal event in sequence.
         AccessibilityEvent selectionEvent = buildAccessibilityEvent(
                 mSelectionNodeId, AccessibilityEvent.TYPE_VIEW_TEXT_SELECTION_CHANGED);
         if (selectionEvent == null) return;
+
         AccessibilityEvent traverseEvent = buildAccessibilityEvent(mSelectionNodeId,
                 AccessibilityEvent.TYPE_VIEW_TEXT_TRAVERSED_AT_MOVEMENT_GRANULARITY);
         if (traverseEvent == null) {
@@ -743,55 +750,116 @@ public class WebContentsAccessibilityImpl extends AccessibilityNodeProvider
             return;
         }
 
-        // Update the cursor or selection based on the traversal. If it's an editable
-        // text node, set the real editing cursor too.
-        if (forwards) {
-            mSelectionEndIndex = itemEndIndex;
+        // Build selection event dependent on whether user is extending selection or not
+        if (extendSelection) {
+            // User started selecting, set the selection start point (only set once per selection)
+            if (!mIsCurrentlyExtendingSelection) {
+                mIsCurrentlyExtendingSelection = true;
+                mSelectionStart = itemStartIndex;
+            }
+
+            selectionEvent.setFromIndex(mSelectionStart);
+            selectionEvent.setToIndex(itemEndIndex);
+
         } else {
-            mSelectionEndIndex = itemStartIndex;
-        }
-        if (!extendSelection) {
-            mSelectionStartIndex = mSelectionEndIndex;
-        }
-        if (WebContentsAccessibilityImplJni.get().isEditableText(
-                    mNativeObj, WebContentsAccessibilityImpl.this, mSelectionNodeId)
-                && WebContentsAccessibilityImplJni.get().isFocused(
-                        mNativeObj, WebContentsAccessibilityImpl.this, mSelectionNodeId)) {
-            WebContentsAccessibilityImplJni.get().setSelection(mNativeObj,
-                    WebContentsAccessibilityImpl.this, mSelectionNodeId, mSelectionStartIndex,
-                    mSelectionEndIndex);
+            // User is no longer selecting, or wasn't originally, reset values
+            mIsCurrentlyExtendingSelection = false;
+            mSelectionStart = -1;
+
+            // Set selection to/from indices to new cursor position, itemEndIndex with forwards nav
+            selectionEvent.setFromIndex(itemEndIndex);
+            selectionEvent.setToIndex(itemEndIndex);
         }
 
-        // The selection event's "from" and "to" indices are just a cursor at the focus
-        // end of the movement, or a selection if extendSelection is true.
-        selectionEvent.setFromIndex(mSelectionStartIndex);
-        selectionEvent.setToIndex(mSelectionStartIndex);
+        // Moving forwards, cursor is now at end of granularity move (itemEndIndex)
+        mCursorIndex = itemEndIndex;
         selectionEvent.setItemCount(text.length());
 
-        // The traverse event's "from" and "to" indices surround the item (e.g. the word,
-        // etc.) with no whitespace.
-        if (forwards
-                && WebContentsAccessibilityImplJni.get().isEditableText(
-                        mNativeObj, WebContentsAccessibilityImpl.this, mSelectionNodeId)) {
-            traverseEvent.setFromIndex(itemStartIndex - 1);
-            traverseEvent.setToIndex(itemEndIndex - 1);
-        } else {
-            traverseEvent.setFromIndex(itemStartIndex);
-            traverseEvent.setToIndex(itemEndIndex);
-        }
+        // Call back to native code to update selection
+        setSelection(selectionEvent);
+
+        // Build traverse event, set appropriate action
+        traverseEvent.setFromIndex(itemStartIndex);
+        traverseEvent.setToIndex(itemEndIndex);
         traverseEvent.setItemCount(text.length());
         traverseEvent.setMovementGranularity(mSelectionGranularity);
         traverseEvent.setContentDescription(text);
+        traverseEvent.setAction(AccessibilityNodeInfo.ACTION_NEXT_AT_MOVEMENT_GRANULARITY);
+
+        mView.requestSendAccessibilityEvent(mView, selectionEvent);
+        mView.requestSendAccessibilityEvent(mView, traverseEvent);
+
+        // Suppress the next event since we have already sent traverse and selection for this move
+        mSuppressNextSelectionEvent = true;
+    }
+
+    @CalledByNative
+    private void finishGranularityMovePrevious(
+            String text, boolean extendSelection, int itemStartIndex, int itemEndIndex) {
+        // Prepare to send both a selection and a traversal event in sequence.
+        AccessibilityEvent selectionEvent = buildAccessibilityEvent(
+                mSelectionNodeId, AccessibilityEvent.TYPE_VIEW_TEXT_SELECTION_CHANGED);
+        if (selectionEvent == null) return;
+
+        AccessibilityEvent traverseEvent = buildAccessibilityEvent(mSelectionNodeId,
+                AccessibilityEvent.TYPE_VIEW_TEXT_TRAVERSED_AT_MOVEMENT_GRANULARITY);
+        if (traverseEvent == null) {
+            selectionEvent.recycle();
+            return;
+        }
+
+        // Build selection event dependent on whether user is extending selection or not
+        if (extendSelection) {
+            // User started selecting, set the selection start point (only set once per selection)
+            if (!mIsCurrentlyExtendingSelection) {
+                mIsCurrentlyExtendingSelection = true;
+                mSelectionStart = itemEndIndex;
+            }
+
+            selectionEvent.setFromIndex(mSelectionStart);
+            selectionEvent.setToIndex(itemStartIndex);
 
-        // The traverse event needs to set its associated action that triggered it.
-        if (forwards) {
-            traverseEvent.setAction(AccessibilityNodeInfo.ACTION_NEXT_AT_MOVEMENT_GRANULARITY);
         } else {
-            traverseEvent.setAction(AccessibilityNodeInfo.ACTION_PREVIOUS_AT_MOVEMENT_GRANULARITY);
+            // User is no longer selecting, or wasn't originally, reset values
+            mIsCurrentlyExtendingSelection = false;
+            mSelectionStart = -1;
+
+            // Set selection to/from indices to new cursor position, itemStartIndex with back nav
+            selectionEvent.setFromIndex(itemStartIndex);
+            selectionEvent.setToIndex(itemStartIndex);
         }
 
+        // Moving backwards, cursor is now at the start of the granularity move (itemStartIndex)
+        mCursorIndex = itemStartIndex;
+        selectionEvent.setItemCount(text.length());
+
+        // Call back to native code to update selection
+        setSelection(selectionEvent);
+
+        // Build traverse event, set appropriate action
+        traverseEvent.setFromIndex(itemStartIndex);
+        traverseEvent.setToIndex(itemEndIndex);
+        traverseEvent.setItemCount(text.length());
+        traverseEvent.setMovementGranularity(mSelectionGranularity);
+        traverseEvent.setContentDescription(text);
+        traverseEvent.setAction(AccessibilityNodeInfo.ACTION_PREVIOUS_AT_MOVEMENT_GRANULARITY);
+
         mView.requestSendAccessibilityEvent(mView, selectionEvent);
         mView.requestSendAccessibilityEvent(mView, traverseEvent);
+
+        // Suppress the next event since we have already sent traverse and selection for this move
+        mSuppressNextSelectionEvent = true;
+    }
+
+    private void setSelection(AccessibilityEvent selectionEvent) {
+        if (WebContentsAccessibilityImplJni.get().isEditableText(
+                    mNativeObj, WebContentsAccessibilityImpl.this, mSelectionNodeId)
+                && WebContentsAccessibilityImplJni.get().isFocused(
+                        mNativeObj, WebContentsAccessibilityImpl.this, mSelectionNodeId)) {
+            WebContentsAccessibilityImplJni.get().setSelection(mNativeObj,
+                    WebContentsAccessibilityImpl.this, mSelectionNodeId,
+                    selectionEvent.getFromIndex(), selectionEvent.getToIndex());
+        }
     }
 
     private boolean scrollForward(int virtualViewId) {
@@ -829,9 +897,10 @@ public class WebContentsAccessibilityImpl extends AccessibilityNodeProvider
         // focus is on the edit field. Granularity move needs to know where the input focus is.
         mSelectionNodeId = mAccessibilityFocusId;
         mSelectionGranularity = NO_GRANULARITY_SELECTED;
-        mSelectionStartIndex = -1;
-        mSelectionEndIndex = WebContentsAccessibilityImplJni.get().getTextLength(
-                mNativeObj, WebContentsAccessibilityImpl.this, newAccessibilityFocusId);
+        mIsCurrentlyExtendingSelection = false;
+        mSelectionStart = -1;
+        mCursorIndex = -1;
+        mSuppressNextSelectionEvent = false;
 
         if (WebContentsAccessibilityImplJni.get().isAutofillPopupNode(
                     mNativeObj, WebContentsAccessibilityImpl.this, mAccessibilityFocusId)) {
@@ -893,6 +962,13 @@ public class WebContentsAccessibilityImpl extends AccessibilityNodeProvider
             return;
         }
 
+        // Do not send an event when we want to suppress this event, update flag for next event
+        if (mSuppressNextSelectionEvent
+                && eventType == AccessibilityEvent.TYPE_VIEW_TEXT_SELECTION_CHANGED) {
+            mSuppressNextSelectionEvent = false;
+            return;
+        }
+
         AccessibilityEvent event = buildAccessibilityEvent(virtualViewId, eventType);
         if (event != null) {
             mView.requestSendAccessibilityEvent(mView, event);