Android Autofill Accessibility, Phase I

Adds accessibility navigation into autofill popup using right swipes. The autofill popup does not have a corresponding AX node in the AX tree sent by renderer to browser.  Hence, we create a proxy AX node that is "attached" to the field that is displaying the popup for navigation.

This feature is currently disabled by default because we have to improve the navigation as follows:

1. Navigate to autofill popup only on double-tap. Currently right swipe takes user into the auto popup.

2. When user right swipes out of last suggestion in the popup, we must navigate to the HTML element that is after the field hosting the popup.

3. When user presses back button, the user is able to edit the text field but the accessibility navigation focus remains on the back button.

Please refer to https://drive.google.com/a/google.com/file/d/0B-jTnU8uuAL9WV9RWXpvYTVQaDQ/view?usp=sharing



BUG=627860
Review-Url: https://codereview.chromium.org/2762123006
Cr-Commit-Position: refs/heads/master@{#461610}
Committed: https://chromium.googlesource.com/chromium/src/+/df20b103282ad2d7103cd5c078712893ba92e255

[csashi, 2017-03-22 01:11:35]

csashi@google.com changed reviewers:
+ dmazzoni@chromium.org

[csashi, 2017-03-22 01:11:36]

Hi Dominic,
I created this patch so it will be easy to discuss how to trap the double-tap
and navigate through the popup. Please review only cursorily, or I can explain
face-to-face. Thanks,
-sashi.

[dmazzoni, 2017-03-22 16:13:54]

This seems along the right lines to me. How well is it working?

https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
(right):

https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:122:
mBrowserAccessibilityManager.OnAutofillPopupDisplayed();
Would it make sense to pass the pop-up View to BrowserAccessibilityManager here?

https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:343:
popup_node_ = BrowserAccessibility::Create();
Maybe this should be called popup_dialog_proxy_node or something like that?

https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:357:
potential_popup_unique_id_ = current_unique_id_;
To clarify, is "potential popup" the node that's the "host" of the popup?

Maybe getting this from the current focused node would be more accurate.
We should confirm that it's the right type of node we expect, then give the
variable a name something like that. It's basically the focused element that
the autofill popup is showing for.

[dmazzoni, 2017-03-22 16:17:44]

TalkBack moves around by setting accessibility focus. I think the Popup
bridge should pass the popup View directly to BrowserAccessibilityManager.
Then when BrowserAccessibilityManager detects that the user has swiped into
the "proxy" node for the popup, it should fire a "set accessibility focus"
event on the real popup View. That will move TalkBack focus inside the real
popup. It should be possible to "trap" it there until the user either
interacts with it, or presses Back. When they press Back, we just tell the
BrowserAccessibilityManager to move accessibility focus back to the element
that was being autofilled.


On Wed, Mar 22, 2017 at 9:13 AM <dmazzoni@chromium.org> wrote:

> This seems along the right lines to me. How well is it working?
>
>
>
>
>
https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
> File
>
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
> (right):
>
>
>
https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
>
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:122:
> mBrowserAccessibilityManager.OnAutofillPopupDisplayed();
> Would it make sense to pass the pop-up View to
> BrowserAccessibilityManager here?
>
>
>
https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
> File
> content/browser/accessibility/browser_accessibility_manager_android.cc
> (right):
>
>
>
https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_manager_android.cc:343:
> popup_node_ = BrowserAccessibility::Create();
> Maybe this should be called popup_dialog_proxy_node or something like
> that?
>
>
>
https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_manager_android.cc:357:
> potential_popup_unique_id_ = current_unique_id_;
> To clarify, is "potential popup" the node that's the "host" of the
> popup?
>
> Maybe getting this from the current focused node would be more accurate.
> We should confirm that it's the right type of node we expect, then give
> the
> variable a name something like that. It's basically the focused element
> that
> the autofill popup is showing for.
>
> https://codereview.chromium.org/2762123006/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[csashi, 2017-03-24 01:32:23]

Hi Dominic,

Things that are working:

1. Right swipe takes us into the popup.
2. Back press brings focus back to edit box and user can edit field.

Things that are not working:

1. I am unable to trap double-tap to activate the popup. Right swipe from the
field that is displaying the popup takes us to the popup. Perhaps this is OK?

2. Back press takes focus back to edit box and we can edit, but I can't find a
way to right swipe to the HTML element that is the next element after the field
displaying the popup.  Accessibility focus remains on the back button. I need to
figure out a way to handle navigation after turning the focus into the popup but
we don't receive performAction calls inside BrowserAccessibilityManager.java
once we have changed focus to inside popup.

Can you please take a look and let me know your thoughts? I will upload a video
for your convenience. Thanks,
-sashi.

https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
(right):

https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:122:
mBrowserAccessibilityManager.OnAutofillPopupDisplayed();
On 2017/03/22 16:13:54, dmazzoni wrote:
> Would it make sense to pass the pop-up View to BrowserAccessibilityManager
here?

Done.

https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:343:
popup_node_ = BrowserAccessibility::Create();
On 2017/03/22 16:13:54, dmazzoni wrote:
> Maybe this should be called popup_dialog_proxy_node or something like that?

Done.

https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:357:
potential_popup_unique_id_ = current_unique_id_;
On 2017/03/22 16:13:54, dmazzoni wrote:
> To clarify, is "potential popup" the node that's the "host" of the popup?
> 
> Maybe getting this from the current focused node would be more accurate.
> We should confirm that it's the right type of node we expect, then give the
> variable a name something like that. It's basically the focused element that
> the autofill popup is showing for.
> 

Done.

[csashi, 2017-03-24 16:32:16]

Hi Dominic,
For your convenience, I created a video @
https://drive.google.com/a/google.com/file/d/0B-jTnU8uuAL9WV9RWXpvYTVQaDQ/vie...

At the 16 second mark you can see that the right swipe takes us into the popup
At the 27 second mark you can see that the back button moves focus back to the
edit box.

However,
I am not able to trap double-tap to activate the popup.
At the 46 second mark you can see that the right swipe takes us out of the popup
but not into the next HTML element. This is because I do not receive
ACTION_NEXT_HTML_ELEMENT once I have relinquished focus to inside the Android
ListView.

Please let me know how we can improve this.
Thanks,
-sashi.

On 2017/03/24 01:32:23, csashi wrote:
> Hi Dominic,
> 
> Things that are working:
> 
> 1. Right swipe takes us into the popup.
> 2. Back press brings focus back to edit box and user can edit field.
> 
> Things that are not working:
> 
> 1. I am unable to trap double-tap to activate the popup. Right swipe from the
> field that is displaying the popup takes us to the popup. Perhaps this is OK?
> 
> 2. Back press takes focus back to edit box and we can edit, but I can't find a
> way to right swipe to the HTML element that is the next element after the
field
> displaying the popup.  Accessibility focus remains on the back button. I need
to
> figure out a way to handle navigation after turning the focus into the popup
but
> we don't receive performAction calls inside BrowserAccessibilityManager.java
> once we have changed focus to inside popup.
> 
> Can you please take a look and let me know your thoughts? I will upload a
video
> for your convenience. Thanks,
> -sashi.
> 
>
https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
> (right):
> 
>
https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:122:
> mBrowserAccessibilityManager.OnAutofillPopupDisplayed();
> On 2017/03/22 16:13:54, dmazzoni wrote:
> > Would it make sense to pass the pop-up View to BrowserAccessibilityManager
> here?
> 
> Done.
> 
>
https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
> File content/browser/accessibility/browser_accessibility_manager_android.cc
> (right):
> 
>
https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_manager_android.cc:343:
> popup_node_ = BrowserAccessibility::Create();
> On 2017/03/22 16:13:54, dmazzoni wrote:
> > Maybe this should be called popup_dialog_proxy_node or something like that?
> 
> Done.
> 
>
https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_manager_android.cc:357:
> potential_popup_unique_id_ = current_unique_id_;
> On 2017/03/22 16:13:54, dmazzoni wrote:
> > To clarify, is "potential popup" the node that's the "host" of the popup?
> > 
> > Maybe getting this from the current focused node would be more accurate.
> > We should confirm that it's the right type of node we expect, then give the
> > variable a name something like that. It's basically the focused element that
> > the autofill popup is showing for.
> > 
> 
> Done.

[dmazzoni, 2017-03-24 20:41:29]

On Thu, Mar 23, 2017 at 6:32 PM csashi via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> 1. I am unable to trap double-tap to activate the popup. Right swipe from
> the
> field that is displaying the popup takes us to the popup. Perhaps this is
> OK?
>

Oh, I think I see the problem. Actually on O and higher, this will be easy
because double-tap sends a Click event.

But on N and lower, double-tap just sends a tap event centered in the
middle of whatever has accessibility focus. I think it would be possible to
intercept but it'd be more work.


> 2. Back press takes focus back to edit box and we can edit, but I can't
> find a
> way to right swipe to the HTML element that is the next element after the
> field
> displaying the popup. Accessibility focus remains on the back button. I
> need to
> figure out a way to handle navigation after turning the focus into the
> popup but
> we don't receive performAction calls inside
> BrowserAccessibilityManager.java
> once we have changed focus to inside popup.
>

I think the popup should communicate back to BrowserAccessibilityManager
explicitly when focus moves out of it again, can you do that?


> Can you please take a look and let me know your thoughts? I will upload a
> video
> for your convenience. Thanks,
> -sashi.
>
>
>
>
>
https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
> File
>
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
> (right):
>
>
>
https://codereview.chromium.org/2762123006/diff/1/chrome/android/java/src/org...
>
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:122:
> mBrowserAccessibilityManager.OnAutofillPopupDisplayed();
> On 2017/03/22 16:13:54, dmazzoni wrote:
> > Would it make sense to pass the pop-up View to
> BrowserAccessibilityManager here?
>
> Done.
>
>
>
>
https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
> File
> content/browser/accessibility/browser_accessibility_manager_android.cc
> (right):
>
>
>
https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_manager_android.cc:343:
> popup_node_ = BrowserAccessibility::Create();
> On 2017/03/22 16:13:54, dmazzoni wrote:
> > Maybe this should be called popup_dialog_proxy_node or something like
> that?
>
> Done.
>
>
>
>
https://codereview.chromium.org/2762123006/diff/1/content/browser/accessibili...
> content/browser/accessibility/browser_accessibility_manager_android.cc:357:
> potential_popup_unique_id_ = current_unique_id_;
> On 2017/03/22 16:13:54, dmazzoni wrote:
> > To clarify, is "potential popup" the node that's the "host" of the
> popup?
> >
> > Maybe getting this from the current focused node would be more
> accurate.
> > We should confirm that it's the right type of node we expect, then
> give the
> > variable a name something like that. It's basically the focused
> element that
> > the autofill popup is showing for.
> >
>
> Done.
>
> https://codereview.chromium.org/2762123006/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[csashi, 2017-03-28 04:58:28]

Description was changed from

==========
Prototype implementation for Android Autofill Accessibility.

BUG=627860
==========

to

==========
Adds accessibility navigation into autofill popup using right swipes. The
autofill popup does not have a corresponding AX node in the AX tree sent by
renderer to browser.  Hence, we create a proxy AX node that is "attached" to the
field that is displaying the popup for navigation.

This feature is currently disabled by default because we have to improve the
navigation as follows:

1. Navigate to autofill popup only on double-tap. Currently right swipe takes
user into the auto popup.

2. When user right swipes out of last suggestion in the popup, we must navigate
to the HTML element that is after the field hosting the popup.

3. When user presses back button, the user is able to edit the text field but
the accessibility navigation focus remains on the back button.

Please refer to
https://drive.google.com/a/google.com/file/d/0B-jTnU8uuAL9WV9RWXpvYTVQaDQ/vie...



BUG=627860
==========

[csashi, 2017-03-28 04:58:28]

csashi@google.com changed reviewers:
+ mathp@chromium.org, tedchoc@chromium.org

[csashi, 2017-03-28 05:01:45]

Hi Dominic,
As discussed, I have bracketed the change under a feature flag that is currently
disabled. I left the back button press code because the feature is usable, even
if not ideal, with the back button.

Hi Mathieu,
Can you please take a look @
chrome/android/java/src/org/chromium/chrome/browser/autofill/ ?

Hi Ted,
Can you please take a look @
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java?

Please feel free to pass on any general advice on how to resolve the issues
mentioned above and in the review thread. Thanks!
-sashi.

[csashi, 2017-03-28 05:06:59]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 05:07:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 05:30:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 05:30:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[csashi, 2017-03-28 16:16:23]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 16:17:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[csashi, 2017-03-28 16:45:02]

On 2017/03/28 05:01:45, csashi wrote:
> Hi Dominic,
> As discussed, I have bracketed the change under a feature flag that is
currently
> disabled. I left the back button press code because the feature is usable,
even
> if not ideal, with the back button.
> 
> Hi Mathieu,
> Can you please take a look @
> chrome/android/java/src/org/chromium/chrome/browser/autofill/ ?
> 
> Hi Ted,
> Can you please take a look @
> chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java?
> 
> Please feel free to pass on any general advice on how to resolve the issues
> mentioned above and in the review thread. Thanks!
> -sashi.

Hi Dominic,
Turns out I did not need to handle back pressed to let user edit the field. So I
removed that.
Please take a look. Thanks!
-sashi.

[csashi, 2017-03-28 16:45:18]

csashi@google.com changed reviewers:
- tedchoc@chromium.org

[csashi, 2017-03-28 16:45:39]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 16:46:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-03-28 16:47:35]

