Fix Android accessibility focus not being drawn over transparent view.

Fix Android accessibility focus not being drawn over transparent view.

BUG=467550
Committed: https://crrev.com/9b015b54ff82e34658cc6e29c6a0ef4ff76302ea
Cr-Commit-Position: refs/heads/master@{#329484}

[dmazzoni, 2015-05-05 20:46:00]

dmazzoni@chromium.org changed reviewers:
+ newt@chromium.org, tedchoc@chromium.org

[Ted C, 2015-05-05 20:47:05]

lgtm

[dmazzoni, 2015-05-05 20:59:39]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-05-05 21:00:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126563003/1

[commit-bot: I haz the power, 2015-05-05 22:47:53]

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

[commit-bot: I haz the power, 2015-05-05 22:47:54]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[dmazzoni, 2015-05-07 04:44:40]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-05-07 04:45:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126563003/1

[commit-bot: I haz the power, 2015-05-07 06:23:12]

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

[commit-bot: I haz the power, 2015-05-07 06:23:13]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[dmazzoni, 2015-05-11 23:02:59]

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

[dmazzoni, 2015-05-11 23:02:59]

+aelias

Here's an alternate fix. This uses the same mechanism we
use to call setWillNotDraw(false) directly on the
SurfaceView when we need to enable animation and it
shouldn't affect WebView at all.

[aelias_OOO_until_Jul13, 2015-05-11 23:25:17]

New approach looks reasonable.

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:428: public void
onAnimationEnd(Animator animation) {
Is there something that guarantees this will get called when touch exploration
is disabled?

[dmazzoni, 2015-05-11 23:34:59]

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:428: public void
onAnimationEnd(Animator animation) {
On 2015/05/11 23:25:17, aelias wrote:
> Is there something that guarantees this will get called when touch exploration
> is disabled?

No, that's why I added the AccessibilityStateChangeListener above and it calls
setWillNotDraw explicitly.

[aelias_OOO_until_Jul13, 2015-05-11 23:36:34]

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:428: public void
onAnimationEnd(Animator animation) {
On 2015/05/11 at 23:34:58, dmazzoni wrote:
> On 2015/05/11 23:25:17, aelias wrote:
> > Is there something that guarantees this will get called when touch
exploration
> > is disabled?
> 
> No, that's why I added the AccessibilityStateChangeListener above and it calls
setWillNotDraw explicitly.

Right, but I'm concerned about who calls setWillNotDraw(true) again when the
accessibility stuff goes away.

[dmazzoni, 2015-05-11 23:41:01]

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:428: public void
onAnimationEnd(Animator animation) {
On 2015/05/11 23:36:34, aelias wrote:
> On 2015/05/11 at 23:34:58, dmazzoni wrote:
> > On 2015/05/11 23:25:17, aelias wrote:
> > > Is there something that guarantees this will get called when touch
> exploration
> > > is disabled?
> > 
> > No, that's why I added the AccessibilityStateChangeListener above and it
calls
> setWillNotDraw explicitly.
> 
> Right, but I'm concerned about who calls setWillNotDraw(true) again when the
> accessibility stuff goes away.

I could call setWillNotDraw(true) if mAnimationsOverContent.isEmpty(), I
believe, is that what you had in mind?

Note that even as it is, all it means is that someone who had accessibility
enabled and dynamically disabled it wouldn't get the power savings of
setWillNotDraw(true) until the next animation or the next restart.

[Ted C, 2015-05-12 00:10:12]

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:338:
mAccessibilityManager.addAccessibilityStateChangeListener(
you'd want to unregister this as well in destroy probably.

[aelias_OOO_until_Jul13, 2015-05-12 00:25:05]

Also, is there a way to write a test for this?

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:428: public void
onAnimationEnd(Animator animation) {
On 2015/05/11 at 23:41:01, dmazzoni wrote:
> I could call setWillNotDraw(true) if mAnimationsOverContent.isEmpty(), I
believe, is that what you had in mind?

Yes, that sounds reasonable.

[dmazzoni, 2015-05-12 17:09:00]

I did some more testing and this wasn't working when resuming TalkBack after it
was suspended, so I switched to using TouchExplorationStateChangeListener, which
is only available in KITKAT or later. On JB, we'll have to live with the state
only being checked once on startup.

Please check that I did the build version checking correctly, that this code is
safe to run on devices prior to KITKAT.

> Also, is there a way to write a test for this?

Not easy, as only privileged processes are able to modify the accessibility
system settings, so I don't think we could do that from our normal test suites.
I'd love to figure out a way around that in the future.

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:338:
mAccessibilityManager.addAccessibilityStateChangeListener(
On 2015/05/12 00:10:12, Ted C wrote:
> you'd want to unregister this as well in destroy probably.

Done.

https://codereview.chromium.org/1126563003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:428: public void
onAnimationEnd(Animator animation) {
On 2015/05/12 00:25:04, aelias wrote:
> On 2015/05/11 at 23:41:01, dmazzoni wrote:
> > I could call setWillNotDraw(true) if mAnimationsOverContent.isEmpty(), I
> believe, is that what you had in mind?
> 
> Yes, that sounds reasonable.

Done.

[aelias_OOO_until_Jul13, 2015-05-12 18:52:34]

OK, if it's hard to test then I won't insist on it.

https://codereview.chromium.org/1126563003/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1126563003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:52: if
(mIsTouchExplorationEnabled) {
Can we refactor this block out into a method called refreshWillNotDraw() that
checks all of these conditions, and every place that touches one of them calls
it?  Then it will be easy to add new conditions in the future without making a
mistake.

[dmazzoni, 2015-05-12 19:28:24]

https://codereview.chromium.org/1126563003/diff/40001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1126563003/diff/40001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:52: if
(mIsTouchExplorationEnabled) {
On 2015/05/12 18:52:33, aelias wrote:
> Can we refactor this block out into a method called refreshWillNotDraw() that
> checks all of these conditions, and every place that touches one of them calls
> it?  Then it will be easy to add new conditions in the future without making a
> mistake.

Good idea! I think this is cleaner. Manually testing now, but please
double-check my logic.

[dmazzoni, 2015-05-12 19:50:01]

Just to confirm, I just manually tested while explicitly logging the call to
setWillNotDraw. Enabling/disabling TalkBack works, and opening Find In Page
(which triggers a short animation) works. So I think the logic is right.

[aelias_OOO_until_Jul13, 2015-05-12 20:23:58]

lgtm

[Ted C, 2015-05-12 20:25:54]

lgtm

[dmazzoni, 2015-05-12 20:53:18]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-05-12 20:53:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126563003/60001

[commit-bot: I haz the power, 2015-05-12 21:01:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-12 21:02:33]

Patchset 4 (id:??) landed as
https://crrev.com/9b015b54ff82e34658cc6e29c6a0ef4ff76302ea
Cr-Commit-Position: refs/heads/master@{#329484}