Accessibility Image Descriptions Port, Part VII - Incognito

This CL is the seventh part of an effort to port the existing Desktop
feature to Android. This feature enables a user to send an image to
Google to process to generate a descriptive alt text if a website
does not provide one.

Design Doc: go/2020-q1-android-image-descriptions
Slide Deck: go/clank-imageDescriptions
Launch Bug: 1057168

Original Desktop Design Doc for reference:
go/chrome-a11y-annotations-design

This is a conservative approach, we use a separate set of profile
Prefs for this feature rather than syncing with the Desktop Prefs.
This can be updated in time as needed.

----------

This CL adds the following:

- Syncs Regular and Incognito behavior as per new reqs
- Corrects unit tests for above
- Adds an AXMode observer to watch for screen readers being enabled
- Adds TabModelObserver to proactively dismiss dialog on tab close

----------

This CL is expanded upon the original CL here: crrev.com/c/2585587
which was reverted here: crrev.com/c/2589933

Rather than a simple Reland, this CL is the original CL plus
improvementss and a bug fix to prevent further issues.


AX-Relnotes: Android accessibility image descriptions behavior in incognito tabs will now match the regular tabs behavior for that profile.
Bug: 1057169, 1158342
Change-Id: I92d9dd87081ca9b7efb09be93ca32f9be7050768
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2596417Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Mark Schillaci <mschillaci@google.com>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Mark Schillaci <mschillaci@google.com>
Cr-Commit-Position: refs/heads/master@{#843359}

chrome/android/java/src/org/chromium/chrome/browser/app/appmenu/AppMenuPropertiesDelegateImpl.java

@@ -484,7 +484,7 @@ public class AppMenuPropertiesDelegateImpl implements AppMenuPropertiesDelegate
             menu.findItem(R.id.get_image_descriptions_id).setVisible(true);
 
             int titleId = R.string.menu_stop_image_descriptions;
-            Profile profile = Profile.fromWebContents(currentTab.getWebContents());
+            Profile profile = Profile.getLastUsedRegularProfile();
             // If image descriptions are not enabled, then we want the menu item to be "Get".
             if (!ImageDescriptionsController.getInstance().imageDescriptionsEnabled(profile)) {
                 titleId = R.string.menu_get_image_descriptions;

chrome/android/junit/src/org/chromium/chrome/browser/app/appmenu/AppMenuPropertiesDelegateUnitTest.java

@@ -51,7 +51,6 @@ import org.chromium.chrome.browser.multiwindow.MultiWindowModeStateDispatcher;
 import org.chromium.chrome.browser.omaha.UpdateMenuItemHelper;
 import org.chromium.chrome.browser.preferences.Pref;
 import org.chromium.chrome.browser.profiles.Profile;
-import org.chromium.chrome.browser.profiles.ProfileJni;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tabmodel.TabModel;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
@@ -116,8 +115,6 @@ public class AppMenuPropertiesDelegateUnitTest {
     @Mock
     private UserPrefs.Natives mUserPrefsJniMock;
     @Mock
-    private Profile.Natives mProfileJniMock;
-    @Mock
     private Profile mProfile;
     @Mock
     private PrefService mPrefService;
@@ -153,10 +150,8 @@ public class AppMenuPropertiesDelegateUnitTest {
 
         mJniMocker.mock(ContentFeatureListImplJni.TEST_HOOKS, mContentFeatureListJniMock);
         mJniMocker.mock(UserPrefsJni.TEST_HOOKS, mUserPrefsJniMock);
-        mJniMocker.mock(ProfileJni.TEST_HOOKS, mProfileJniMock);
         Profile.setLastUsedProfileForTesting(mProfile);
         Mockito.when(mUserPrefsJniMock.get(mProfile)).thenReturn(mPrefService);
-        Mockito.when(mProfileJniMock.fromWebContents(any())).thenReturn(mProfile);
         FeatureList.setTestCanUseDefaultsForTesting();
 
         mAppMenuPropertiesDelegate = Mockito.spy(new AppMenuPropertiesDelegateImpl(

chrome/browser/accessibility/accessibility_labels_service.cc

@@ -37,6 +37,7 @@
 #include "base/android/jni_android.h"
 #include "chrome/browser/image_descriptions/jni_headers/ImageDescriptionsController_jni.h"
 #include "content/public/browser/web_contents.h"
+#include "ui/accessibility/platform/ax_platform_node.h"
 #endif
 
 using LanguageInfo = language::UrlLanguageHistogram::LanguageInfo;
@@ -123,10 +124,28 @@ class ImageAnnotatorClient : public image_annotation::Annotator::Client {
 }  // namespace
 
 #if !defined(OS_ANDROID)
+AccessibilityLabelsService::AccessibilityLabelsService(Profile* profile)
+    : profile_(profile) {}
 AccessibilityLabelsService::~AccessibilityLabelsService() = default;
 #else
+// On Android we must add/remove a NetworkChangeObserver during construction/
+// destruction to provide the "Only on Wi-Fi" functionality.
+// We also add/remove an AXModeObserver to track users enabling a screenreader.
+AccessibilityLabelsService::AccessibilityLabelsService(Profile* profile)
+    : profile_(profile) {
+  // Ensure the |BrowserAccessibilityState| is constructed before adding any
+  // observers. The |BrowserAccessibilityState| may change the accessibility
+  // mode in its constructor, so if we register the observer before the
+  // constructor, we will get a crash.
+  auto* state = content::BrowserAccessibilityState::GetInstance();
+  DCHECK(state);
+
+  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+  ui::AXPlatformNode::AddAXModeObserver(this);
+}
 AccessibilityLabelsService::~AccessibilityLabelsService() {
   net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+  ui::AXPlatformNode::RemoveAXModeObserver(this);
 }
 #endif
 
@@ -157,12 +176,6 @@ void AccessibilityLabelsService::InitOffTheRecordPrefs(
       prefs::kAccessibilityImageLabelsEnabled, false);
   off_the_record_profile->GetPrefs()->SetBoolean(
       prefs::kAccessibilityImageLabelsOptInAccepted, false);
-#if defined(OS_ANDROID)
-  off_the_record_profile->GetPrefs()->SetBoolean(
-      prefs::kAccessibilityImageLabelsEnabledAndroid, false);
-  off_the_record_profile->GetPrefs()->SetBoolean(
-      prefs::kAccessibilityImageLabelsOnlyOnWifi, true);
-#endif
 }
 
 void AccessibilityLabelsService::Init() {
@@ -189,13 +202,6 @@ void AccessibilityLabelsService::Init() {
           weak_factory_.GetWeakPtr()));
 }
 
-AccessibilityLabelsService::AccessibilityLabelsService(Profile* profile)
-    : profile_(profile) {
-#if defined(OS_ANDROID)
-  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
-#endif
-}
-
 ui::AXMode AccessibilityLabelsService::GetAXMode() {
   ui::AXMode ax_mode =
       content::BrowserAccessibilityState::GetInstance()->GetAccessibilityMode();
@@ -302,6 +308,13 @@ void AccessibilityLabelsService::OnNetworkChanged(
       ->SetImageLabelsModeForProfile(GetAndroidEnabledStatus(), profile_);
 }
 
+void AccessibilityLabelsService::OnAXModeAdded(ui::AXMode mode) {
+  // When the AXMode changes (e.g. user turned on a screenreader), we want to
+  // (potentially) update the AXMode of all web contents for current profile.
+  content::BrowserAccessibilityState::GetInstance()
+      ->SetImageLabelsModeForProfile(GetAndroidEnabledStatus(), profile_);
+}
+
 bool AccessibilityLabelsService::GetAndroidEnabledStatus() {
   // On Android, user has an option to toggle "only on wifi", so also check
   // the current connection type if necessary.
@@ -326,6 +339,9 @@ void JNI_ImageDescriptionsController_GetImageDescriptionsOnce(
   content::WebContents* web_contents =
       content::WebContents::FromJavaWebContents(j_web_contents);
 
+  if (!web_contents)
+    return;
+
   ui::AXActionData action_data;
   action_data.action = ax::mojom::Action::kAnnotatePageImages;
 

chrome/browser/accessibility/accessibility_labels_service.h

@@ -16,6 +16,7 @@
 
 #if defined(OS_ANDROID)
 #include "net/base/network_change_notifier.h"
+#include "ui/accessibility/ax_mode_observer.h"
 #endif
 
 class Profile;
@@ -35,9 +36,11 @@ class PrefRegistrySyncable;
 class AccessibilityLabelsService
     : public KeyedService
 #if defined(OS_ANDROID)
-    // On Android, implement NetworkChangeObserver for "only on wifi" option.
+    // On Android, implement NetworkChangeObserver for "only on wifi" option,
+    // and an AXModeObserver for detecting when a screen reader is enabled.
     ,
-      public net::NetworkChangeNotifier::NetworkChangeObserver
+      public net::NetworkChangeNotifier::NetworkChangeObserver,
+      public ui::AXModeObserver
 #endif
 {
 
@@ -71,6 +74,9 @@ class AccessibilityLabelsService
   void OnNetworkChanged(
       net::NetworkChangeNotifier::ConnectionType type) override;
 
+  // ui::AXModeObserver
+  void OnAXModeAdded(ui::AXMode mode) override;
+
   bool GetAndroidEnabledStatus();
 #endif
 

chrome/browser/image_descriptions/BUILD.gn

@@ -28,7 +28,6 @@ android_library("java") {
     "//content/public/android:content_java",
     "//net/android:net_java",
     "//third_party/android_deps:androidx_annotation_annotation_java",
-    "//third_party/android_deps:androidx_fragment_fragment_java",
     "//third_party/android_deps:androidx_preference_preference_java",
     "//ui/android:ui_full_java",
   ]

chrome/browser/image_descriptions/android/java/src/org/chromium/chrome/browser/image_descriptions/ImageDescriptionsController.java

@@ -116,7 +116,7 @@ public class ImageDescriptionsController {
      */
     public void onImageDescriptionsMenuItemSelected(
             Context context, ModalDialogManager modalDialogManager, WebContents webContents) {
-        Profile profile = Profile.fromWebContents(webContents);
+        Profile profile = Profile.getLastUsedRegularProfile();
         boolean enabledBeforeMenuItemSelected = imageDescriptionsEnabled(profile);
 
         if (enabledBeforeMenuItemSelected) {

chrome/browser/image_descriptions/android/java/src/org/chromium/chrome/browser/image_descriptions/ImageDescriptionsControllerTest.java

@@ -41,7 +41,6 @@ import org.chromium.chrome.browser.preferences.ChromePreferenceKeys;
 import org.chromium.chrome.browser.preferences.Pref;
 import org.chromium.chrome.browser.preferences.SharedPreferencesManager;
 import org.chromium.chrome.browser.profiles.Profile;
-import org.chromium.chrome.browser.profiles.ProfileJni;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.components.prefs.PrefService;
 import org.chromium.components.user_prefs.UserPrefs;
@@ -67,9 +66,6 @@ public class ImageDescriptionsControllerTest extends DummyUiActivityTestCase {
     @Mock
     private UserPrefs.Natives mUserPrefsJniMock;
 
-    @Mock
-    private Profile.Natives mProfileJniMock;
-
     @Mock
     private Profile mProfile;
 
@@ -95,9 +91,6 @@ public class ImageDescriptionsControllerTest extends DummyUiActivityTestCase {
         Profile.setLastUsedProfileForTesting(mProfile);
         when(mUserPrefsJniMock.get(mProfile)).thenReturn(mPrefService);
 
-        mJniMocker.mock(ProfileJni.TEST_HOOKS, mProfileJniMock);
-        when(mProfileJniMock.fromWebContents(any())).thenReturn(mProfile);
-
         mJniMocker.mock(ImageDescriptionsControllerJni.TEST_HOOKS, mControllerJniMock);
 
         resetSharedPreferences();

chrome/browser/image_descriptions/android/java/src/org/chromium/chrome/browser/image_descriptions/ImageDescriptionsDialog.java

@@ -10,12 +10,16 @@ import android.view.View;
 import android.widget.CheckBox;
 import android.widget.RadioGroup;
 
+import androidx.annotation.Nullable;
+
 import org.chromium.chrome.browser.device.DeviceConditions;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.browser_ui.widget.RadioButtonWithDescription;
 import org.chromium.components.browser_ui.widget.RadioButtonWithDescriptionLayout;
 import org.chromium.content_public.browser.WebContents;
+import org.chromium.content_public.browser.WebContentsObserver;
 import org.chromium.net.ConnectionType;
+import org.chromium.ui.base.WindowAndroid;
 import org.chromium.ui.modaldialog.DialogDismissalCause;
 import org.chromium.ui.modaldialog.ModalDialogManager;
 import org.chromium.ui.modaldialog.ModalDialogProperties;
@@ -33,6 +37,7 @@ public class ImageDescriptionsDialog
 
     private ModalDialogManager mModalDialogManager;
     private PropertyModel mPropertyModel;
+    private WebContentsObserver mWebContentsObserver;
 
     private RadioButtonWithDescriptionLayout mRadioGroup;
     private RadioButtonWithDescription mOptionJustOnceRadioButton;
@@ -52,7 +57,7 @@ public class ImageDescriptionsDialog
         mModalDialogManager = modalDialogManager;
         mControllerDelegate = delegate;
         mWebContents = webContents;
-        mProfile = Profile.fromWebContents(mWebContents);
+        mProfile = Profile.getLastUsedRegularProfile();
         mContext = context;
 
         // Set initial state.
@@ -87,6 +92,32 @@ public class ImageDescriptionsDialog
             updateOptionalCheckbox(R.string.dont_ask_again, mDontAskAgainState);
         }
 
+        // Create a |WebContentsObserver| to track changes in state for which we should
+        // dismiss the dialog, such as navigation, and |mWebContents| being hidden or destroyed.
+        mWebContentsObserver = new WebContentsObserver(mWebContents) {
+            @Override
+            public void wasHidden() {
+                unregisterObserver();
+            }
+
+            @Override
+            public void navigationEntryCommitted() {
+                unregisterObserver();
+            }
+
+            @Override
+            public void onTopLevelNativeWindowChanged(@Nullable WindowAndroid windowAndroid) {
+                // Dismiss the dialog when the associated WebContents is detached from the window.
+                if (windowAndroid == null) unregisterObserver();
+            }
+
+            @Override
+            public void destroy() {
+                super.destroy();
+                dismissEarly();
+            }
+        };
+
         // Build our dialog property model.
         mPropertyModel =
                 new PropertyModel.Builder(ModalDialogProperties.ALL_KEYS)
@@ -169,6 +200,14 @@ public class ImageDescriptionsDialog
         mOptionalCheckbox.setChecked(state);
     }
 
+    /**
+     * Helper method to unregister |mWebContentsObserver| during changes in state to |mWebContents|.
+     * The call to #destroy() will also dismiss the dialog.
+     */
+    private void unregisterObserver() {
+        mWebContentsObserver.destroy();
+    }
+
     /**
      * Helper method to display this dialog.
      */
@@ -176,11 +215,19 @@ public class ImageDescriptionsDialog
         mModalDialogManager.showDialog(mPropertyModel, ModalDialogManager.ModalDialogType.APP);
     }
 
+    /**
+     * Helper method to dismiss this dialog from changes to state of the |mWebContents|.
+     * Consider proactively dismissing the dialog to be a negative button click.
+     */
+    private void dismissEarly() {
+        dismiss(DialogDismissalCause.NEGATIVE_BUTTON_CLICKED);
+    }
+
     /**
      * Helper method to dismiss this dialog.
      * @param dialogDismissableCause        DialogDismissalCause, e.g. positive or negative
      */
-    public void dismiss(int dialogDismissableCause) {
+    private void dismiss(int dialogDismissableCause) {
         mModalDialogManager.dismissDialog(mPropertyModel, dialogDismissableCause);
     }
 }

chrome/browser/image_descriptions/android/java/src/org/chromium/chrome/browser/image_descriptions/ImageDescriptionsDialogTest.java

@@ -44,7 +44,6 @@ import org.chromium.chrome.browser.preferences.ChromePreferenceKeys;
 import org.chromium.chrome.browser.preferences.Pref;
 import org.chromium.chrome.browser.preferences.SharedPreferencesManager;
 import org.chromium.chrome.browser.profiles.Profile;
-import org.chromium.chrome.browser.profiles.ProfileJni;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.components.browser_ui.modaldialog.AppModalPresenter;
 import org.chromium.components.browser_ui.widget.RadioButtonWithDescription;
@@ -72,9 +71,6 @@ public class ImageDescriptionsDialogTest extends DummyUiActivityTestCase {
     @Mock
     private UserPrefs.Natives mUserPrefsJniMock;
 
-    @Mock
-    private Profile.Natives mProfileJniMock;
-
     @Mock
     private Profile mProfile;
 
@@ -99,9 +95,6 @@ public class ImageDescriptionsDialogTest extends DummyUiActivityTestCase {
         Profile.setLastUsedProfileForTesting(mProfile);
         when(mUserPrefsJniMock.get(mProfile)).thenReturn(mPrefService);
 
-        mJniMocker.mock(ProfileJni.TEST_HOOKS, mProfileJniMock);
-        when(mProfileJniMock.fromWebContents(any())).thenReturn(mProfile);
-
         mAppModalPresenter = new AppModalPresenter(getActivity());
         mModalDialogManager =
                 new ModalDialogManager(mAppModalPresenter, ModalDialogManager.ModalDialogType.APP);