https://codereview.chromium.org/2762123006/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2762123006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1490:
&& browserAccessibilityManager.handleBackPressed()) {
Instead of handleBackPressed(), maybe call this something
more specific like refocusAfterPopupCloses()

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:124:
int32_t element_hosting_autofill_popup_id_ = -1;
I think globals like this are typically named something
like g_element_hosting_autofill_popup_id

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:128:
int32_t current_focused_id_ = -1;
You don't need to keep track of this. Just call GetFocus()
and get the ID of the return value.

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:970:
LOG(ERROR) << "AutofillPopup proxy node exists, unique_id="
Remove this debugging

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:975:
autofill_popup_proxy_node_ = BrowserAccessibility::Create();
Do we need a check that this doesn't already exist?

There's no chance OnAutofillPopupDisplayed could be
called twice?

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:988:
element_hosting_autofill_popup_id_ = current_focused_id_;
Use GetFocus()->GetId() here

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:996: if
(autofill_popup_proxy_node_) {
Do we also need to clean this up if
BrowserAccessibilityManagerAndroid is destroyed
before OnAutofillPopupDismissed is called, like if a
tab is closed?

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_android.h
(right):

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.h:232: const
base::android::JavaParamRef<jobject>& obj);
Do we need both IsAutofillPopupNode and
GetElementHostingAutofillPopupId?

https://codereview.chromium.org/2762123006/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java
(right):

https://codereview.chromium.org/2762123006/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:77:
private ListView mAutofillPopupListView;
Does this class need to know it's a ListView or could
it just take a View?

https://codereview.chromium.org/2762123006/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:563:
mAutofillPopupListView.sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED);
Do you need to call this? requestFocus() should be
sufficient.

