Android shouldn't fire focus events when the WebView itself isn't focused

Android shouldn't fire focus events when the WebView itself isn't focused

BUG=648391
Committed: https://crrev.com/ad960c92315b64c662491152381b53b22c81a9c8
Cr-Commit-Position: refs/heads/master@{#426993}

[dmazzoni, 2016-10-14 17:55:53]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org

[David Tseng, 2016-10-14 21:41:04]

Do you need to do this for other events like text selection changes, live
regions, etc?

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

https://codereview.chromium.org/2416293002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:688:
moveAccessibilityFocusToId(id);
Don't you want to do this regardless? (i.e. sending focus is conditioned on the
manager having focus, but moving focus isn't).

https://codereview.chromium.org/2416293002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:754:
moveAccessibilityFocusToId(id);
Ditto

[dmazzoni, 2016-10-17 19:03:19]

Good idea regarding suppressing other events like
live regions and text selection when accessibility
focus is not in the web view, done.

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

https://codereview.chromium.org/2416293002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:688:
moveAccessibilityFocusToId(id);
On 2016/10/14 21:41:04, David Tseng wrote:
> Don't you want to do this regardless? (i.e. sending focus is conditioned on
the
> manager having focus, but moving focus isn't).

I don't think so. Accessibility focus in this particular
class represents actual accessibility focus on Android, so
if we choose to ignore a focus event from the web and not
fire the Android event, we shouldn't update our internal
state.

If we updated our internal state such that we thought
that view had accessibility focus already, then if the
user later tried to navigate to it by touch-exploring or
linear navigation we'd fail to fire the right event
because we'd think it already had accessibility focus.

[dmazzoni, 2016-10-17 19:03:23]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 19:04:06]

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, 2016-10-17 21:14:14]

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

[commit-bot: I haz the power, 2016-10-17 21:14:15]

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...)

[David Tseng, 2016-10-20 03:09:33]

lgtm

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

https://codereview.chromium.org/2416293002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:688:
moveAccessibilityFocusToId(id);
On 2016/10/17 19:03:19, dmazzoni wrote:
> On 2016/10/14 21:41:04, David Tseng wrote:
> > Don't you want to do this regardless? (i.e. sending focus is conditioned on
> the
> > manager having focus, but moving focus isn't).
> 
> I don't think so. Accessibility focus in this particular
> class represents actual accessibility focus on Android, so
> if we choose to ignore a focus event from the web and not
> fire the Android event, we shouldn't update our internal
> state.
> 
> If we updated our internal state such that we thought
> that view had accessibility focus already, then if the
> user later tried to navigate to it by touch-exploring or
> linear navigation we'd fail to fire the right event
> because we'd think it already had accessibility focus.

Ok. I haven't read through the details here, so will defer to you. If a webview
doesn't have focus, I would have expected setting the accessibility focused id
would have no effect on the outer android app and that when the webview gets
accessibility focus, the inner focused view would actually be the focused view.
When linear navigating, focus would be placed either at the beginning or end of
the webview depending on the direction of the linear navigation. When touch
exploring, the hit test would determine the accessibility focus.

[dmazzoni, 2016-10-22 21:50:09]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-10-22 21:50:15]

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, 2016-10-23 13:36:40]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-23 13:38:48]

Description was changed from

==========
Android shouldn't fire focus events when the WebView itself isn't focused

BUG=648391
==========

to

==========
Android shouldn't fire focus events when the WebView itself isn't focused

BUG=648391

Committed: https://crrev.com/ad960c92315b64c662491152381b53b22c81a9c8
Cr-Commit-Position: refs/heads/master@{#426993}
==========

[commit-bot: I haz the power, 2016-10-23 13:38:49]

Patchset 2 (id:??) landed as
https://crrev.com/ad960c92315b64c662491152381b53b22c81a9c8
Cr-Commit-Position: refs/heads/master@{#426993}