New issue
Advanced search Search tips

Issue 280717 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug
Team-Accessibility

Blocking:
issue 157700
issue 276027
issue 278456
issue 278551



Sign in to add a comment

Chrome on Android should only enable accessibility for "real" accessibility services

Project Member Reported by dmazz...@chromium.org, Aug 28 2013

Issue description

Starting with M30, Chrome on Android now has native accessibility support. It's enabled only when an Android accessibility service is running.

Only a small fraction of Android users really need accessibility (<1%), but apparently a lot of Android users have an accessibility service running, because the accessibility APIs are useful for other services. Two examples are Lightflow and Pebble:

https://play.google.com/store/apps/details?id=com.rageconsulting.android.lightflowlite&hl=en
https://play.google.com/store/apps/details?id=com.getpebble.android&hl=en

In particular, Lightflow has millions of users - but Lightflow doesn't need Chrome's web accessibility support. Enabling accessibility support in Chrome on Android will add cpu and memory cost, and there are currently some crashing bugs as well. (The crashes should be fixed either way, but some cpu and memory cost is unavoidable.)

To fix this, we can defer enabling Chrome's accessibility support until an accessibility service tries to retrieve accessibility properties of the web content. That will still allow TalkBack and BrailleBack full access to Chrome, but apps like Lightflow and Pebble will no longer trigger it.

 
Blocking: chromium:157700
Blocking: chromium:276027
Blocking: chromium:278551
Labels: ReleaseBlock-Stable
I'm confident in this patch. Here's how I tested it:

Use this url from bug 278551 as a way to test a crash that happens whenever Chrome is in accessibility mode: http://m.digitalspy.co.uk/music/news/a509675/iggy-azalea-responds-to-angry-fans-after-reading-leeds-no-show-claims.html

Use "TalkBack", "Lightflow", and "DashClock Music Extension" as examples of accessibility services to test. TalkBack should enable Chrome's accessibility mode, but the other two should not.

My test strategy:

1. With master, accessibility off, url does not crash
     CONFIRMED
2. With master, Lightflow on, url crashes
     TAB CRASH
3. With patch, Lightflow on, url does not crash
     DOES NOT CRASH
4. With patch, TalkBack on, url crashes and accessibility still works
     TAB CRASH, ACCESSIBILITY STILL WORKS
5. With patch, DashClock on, url does not crash
     DOES NOT CRASH
6. With master, DashClock on, url crashes
     TAB CRASH

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 30 2013

------------------------------------------------------------------------
r220541 | dmazzoni@chromium.org | 2013-08-30T09:05:09.079562Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java?r1=220541&r2=220540&pathrev=220541

Defer enabling native accessibility until it's used.

We were enabling native accessibility if any accessibility service was
running, but many Android users have accessibility services other than
screen readers. Now we only enable native accessibility if
getAccessibilityNodeProvider is called on the web content.
Many popular Android apps that happen to be accessibility services will
no longer trigger Chrome's accessibility mode.

BUG= 280717 

Review URL: https://chromiumcodereview.appspot.com/23750002
------------------------------------------------------------------------
Labels: Merge-Requested
This change will reduce the impact of all of the related bugs listed above (157700, 276027, 278551) to noise. We should merge it to M30 immediately!

Comment 9 by kareng@google.com, Aug 30 2013

Labels: -Merge-Requested Merge-Approved
lol ok then! :)
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 3 2013

Labels: -Merge-Approved merge-merged-1599
------------------------------------------------------------------------
r220951 | dmazzoni@google.com | 2013-09-03T16:01:54.049704Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1599/src/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java?r1=220951&r2=220950&pathrev=220951

Merge 220541 "Defer enabling native accessibility until it's used."

> Defer enabling native accessibility until it's used.
> 
> We were enabling native accessibility if any accessibility service was
> running, but many Android users have accessibility services other than
> screen readers. Now we only enable native accessibility if
> getAccessibilityNodeProvider is called on the web content.
> Many popular Android apps that happen to be accessibility services will
> no longer trigger Chrome's accessibility mode.
> 
> BUG= 280717 
> 
> Review URL: https://chromiumcodereview.appspot.com/23750002

TBR=dmazzoni@chromium.org

Review URL: https://codereview.chromium.org/23503028
------------------------------------------------------------------------
Status: Fixed
Blocking: chromium:278456
If the fix here is expected to help with the crashes noted in Blocking bugs, can you please update all of them to be Fixed. QA can then reopen if we find the underlying crashes crop up again.  Thx !!
OK, they're all marked as fixed. There should be no user-visible bugs left.

I still have underlying bugs to fix (that can't be reproduced without a custom build) so I'm going to file new bugs for those to not confuse things.

Sign in to add a comment