https://codereview.chromium.org/2762123006/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:574:
if (mAccessibilityManager.isEnabled() && mNativeObj != 0
Reverse these tests to have less indentation and easier
readability:

if (!mAccessibilityManager.isEnabled() || mNativeObj == 0)
    return false;

...

[commit-bot: I haz the power, 2017-03-28 17:42:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 17:42:34]

Dry run: This issue passed the CQ dry run.

[csashi, 2017-03-28 17:48:10]

Hi Dominic,
Please take a look. Thanks!
-sashi.

https://codereview.chromium.org/2762123006/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2762123006/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1490:
&& browserAccessibilityManager.handleBackPressed()) {
On 2017/03/28 16:47:35, dmazzoni wrote:
> Instead of handleBackPressed(), maybe call this something
> more specific like refocusAfterPopupCloses()

Acknowledged. Looks like your review crossed my update. Turns out I did not need
this change to let user edit the text field.

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:124:
int32_t element_hosting_autofill_popup_id_ = -1;
On 2017/03/28 16:47:35, dmazzoni wrote:
> I think globals like this are typically named something
> like g_element_hosting_autofill_popup_id

Done.

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:970:
LOG(ERROR) << "AutofillPopup proxy node exists, unique_id="
On 2017/03/28 16:47:35, dmazzoni wrote:
> Remove this debugging

Done.

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:975:
autofill_popup_proxy_node_ = BrowserAccessibility::Create();
On 2017/03/28 16:47:35, dmazzoni wrote:
> Do we need a check that this doesn't already exist?

The if statement above checks that.

> 
> There's no chance OnAutofillPopupDisplayed could be
> called twice?

Seems that would be the wrong flow, but it seems better to assume that
OnAutofillPopupDisplayed could be called in succession without an intervening
dismiss.

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:988:
element_hosting_autofill_popup_id_ = current_focused_id_;
On 2017/03/28 16:47:35, dmazzoni wrote:
> Use GetFocus()->GetId() here

Done.

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:996: if
(autofill_popup_proxy_node_) {
On 2017/03/28 16:47:35, dmazzoni wrote:
> Do we also need to clean this up if
> BrowserAccessibilityManagerAndroid is destroyed
> before OnAutofillPopupDismissed is called, like if a
> tab is closed?

Done.

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_android.h
(right):

https://codereview.chromium.org/2762123006/diff/120001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.h:232: const
base::android::JavaParamRef<jobject>& obj);
On 2017/03/28 16:47:35, dmazzoni wrote:
> Do we need both IsAutofillPopupNode and
> GetElementHostingAutofillPopupId?

Done.

https://codereview.chromium.org/2762123006/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java
(right):

https://codereview.chromium.org/2762123006/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:77:
private ListView mAutofillPopupListView;
On 2017/03/28 16:47:35, dmazzoni wrote:
> Does this class need to know it's a ListView or could
> it just take a View?

Done.

https://codereview.chromium.org/2762123006/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:563:
mAutofillPopupListView.sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED);
On 2017/03/28 16:47:35, dmazzoni wrote:
> Do you need to call this? requestFocus() should be
> sufficient.

