Resolve lint warnings related to ClickableViewAccessibility when a class overrides onTouch event.

Suppress lint warnings related to ClickableViewAccessibility while implementing
onTouchEvent()

The following lint warnings are reported when chrome shell is built:
1) org/chromium/content/browser/PopupZoomer.class
    Custom view org/chromium/content/browser/PopupZoomer overrides
    onTouchEvent but not performClick: ClickableViewAccessibility [warning]

2) org/chromium/content/browser/input/PopupTouchHandleDrawable.class
    Custom view org/chromium/content/browser/input/PopupTouchHandleDrawable
    overrides onTouchEvent but not performClick: ClickableViewAccessibility [warning]

3) org/chromium/chrome/shell/ChromeShellToolbar$7.class
    org/chromium/chrome/shell/ChromeShellToolbar$7#onTouch
    should call View#performClick when a click is detected:
    ClickableViewAccessibility [warning]

4) org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.class
    org/chromium/chrome/browser/appmenu/AppMenuButtonHelper#onTouch
    should call View#performClick when a click is detected:
    ClickableViewAccessibility [warning]

As performClick is not required to be overriden hence these warnings can be suppressed.

BUG=None.

Committed: https://crrev.com/813cb6cd34822072dd6b0bc1744c31ff4c0bac88
Cr-Commit-Position: refs/heads/master@{#300847}

[wajahat, 2014-10-09 12:13:03]

wajahat.s@samsung.com changed reviewers:
+ aurimas@chromium.org, nyquist@chromium.org, yfriedman@chromium.org

[wajahat, 2014-10-09 12:13:03]

Please review, Thanks!

[aurimas (slooooooooow), 2014-10-09 22:18:20]

aurimas@chromium.org changed reviewers:
+ newt@chromium.org

[aurimas (slooooooooow), 2014-10-09 22:18:21]

+newt: thoughts?

[newt (away), 2014-10-09 22:39:04]

These lint warnings are legitimate problems that need to be fixed. They prevent
accessibility (e.g. TalkBack) from working property.

If you could *fix* these warnings, that would be fantastic. It's a pretty easy
fix, as explained here:
http://developer.android.com/guide/topics/ui/accessibility/apps.html#custom-t...

[wajahat, 2014-10-13 14:53:22]

On 2014/10/09 22:39:04, newt wrote:
> These lint warnings are legitimate problems that need to be fixed. They
prevent
> accessibility (e.g. TalkBack) from working property.
> 
> If you could *fix* these warnings, that would be fantastic. It's a pretty easy
> fix, as explained here:
>
http://developer.android.com/guide/topics/ui/accessibility/apps.html#custom-t...

Thanks newt for the explanation, as suggested i have tried to fix these
and updated CL description, PTAL. Thanks!

[wajahat, 2014-10-15 04:46:19]

On 2014/10/13 14:53:22, wajahat wrote:
> On 2014/10/09 22:39:04, newt wrote:
> > These lint warnings are legitimate problems that need to be fixed. They
> prevent
> > accessibility (e.g. TalkBack) from working property.
> > 
> > If you could *fix* these warnings, that would be fantastic. It's a pretty
easy
> > fix, as explained here:
> >
>
http://developer.android.com/guide/topics/ui/accessibility/apps.html#custom-t...
> 
> Thanks newt for the explanation, as suggested i have tried to fix these
> and updated CL description, PTAL. Thanks!

ping ... @newt
PTAL! Thanks

[newt (away), 2014-10-20 22:20:53]

jdduke: Could you look at the comment in PopupTouchHandleDrawable?

https://codereview.chromium.org/640093003/diff/50001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java
(right):

https://codereview.chromium.org/640093003/diff/50001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java:92:
mMenuButton.performClick();
This isn't the correct usage of performClick(), but the app menu works fine in
accessibility mode event without this change, so I'd suppress the lint warning.

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:37:
class PopupZoomer extends View {
actually the PopupZoomer doesn't appear in accessibility mode, so it's probably
fine just to suppress the warning here

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:131:
performClick();
This isn't how performClick() should be used. See the comment "// Handle the
action for the custom click here" on
http://developer.android.com/guide/topics/ui/accessibility/apps.html

The point of performClick() is that you handle the click *inside of*
performClick() so that accessibility apps can call performClick() directly to
simulate a click on that View while bypassing onTouchEvent().

Implementing accessibility for this view looks rather complicated, however,
because we need to handle dragging, not clicking.

jdduke: Can we make this accessible?

[jdduke (slow), 2014-10-20 23:06:16]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2014-10-20 23:06:17]

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:131:
performClick();
On 2014/10/20 22:20:53, newt wrote:
> This isn't how performClick() should be used. See the comment "// Handle the
> action for the custom click here" on
> http://developer.android.com/guide/topics/ui/accessibility/apps.html
> 
> The point of performClick() is that you handle the click *inside of*
> performClick() so that accessibility apps can call performClick() directly to
> simulate a click on that View while bypassing onTouchEvent().
> 
> Implementing accessibility for this view looks rather complicated, however,
> because we need to handle dragging, not clicking.
> 
> jdduke: Can we make this accessible?

I'm not familiar (at all) with Android's accesibility conventions, but it seems
like this could be made accessible by having |performClick()| inject an
ACTION_DOWN + ACTION_UP motion event into |OnTouchEvent|, probably with a
location that is in the middle of the drawable. Or should that logic go in
|onClick()|?

The accessibility docs say it should be other way around (onTouchEvent should
call performClick), but that doesn't make a lot of sense here. If we implement
the logic I suggested, we could probably suppress the associated warnings.

[wajahat, 2014-10-21 14:34:56]

Comments: PopupZoomer doesn't appear in accessibility mode, 
app menu works fine in accessibility mode event without overriding accessibility
click event same with ChromeShellToolbar, fixing warning does not make any sense
in PopupTouchHandleDrawable case.

from the comments, i feel its ok to suppress warnings for now. 
PTAL! Thanks.

https://codereview.chromium.org/640093003/diff/50001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java
(right):

https://codereview.chromium.org/640093003/diff/50001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuButtonHelper.java:92:
mMenuButton.performClick();
On 2014/10/20 22:20:53, newt wrote:
> This isn't the correct usage of performClick(), but the app menu works fine in
> accessibility mode event without this change, so I'd suppress the lint
warning.

Done.

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java
(right):

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/PopupZoomer.java:37:
class PopupZoomer extends View {
On 2014/10/20 22:20:53, newt wrote:
> actually the PopupZoomer doesn't appear in accessibility mode, so it's
probably
> fine just to suppress the warning here

Done.

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java
(right):

https://codereview.chromium.org/640093003/diff/50001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/input/PopupTouchHandleDrawable.java:131:
performClick();
On 2014/10/20 23:06:17, jdduke (OOO until Oct 21) wrote:
> On 2014/10/20 22:20:53, newt wrote:
> > This isn't how performClick() should be used. See the comment "// Handle the
> > action for the custom click here" on
> > http://developer.android.com/guide/topics/ui/accessibility/apps.html
> > 
> > The point of performClick() is that you handle the click *inside of*
> > performClick() so that accessibility apps can call performClick() directly
to
> > simulate a click on that View while bypassing onTouchEvent().
> > 
> > Implementing accessibility for this view looks rather complicated, however,
> > because we need to handle dragging, not clicking.
> > 
> > jdduke: Can we make this accessible?
> 
> I'm not familiar (at all) with Android's accesibility conventions, but it
seems
> like this could be made accessible by having |performClick()| inject an
> ACTION_DOWN + ACTION_UP motion event into |OnTouchEvent|, probably with a
> location that is in the middle of the drawable. Or should that logic go in
> |onClick()|?
> 
> The accessibility docs say it should be other way around (onTouchEvent should
> call performClick), but that doesn't make a lot of sense here. If we implement
> the logic I suggested, we could probably suppress the associated warnings.

So going with the comments, it is ok to suppress the warnings for now

[jdduke (slow), 2014-10-21 15:22:09]

Changes to PopupTouchHandleDrawable.java look good.

[wajahat, 2014-10-22 05:41:15]

On 2014/10/21 15:22:09, jdduke wrote:
> Changes to PopupTouchHandleDrawable.java look good.

@newt comments ???

[newt (away), 2014-10-22 06:42:18]

lgtm

[wajahat, 2014-10-22 10:51:36]

On 2014/10/21 15:22:09, jdduke wrote:
> Changes to PopupTouchHandleDrawable.java look good.

@jdduke comments? can we land this ?

[jdduke (slow), 2014-10-22 12:56:52]

On 2014/10/22 10:51:36, wajahat wrote:
> On 2014/10/21 15:22:09, jdduke wrote:
> > Changes to PopupTouchHandleDrawable.java look good.
> 
> @jdduke comments? can we land this ?

Yes, lgtm.

[wajahat, 2014-10-22 13:01:14]

On 2014/10/22 12:56:52, jdduke wrote:
> On 2014/10/22 10:51:36, wajahat wrote:
> > On 2014/10/21 15:22:09, jdduke wrote:
> > > Changes to PopupTouchHandleDrawable.java look good.
> > 
> > @jdduke comments? can we land this ?
> 
> Yes, lgtm.

@Yaron/nyquist, Pl. comment for AppMenuButtonHelper.java &
ChromeShellToolbar.java ?

[Yaron, 2014-10-22 18:40:26]

rs lgtm (newt already reviewed)

[wajahat, 2014-10-23 05:34:09]

The CQ bit was checked by wajahat.s@samsung.com

[commit-bot: I haz the power, 2014-10-23 05:35:12]

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

[commit-bot: I haz the power, 2014-10-23 06:22:53]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2014-10-23 06:23:44]

Patchset 3 (id:??) landed as
https://crrev.com/813cb6cd34822072dd6b0bc1744c31ff4c0bac88
Cr-Commit-Position: refs/heads/master@{#300847}