Done.

https://codereview.chromium.org/2762123006/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:574:
if (mAccessibilityManager.isEnabled() && mNativeObj != 0
On 2017/03/28 16:47:35, dmazzoni wrote:
> Reverse these tests to have less indentation and easier
> readability:
> 
> if (!mAccessibilityManager.isEnabled() || mNativeObj == 0)
>     return false;
> 
> ...
> 
> 

Acknowledged. Function not needed.

[csashi, 2017-03-28 17:48:47]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 17:49:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 19:21:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 19:21:12]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-03-29 17:28:44]

lgtm

https://codereview.chromium.org/2762123006/diff/240001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2762123006/diff/240001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:124:
int32_t g_element_hosting_autofill_popup_id_ = -1;
Nit: no training underscore on globals, only on class members.

https://codereview.chromium.org/2762123006/diff/240001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:991:
g_element_hosting_autofill_popup_id_ = current_focus->unique_id();
Rename g_element_hosting_autofill_popup_id_
g_element_hosting_autofill_popup_unique_id - we have multiple types of ids, and
"unique id" should always be named such to avoid confusion

https://codereview.chromium.org/2762123006/diff/240001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java
(left):

https://codereview.chromium.org/2762123006/diff/240001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:38:
private static final String TAG = "BrowserAccessibilityManager";
Why is this deleted?

[csashi, 2017-03-29 18:26:26]

csashi@google.com changed reviewers:
+ cpu@chromium.org, jochen@chromium.org

[csashi, 2017-03-29 18:26:27]

Hi Mathieu,
Can you approve for
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java?

Hi Carlos,
Can you approve for chrome/browser/, content/app, content/browser and
content/public?

Thanks,
-sashi.

https://codereview.chromium.org/2762123006/diff/240001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2762123006/diff/240001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:124:
int32_t g_element_hosting_autofill_popup_id_ = -1;
On 2017/03/29 17:28:43, dmazzoni wrote:
> Nit: no training underscore on globals, only on class members.

Done.

https://codereview.chromium.org/2762123006/diff/240001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.cc:991:
g_element_hosting_autofill_popup_id_ = current_focus->unique_id();
On 2017/03/29 17:28:43, dmazzoni wrote:
> Rename g_element_hosting_autofill_popup_id_
> g_element_hosting_autofill_popup_unique_id - we have multiple types of ids,
and
> "unique id" should always be named such to avoid confusion

Done.

https://codereview.chromium.org/2762123006/diff/240001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java
(left):

https://codereview.chromium.org/2762123006/diff/240001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:38:
private static final String TAG = "BrowserAccessibilityManager";
On 2017/03/29 17:28:43, dmazzoni wrote:
> Why is this deleted?

It was not used. I can add it back if the convention is to define a TAG even
without Log statements.

[csashi, 2017-03-29 18:26:42]

csashi@google.com changed reviewers:
- jochen@chromium.org

[csashi, 2017-03-29 18:27:44]

Hi Carlos,
Correction. I need your approval only for content/
-sashi.

[csashi, 2017-03-29 18:28:32]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 18:29:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 20:24:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 20:24:05]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[csashi, 2017-03-29 21:12:16]

csashi@google.com changed reviewers:
+ ericwilligers@chromium.org

[csashi, 2017-03-29 21:12:17]

Hi Eric,
Please review changes in tools/metrics/histograms/. Thanks,
-sashi.

[csashi, 2017-03-29 21:17:00]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 21:17:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 22:35:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 22:35:51]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Eric Willigers, 2017-03-29 23:04:48]

ericwilligers@chromium.org changed reviewers:
+ holte@chromium.org
- ericwilligers@chromium.org

[Eric Willigers, 2017-03-29 23:06:35]

On 2017/03/29 21:12:17, csashi wrote:
> Hi Eric,
> Please review changes in tools/metrics/histograms/. Thanks,
> -sashi.

I've added holte@ for the histograms review. I review UseCounter histogram
changes only.

[Steven Holte, 2017-03-30 00:15:27]

histograms lgtm

[Mathieu, 2017-03-30 13:21:54]

lgtm

[csashi, 2017-03-30 16:44:07]

On 2017/03/29 23:06:35, Eric Willigers wrote:
> On 2017/03/29 21:12:17, csashi wrote:
> > Hi Eric,
> > Please review changes in tools/metrics/histograms/. Thanks,
> > -sashi.
> 
> I've added holte@ for the histograms review. I review UseCounter histogram
> changes only.

Hi Eric,
Looks like I need your approval too (according to Chrome Butler).Can you please
approve? Thanks!
-sashi.

[csashi, 2017-03-30 16:44:29]

csashi@google.com changed reviewers:
+ ericwilligers@chromium.org

[csashi, 2017-03-30 19:24:27]

On 2017/03/30 16:44:07, csashi wrote:
> On 2017/03/29 23:06:35, Eric Willigers wrote:
> > On 2017/03/29 21:12:17, csashi wrote:
> > > Hi Eric,
> > > Please review changes in tools/metrics/histograms/. Thanks,
> > > -sashi.
> > 
> > I've added holte@ for the histograms review. I review UseCounter histogram
> > changes only.
> 
> Hi Eric,
> Looks like I need your approval too (according to Chrome Butler).Can you
please
> approve? Thanks!
> -sashi.

Hi Eric,
Sorry - looks like Chrome Butler is not showing the correct info. Steven's
approval should be sufficient.
-sashi.

[csashi, 2017-03-30 19:24:42]

csashi@google.com changed reviewers:
- ericwilligers@chromium.org

[csashi, 2017-03-31 04:19:10]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 04:19:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 05:24:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 05:24:30]

Dry run: This issue passed the CQ dry run.

[csashi, 2017-03-31 18:59:59]

csashi@google.com changed reviewers:
+ jochen@chromium.org

[csashi, 2017-03-31 19:00:00]

Hi jochen@chromium.org: Please review changes in content/
Thanks,
-sashi.

[csashi, 2017-03-31 19:00:33]

csashi@google.com changed reviewers:
- jochen@chromium.org

[csashi, 2017-04-03 18:00:01]

On 2017/03/29 18:27:44, csashi wrote:
> Hi Carlos,
> Correction. I need your approval only for content/
> -sashi.

Hi Carlos,
Can you approve content/ or reassign to another reviewer?
Thanks,

[cpu_(ooo_6.6-7.5), 2017-04-03 23:21:20]

https://codereview.chromium.org/2762123006/diff/300001/content/public/common/...
File content/public/common/content_features.h (right):

https://codereview.chromium.org/2762123006/diff/300001/content/public/common/...
content/public/common/content_features.h:76: CONTENT_EXPORT extern const
base::Feature kNativeAndroidHistoryManager;
seems line 19 should be here?

[cpu_(ooo_6.6-7.5), 2017-04-03 23:23:59]

the rest lgtm

https://codereview.chromium.org/2762123006/diff/300001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_android.h
(right):

https://codereview.chromium.org/2762123006/diff/300001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.h:233:
jboolean IsAutofillPopupNode(JNIEnv* env,
lets indent 233 the same as the two above.

[csashi, 2017-04-03 23:35:09]

https://codereview.chromium.org/2762123006/diff/300001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager_android.h
(right):

https://codereview.chromium.org/2762123006/diff/300001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager_android.h:233:
jboolean IsAutofillPopupNode(JNIEnv* env,
On 2017/04/03 23:23:59, cpu wrote:
> lets indent 233 the same as the two above.

Done.

https://codereview.chromium.org/2762123006/diff/300001/content/public/common/...
File content/public/common/content_features.h (right):

https://codereview.chromium.org/2762123006/diff/300001/content/public/common/...
content/public/common/content_features.h:76: CONTENT_EXPORT extern const
base::Feature kNativeAndroidHistoryManager;
On 2017/04/03 23:21:19, cpu wrote:
> seems line 19 should be here?

Done.

[csashi, 2017-04-03 23:35:54]

The CQ bit was checked by csashi@google.com

[csashi, 2017-04-03 23:35:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, mathp@chromium.org, holte@chromium.org, cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/2762123006/#ps320001
(title: "Move kAndroidAutofillAccessibility feature under OS_ANDROID.")

[commit-bot: I haz the power, 2017-04-03 23:36:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 23:46:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 23:46:28]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[csashi, 2017-04-04 00:19:20]

The CQ bit was checked by csashi@google.com

[csashi, 2017-04-04 00:19:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
cpu@chromium.org, dmazzoni@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2762123006/#ps340001
(title: "Move kAndroidAutofillAccessibility feature under OS_ANDROID.")

[commit-bot: I haz the power, 2017-04-04 00:19:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 01:53:28]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1491265160010500,
"parent_rev": "4d80e56ccda893a12d9ef7c27547b7586ce582cf", "commit_rev":
"df20b103282ad2d7103cd5c078712893ba92e255"}

[commit-bot: I haz the power, 2017-04-04 01:54:18]

Description was changed from

==========
Adds accessibility navigation into autofill popup using right swipes. The
autofill popup does not have a corresponding AX node in the AX tree sent by
renderer to browser.  Hence, we create a proxy AX node that is "attached" to the
field that is displaying the popup for navigation.

This feature is currently disabled by default because we have to improve the
navigation as follows:

1. Navigate to autofill popup only on double-tap. Currently right swipe takes
user into the auto popup.

2. When user right swipes out of last suggestion in the popup, we must navigate
to the HTML element that is after the field hosting the popup.

3. When user presses back button, the user is able to edit the text field but
the accessibility navigation focus remains on the back button.

Please refer to
https://drive.google.com/a/google.com/file/d/0B-jTnU8uuAL9WV9RWXpvYTVQaDQ/vie...



BUG=627860
==========

to

==========
Adds accessibility navigation into autofill popup using right swipes. The
autofill popup does not have a corresponding AX node in the AX tree sent by
renderer to browser.  Hence, we create a proxy AX node that is "attached" to the
field that is displaying the popup for navigation.

This feature is currently disabled by default because we have to improve the
navigation as follows:

1. Navigate to autofill popup only on double-tap. Currently right swipe takes
user into the auto popup.

2. When user right swipes out of last suggestion in the popup, we must navigate
to the HTML element that is after the field hosting the popup.

3. When user presses back button, the user is able to edit the text field but
the accessibility navigation focus remains on the back button.

Please refer to
https://drive.google.com/a/google.com/file/d/0B-jTnU8uuAL9WV9RWXpvYTVQaDQ/vie...



BUG=627860

Review-Url: https://codereview.chromium.org/2762123006
Cr-Commit-Position: refs/heads/master@{#461610}
Committed:
https://chromium.googlesource.com/chromium/src/+/df20b103282ad2d7103cd5c07871...
==========

[commit-bot: I haz the power, 2017-04-04 01:54:19]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/df20b103282ad2d7103cd5c07871...