Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15741009:
Native Android accessibility. (Closed)
|
Created:
7 years, 7 months ago by dmazzoni Modified:
7 years, 6 months ago CC:
chromium-reviews, James Su, yusukes+watch_chromium.org, hashimoto+watch_chromium.org, aboxhall+watch_chromium.org, yoshiki+watch_chromium.org, jam, penghuang+watch_chromium.org, yuzo+watch_chromium.org, davidbarr+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, ctguil+watch_chromium.org, zork+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionNative Android accessibility.
This is the final changelist that completes the initial implementation.
When accessibility is on and script injection is turned off or
unavailable, constructs a BrowserAccessibilityManager for the
ContentViewCore that provides a native accessibility implementation.
BUG=242953
R=benm@chromium.org, bulach@chromium.org, dtrainor@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207875
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208379
Patch Set 1 #Patch Set 2 : Candidate patch for public demo #
Total comments: 21
Patch Set 3 : Rebase #
Total comments: 121
Patch Set 4 : Address most reviewer feedback #
Total comments: 9
Patch Set 5 : Rebase #Patch Set 6 : Remove file not needed here #Patch Set 7 : Merge error #Patch Set 8 : Fix #
Total comments: 2
Patch Set 9 : Enable only when script injection is off, address other feedback #Patch Set 10 : Split RendererAccessibilityFocusOnly fix into separate changelist #
Total comments: 28
Patch Set 11 : Don't call super from AwContents #
Total comments: 2
Patch Set 12 : Now build objs from C++ code using Java helpers #
Total comments: 33
Patch Set 13 : Final feedback #Patch Set 14 : Rebase #Patch Set 15 : Fix rebase error #
Total comments: 4
Patch Set 16 : Rebase, update dispatchHoverEvent comment #Patch Set 17 : Fix swipe navigation, fix ICS #Patch Set 18 : Fix AwContents for ICS #
Total comments: 2
Patch Set 19 : Fix AwContents #
Total comments: 3
Patch Set 20 : Don't need BaseBrowserAccessibilityManager #Patch Set 21 : Added comment for SDK version check in C++ code #Patch Set 22 : Maybe we need BaseBrowserAccessibilityManager after all #
Total comments: 1
Patch Set 23 : Move AccessibilityNodeProvider to JellyBeanBrowserAccessibilityManager #Patch Set 24 : Unregister ContentObserver to fix leak #Patch Set 25 : Switch from using dispatchHoverEvent to onHoverEvent #Patch Set 26 : Rebase #Messages
Total messages: 52 (0 generated)
Here's a snapshot of what we have so far.
@dtrainor: we're going to split out a lot of the C++ infrastructure into a separate change, and we need to add support for dynamically choosing native or injected accessibility. However, please review this now, we're ready for feedback!
https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:337: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); This seems to be called despite accessibility being turned off, causing webview apps to crash: W/System.err( 2195): java.lang.IllegalStateException: Accessibility off. Did you forget to check that? W/System.err( 2195): at android.view.accessibility.AccessibilityManager.sendAccessibilityEvent(AccessibilityManager.java:238) W/System.err( 2195): at android.view.ViewRootImpl.requestSendAccessibilityEvent(ViewRootImpl.java:5785) W/System.err( 2195): at android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) W/System.err( 2195): at android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) W/System.err( 2195): at android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) W/System.err( 2195): at android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) W/System.err( 2195): at android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) W/System.err( 2195): at android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) W/System.err( 2195): at android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) W/System.err( 2195): at android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) W/System.err( 2195): at org.chromium.content.browser.accessibility.BrowserAccessibilityManager.sendAccessibilityEvent(BrowserAccessibilityManager.java:308) W/System.err( 2195): at org.chromium.content.browser.accessibility.BrowserAccessibilityManager.handleFocusChanged(BrowserAccessibilityManager.java:337)
On Tue, Jun 4, 2013 at 6:13 AM, <benm@chromium.org> wrote: > AccessibilityEvent.TYPE_VIEW_**ACCESSIBILITY_FOCUSED); > This seems to be called despite accessibility being turned off, causing > webview apps to crash: > Sorry, we had been testing with TalkBack on exclusively. Alice just added that check yesterday...we'll upload that shortly. Does it work if TalkBack is on?
Just trying with TalkBack switched on ... no crashes, but I can't get it to speak any web content. Using the old browser (with the new WebView) on cnn.com, it says "WebView" when I highlight the view, but then don't seem to be able to get it to say anything else. Is there a special gesture I need? Not very experienced with talk back (although I can get it to work in the old browser with old webview). Cheers On 4 June 2013 14:53, Dominic Mazzoni <dmazzoni@chromium.org> wrote: > On Tue, Jun 4, 2013 at 6:13 AM, <benm@chromium.org> wrote: > >> AccessibilityEvent.TYPE_VIEW_**ACCESSIBILITY_FOCUSED); >> This seems to be called despite accessibility being turned off, causing >> webview apps to crash: >> > > Sorry, we had been testing with TalkBack on exclusively. Alice just added > that check yesterday...we'll upload that shortly. Does it work if TalkBack > is on? > >
You shouldn't have to do anything for it to work, it should say the page name instead of WebView. I guess we should start building WebView so we can debug it and figure out what piece might be missing. Who do we need to ping to get us access to the clank mdb group? - Dominic On Tue, Jun 4, 2013 at 8:05 AM, Ben Murdoch <benm@chromium.org> wrote: > Just trying with TalkBack switched on ... no crashes, but I can't get it > to speak any web content. Using the old browser (with the new WebView) on > cnn.com, it says "WebView" when I highlight the view, but then don't seem > to be able to get it to say anything else. Is there a special gesture I > need? Not very experienced with talk back (although I can get it to work in > the old browser with old webview). > > Cheers > > > On 4 June 2013 14:53, Dominic Mazzoni <dmazzoni@chromium.org> wrote: > >> On Tue, Jun 4, 2013 at 6:13 AM, <benm@chromium.org> wrote: >> >>> AccessibilityEvent.TYPE_VIEW_**ACCESSIBILITY_FOCUSED); >>> This seems to be called despite accessibility being turned off, causing >>> webview apps to crash: >>> >> >> Sorry, we had been testing with TalkBack on exclusively. Alice just added >> that check yesterday...we'll upload that shortly. Does it work if TalkBack >> is on? >> >> >
On 4 June 2013 16:13, Dominic Mazzoni <dmazzoni@chromium.org> wrote: > You shouldn't have to do anything for it to work, it should say the page > name instead of WebView. > > I guess we should start building WebView so we can debug it and figure out > what piece might be missing. Who do we need to ping to get us access to the > clank mdb group? > I think that acw@ needs to approve MTVers, but joth or dtrainor should be able to confirm. I just noticed one thing - the WebView only proxies View.performAccessibilityAction, View.onInitializeAccessibilityEvent and View.onInitializeAccessibilityNodeInfo to ContentViewCore. I see you added View.dispatchHoverEvent and View.getAccessibilityNodeProvider to ContentView - would not forwarding those calls too stop it from working? > > - Dominic > > On Tue, Jun 4, 2013 at 8:05 AM, Ben Murdoch <benm@chromium.org> wrote: > >> Just trying with TalkBack switched on ... no crashes, but I can't get it >> to speak any web content. Using the old browser (with the new WebView) on >> cnn.com, it says "WebView" when I highlight the view, but then don't >> seem to be able to get it to say anything else. Is there a special gesture >> I need? Not very experienced with talk back (although I can get it to work >> in the old browser with old webview). >> >> Cheers >> >> >> On 4 June 2013 14:53, Dominic Mazzoni <dmazzoni@chromium.org> wrote: >> >>> On Tue, Jun 4, 2013 at 6:13 AM, <benm@chromium.org> wrote: >>> >>>> AccessibilityEvent.TYPE_VIEW_**ACCESSIBILITY_FOCUSED); >>>> This seems to be called despite accessibility being turned off, causing >>>> webview apps to crash: >>>> >>> >>> Sorry, we had been testing with TalkBack on exclusively. Alice just >>> added that check yesterday...we'll upload that shortly. Does it work if >>> TalkBack is on? >>> >>> >> >
On Tue, Jun 4, 2013 at 8:32 AM, Ben Murdoch <benm@chromium.org> wrote: > I just noticed one thing - the WebView only proxies View.performAccessibilityAction, > View.onInitializeAccessibilityEvent and View. > onInitializeAccessibilityNodeInfo to ContentViewCore. I see you added > View.dispatchHoverEvent and View.getAccessibilityNodeProvider to > ContentView - would not forwarding those calls too stop it from working? > Yes, that would totally explain it! Both are needed. - Dominic > > > >> >> - Dominic >> >> On Tue, Jun 4, 2013 at 8:05 AM, Ben Murdoch <benm@chromium.org> wrote: >> >>> Just trying with TalkBack switched on ... no crashes, but I can't get it >>> to speak any web content. Using the old browser (with the new WebView) on >>> cnn.com, it says "WebView" when I highlight the view, but then don't >>> seem to be able to get it to say anything else. Is there a special gesture >>> I need? Not very experienced with talk back (although I can get it to work >>> in the old browser with old webview). >>> >>> Cheers >>> >>> >>> On 4 June 2013 14:53, Dominic Mazzoni <dmazzoni@chromium.org> wrote: >>> >>>> On Tue, Jun 4, 2013 at 6:13 AM, <benm@chromium.org> wrote: >>>> >>>>> AccessibilityEvent.TYPE_VIEW_**ACCESSIBILITY_FOCUSED); >>>>> This seems to be called despite accessibility being turned off, causing >>>>> webview apps to crash: >>>>> >>>> >>>> Sorry, we had been testing with TalkBack on exclusively. Alice just >>>> added that check yesterday...we'll upload that shortly. Does it work if >>>> TalkBack is on? >>>> >>>> >>> >> >
OK, let me hook that up and try again. On 4 June 2013 16:41, Dominic Mazzoni <dmazzoni@chromium.org> wrote: > On Tue, Jun 4, 2013 at 8:32 AM, Ben Murdoch <benm@chromium.org> wrote: > >> I just noticed one thing - the WebView only proxies View.performAccessibilityAction, >> View.onInitializeAccessibilityEvent and View. >> onInitializeAccessibilityNodeInfo to ContentViewCore. I see you added >> View.dispatchHoverEvent and View.getAccessibilityNodeProvider to >> ContentView - would not forwarding those calls too stop it from working? >> > > Yes, that would totally explain it! Both are needed. > > - Dominic > > >> >> >> >>> >>> - Dominic >>> >>> On Tue, Jun 4, 2013 at 8:05 AM, Ben Murdoch <benm@chromium.org> wrote: >>> >>>> Just trying with TalkBack switched on ... no crashes, but I can't get >>>> it to speak any web content. Using the old browser (with the new WebView) >>>> on cnn.com, it says "WebView" when I highlight the view, but then >>>> don't seem to be able to get it to say anything else. Is there a special >>>> gesture I need? Not very experienced with talk back (although I can get it >>>> to work in the old browser with old webview). >>>> >>>> Cheers >>>> >>>> >>>> On 4 June 2013 14:53, Dominic Mazzoni <dmazzoni@chromium.org> wrote: >>>> >>>>> On Tue, Jun 4, 2013 at 6:13 AM, <benm@chromium.org> wrote: >>>>> >>>>>> AccessibilityEvent.TYPE_VIEW_**ACCESSIBILITY_FOCUSED); >>>>>> This seems to be called despite accessibility being turned off, >>>>>> causing >>>>>> webview apps to crash: >>>>>> >>>>> >>>>> Sorry, we had been testing with TalkBack on exclusively. Alice just >>>>> added that check yesterday...we'll upload that shortly. Does it work if >>>>> TalkBack is on? >>>>> >>>>> >>>> >>> >> >
https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:121: info.setEnabled(true); Gets set later, can drop this line. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:122: info.setVisibleToUser(true); Gets set later, can drop this line. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:135: int childCount = BrowserAccessibility.nativeGetChildCountJNI(nativeNode); Move closer to loop that uses this var. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:186: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); Views should never send this event or manage accessibility focus state except in response to ACTION_ACCESSIBILITY_FOCUS. Instead, keep track of the currently hovered id and send TYPE_VIEW_HOVER_ENTER and _EXIT pairs when the id changes (or the user leaves the view completely). The accessibility service will handle focus placement. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:197: public void focusAndHideOrShowKeyboardIfNecessary(int virtualViewId) { The keyboard should only be shown when an item gets system focus, and should not need special handling for accessibility. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:244: sendAccessibilityEvent(virtualViewId, What happens when a user is moving focus with a keyboard? Does the web content still send the correct events? https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:329: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); Views should not manually move accessibility focus. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:337: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); If focus here means system focus, please only send a VIEW_FOCUSED event and let the service handle accessibility focus placement. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:354: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_TEXT_CHANGED); Does this also raise a TEXT_SELECTION_CHANGED event from the cursor position moving?
On 2013/06/04 15:45:31, benm wrote: > OK, let me hook that up and try again. > Hm, this doesn't seem to be quite enough. It doesn't say "WebView" anymore, but doesn't say anything else either :-/
Initial nits. I'm not an owner on anything but chrome/android and I don't know enough about the underlying chromium accessibility system to really be a reviewer for that, but I gave the initial review on what I saw (mostly Java). https://chromiumcodereview.appspot.com/15741009/diff/6001/build/android/pylib... File build/android/pylib/gtest/test_runner.py (left): https://chromiumcodereview.appspot.com/15741009/diff/6001/build/android/pylib... build/android/pylib/gtest/test_runner.py:125: 'content/test/data/content-disposition-inline.html', What happened to these for content_browsertests? Do we want both sets of test data? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/acc... File content/browser/accessibility/browser_accessibility_manager_android.cc (right): https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/acc... content/browser/accessibility/browser_accessibility_manager_android.cc:151: void BrowserAccessibilityManagerAndroid::FuzzyHitTestImpl( Could this also be static? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/acc... content/browser/accessibility/browser_accessibility_manager_android.cc:185: static int clamp(int val, int min, int max) { anonymous namespace instead of static? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/acc... content/browser/accessibility/browser_accessibility_manager_android.cc:186: if (val < min) Since you're std in most places: std::min(std::max(val, min), max)? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/acc... content/browser/accessibility/browser_accessibility_manager_android.cc:202: return std::sqrt(dx * dx + dy * dy); You could always just track MinDistanceSquared instead of MinDistance and never do the expensive sqrt. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/acc... File content/browser/accessibility/dump_accessibility_tree_browsertest.cc (right): https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/acc... content/browser/accessibility/dump_accessibility_tree_browsertest.cc:150: LOG(ERROR) << "HTML CONTENTS:\n" << html_contents; Remove? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/ren... File content/browser/renderer_host/render_view_host_impl.cc (right): https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/ren... content/browser/renderer_host/render_view_host_impl.cc:1914: std::map<AccessibilityNodeData::StringAttribute, string16>::const_iterator iter; line too long. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/ren... content/browser/renderer_host/render_view_host_impl.cc:1919: if (doc_url == ASCIIToUTF16("about:blank")) I feel like about:blank a constant somewhere. I don't remember the place though. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/ren... content/browser/renderer_host/render_view_host_impl.cc:1920: is_about_blank = true; break here? Could also just put the logic in here and break. No need to save the bool? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/ren... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/ren... content/browser/renderer_host/render_widget_host_view_android.cc:818: void RenderWidgetHostViewAndroid::OnAccessibilityNotifications( What causes this event to be thrown? Ideally we'll *never* get these events unless accessibility is turned on at the Android Framework level. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/browser/ren... content/browser/renderer_host/render_widget_host_view_android.cc:834: LOG(INFO) << "SetAccessibilityFocus"; Remove https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:663: //mAccessibilityInjector.addOrRemoveAccessibilityApisIfNecessary(); Uncomment. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1115: //mAccessibilityInjector.addOrRemoveAccessibilityApisIfNecessary(); Uncomment. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1677: if (mBrowserAccessibilityManager != null) Is this a proper check to tell if accessibility is enabled and we need to pass the event to mBrowserAccessibilityManager? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1678: return mBrowserAccessibilityManager.dispatchHoverEvent(event); { } around if blocks in Java (unless you can fit the entire thing on one line ie: if (a) b(); https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1680: return false; // TODO: call super on ContentView's dispatchHoverEvent? Probably, since it would have hit the root view in the first place before we overrode it here. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2217: mBrowserAccessibilityManager.notifyFrameInfoInitialized(); null check? You do it above. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2620: /* Uncomment before landing. IMO Add a boolean above and key off of that everywhere. Just leave it set to a certain value and we can add code for setting it later. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2628: public void setBrowserAccessibilityManager(BrowserAccessibilityManager manager) { Javadoc. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2632: public BrowserAccessibilityManager getBrowserAccessibilityManager() { Javadoc https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... File content/public/android/java/src/org/chromium/content/browser/accessibility/AccessibilityInjector.java (right): https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/AccessibilityInjector.java:232: // injectAccessibilityScriptIntoPage(); Uncomment. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java (right): https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java:17: public class BrowserAccessibility { Add a private constructor if it can't be initialized. Also generally we don't expose the native*** method as public, but rather a Java one that delegates to the native. Ideally we could then write unit tests and mock the native component out. It might make more sense to make those native methods private, and have something like: populateAccessibilityNodeInfo(AccessibilityNodeInfo info, ContentViewCore core, int nodePtr)? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:45: private boolean mUserHasTouchExplored = false; Don't need "= false;" https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:50: public static BrowserAccessibilityManager create(int nativeBrowserAccessibilityManagerAndroid, Javadoc or make private https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:51: ContentViewCore contentViewCore) { Java function format just indents by 8 on the next line. Don't have to make parameters line up at this point. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:52: Log.i("AX", "BrowserAccessibilityManager java create"); Remove? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:57: BrowserAccessibilityManager(int nativeBrowserAccessibilityManagerAndroid, Javadoc or just make private https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:76: public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) { Javadoc https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:77: int rootId = nativeGetRootId(mNativeObj); Check if mNativeObj == 0 before using it everywhere (maybe at the top of all of the methods). Just to handle the java object potentially living a bit longer than the native component. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:79: virtualViewId = rootId; { } around this and all one line if statements. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:94: Rect boundsInParent = (Rect)BrowserAccessibility.nativeGetRectInParentJNI(nativeNode); space after (Rect) https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:97: boundsInParent.offset(0, (int)mRenderCoordinates.getContentOffsetYPix()); space after (int) https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:100: Rect rect = (Rect)BrowserAccessibility.nativeGetAbsoluteRectJNI(nativeNode); Space after (Rect) https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:103: rect.offset(-(int)mRenderCoordinates.getScrollX(), Space after (int) for all of this. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:120: info.setContentDescription(BrowserAccessibility.nativeGetNameJNI(nativeNode)); Maybe group all of these initial set* parameters in one place up top and just have the two rect setting components down below. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:135: int childCount = BrowserAccessibility.nativeGetChildCountJNI(nativeNode); Move this down below near the for loop. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:164: public List<AccessibilityNodeInfo> findAccessibilityNodeInfosByText(String text, Javadoc https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:165: int virtualViewId) { Just indent by 8. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:169: public boolean dispatchHoverEvent(MotionEvent event) { Javadoc https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:177: int cssX = (int)(mRenderCoordinates.fromPixToLocalCss(x) + mRenderCoordinates.getScrollX()); space after (int) https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:190: // if necessary. TODO: figure out what happens when there's an ACTION_HOVER_EXIT format of todo's: // TODO(dmazzoni): ... https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:197: public void focusAndHideOrShowKeyboardIfNecessary(int virtualViewId) { Javadoc. I'm also surprised the keyboard doesn't show/hide automatically when we focus the actual node in the Blink DOM. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:206: (InputMethodManager)mContentViewCore.getContext().getSystemService( Space after (InputMethodManager). Indent +8 instead of +4. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:210: InputMethodManager.HIDE_IMPLICIT_ONLY); Just indent +8 instead of lining this up. Same for all methods that wrap. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:229: public boolean performAction(int virtualViewId, int action, Bundle arguments) { javadoc https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:231: int nativeRootNode = nativeGetNativeNodeById(mNativeObj, rootId); Can you just have a nativeGetRootNode()? Would save you the JNI which is actually somewhat expensive. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:239: case AccessibilityNodeInfo.ACTION_ACCESSIBILITY_FOCUS: Fix switch statement indent. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:250: /* sendAccessibilityEvent(virtualViewId, ? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:273: event.setClassName(BrowserAccessibility.nativeGetClassNameJNI(nativeNode)); This is going to be expensive :(. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:290: nativeNode)); indent +4 more. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:307: mContentViewCore.getContainerView().invalidate(); Why do we need to invalidate here? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:309: mView, event); +4 indent, should fit on line above though. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:312: public void notifyFrameInfoInitialized() { javadoc https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:314: return; put return on same line as if if it fits, etc. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:319: sendAccessibilityEvent(mA11yFocusId, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); Line too long. https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:329: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); Will this drag focus from anywhere else in the app (the UI) when the page finishes loading? https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:337: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); Also does focusChanged mean focused? On 2013/06/04 13:13:12, benm wrote: > This seems to be called despite accessibility being turned off, causing webview > apps to crash: > > W/System.err( 2195): java.lang.IllegalStateException: Accessibility off. Did you > forget to check that? > W/System.err( 2195): at > android.view.accessibility.AccessibilityManager.sendAccessibilityEvent(AccessibilityManager.java:238) > W/System.err( 2195): at > android.view.ViewRootImpl.requestSendAccessibilityEvent(ViewRootImpl.java:5785) > W/System.err( 2195): at > android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) > W/System.err( 2195): at > android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) > W/System.err( 2195): at > android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) > W/System.err( 2195): at > android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) > W/System.err( 2195): at > android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) > W/System.err( 2195): at > android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) > W/System.err( 2195): at > android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) > W/System.err( 2195): at > android.view.ViewGroup.requestSendAccessibilityEvent(ViewGroup.java:705) > W/System.err( 2195): at > org.chromium.content.browser.accessibility.BrowserAccessibilityManager.sendAccessibilityEvent(BrowserAccessibilityManager.java:308) > W/System.err( 2195): at > org.chromium.content.browser.accessibility.BrowserAccessibilityManager.handleFocusChanged(BrowserAccessibilityManager.java:337) https://chromiumcodereview.appspot.com/15741009/diff/6001/content/public/andr... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:378: private void announceObjectShow(int id, boolean force_assertive) { forceAssertive
https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2639: public AccessibilityNodeProvider getAccessibilityNodeProvider(View host) { Do we need the |host| param here?
On 2013/06/04 18:16:50, benm wrote:
> On 2013/06/04 15:45:31, benm wrote:
> > OK, let me hook that up and try again.
> >
>
> Hm, this doesn't seem to be quite enough. It doesn't say "WebView" anymore,
but
> doesn't say anything else either :-/
Seems to be a problem with merged-threads-mode, I'm looking into it. In the
meantime, if you can incorporate the following diff to your patch, I'll take
care of downstream WebView plumbing.
diff --git
a/android_webview/java/src/org/chromium/android_webview/AwContents.java
b/android_webview/java/src/org/chromium/android_webview/AwContents.java
index b3be1b8..f809b14 100644
--- a/android_webview/java/src/org/chromium/android_webview/AwContents.java
+++ b/android_webview/java/src/org/chromium/android_webview/AwContents.java
@@ -24,6 +24,7 @@ import android.view.ViewGroup;
import android.view.ViewTreeObserver;
import android.view.accessibility.AccessibilityEvent;
import android.view.accessibility.AccessibilityNodeInfo;
+import android.view.accessibility.AccessibilityNodeProvider;
import android.view.inputmethod.EditorInfo;
import android.view.inputmethod.InputConnection;
import android.webkit.GeolocationPermissions;
@@ -832,6 +833,13 @@ public class AwContents {
}
/**
+ * @see android.webkit.WebView#dispatchHoverEvent(KeyEvent)
+ */
+ public boolean dispatchHoverEvent(MotionEvent event) {
+ return mContentViewCore.dispatchHoverEvent(event);
+ }
+
+ /**
* Clears the resource cache. Note that the cache is per-application, so
this will clear the
* cache for all WebViews used.
*
@@ -1221,6 +1229,13 @@ public class AwContents {
}
/**
+ * @see android.webkit.WebView#getAccessibilityNodeProvider()
+ */
+ public AccessibilityNodeProvider getAccessibilityNodeProvider() {
+ return mContentViewCore.getAccessibilityNodeProvider(mContainerView);
+ }
+
+ /**
* @see
android.webkit.WebView#onInitializeAccessibilityNodeInfo(AccessibilityNodeInfo)
*/
public void onInitializeAccessibilityNodeInfo(AccessibilityNodeInfo info) {
We've addressed almost all feedback. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:121: info.setEnabled(true); On 2013/06/04 17:50:22, alanv wrote: > Gets set later, can drop this line. Done. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:122: info.setVisibleToUser(true); On 2013/06/04 17:50:22, alanv wrote: > Gets set later, can drop this line. Done. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:135: int childCount = BrowserAccessibility.nativeGetChildCountJNI(nativeNode); On 2013/06/04 17:50:22, alanv wrote: > Move closer to loop that uses this var. Done. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:197: public void focusAndHideOrShowKeyboardIfNecessary(int virtualViewId) { On 2013/06/04 17:50:22, alanv wrote: > The keyboard should only be shown when an item gets system focus, and should not > need special handling for accessibility. We added support for an "auto focus mode", which needs this code - but we're leaving it disabled for now, to keep things simple. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:244: sendAccessibilityEvent(virtualViewId, On 2013/06/04 17:50:22, alanv wrote: > What happens when a user is moving focus with a keyboard? Does the web content > still send the correct events? Yes, those are the handleXXX methods below. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:329: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); On 2013/06/04 17:50:22, alanv wrote: > Views should not manually move accessibility focus. I think this can be a good experience - we specifically don't do anything if the user has already explored since initiating loading of the page. What if we send TYPE_VIEW_FOCUSED instead? https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:337: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); On 2013/06/04 17:50:22, alanv wrote: > If focus here means system focus, please only send a VIEW_FOCUSED event and let > the service handle accessibility focus placement. Done. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:354: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_TEXT_CHANGED); On 2013/06/04 17:50:22, alanv wrote: > Does this also raise a TEXT_SELECTION_CHANGED event from the cursor position > moving? If only one changes, it definitely does the right thing now. If both change, which one implies which, or is it okay to send both for a single user action? https://codereview.chromium.org/15741009/diff/6001/build/android/pylib/gtest/... File build/android/pylib/gtest/test_runner.py (left): https://codereview.chromium.org/15741009/diff/6001/build/android/pylib/gtest/... build/android/pylib/gtest/test_runner.py:125: 'content/test/data/content-disposition-inline.html', On 2013/06/04 20:27:18, David Trainor wrote: > What happened to these for content_browsertests? Do we want both sets of test > data? Sorry, that was for my own testing. Reverted. https://codereview.chromium.org/15741009/diff/6001/content/browser/accessibil... File content/browser/accessibility/browser_accessibility_manager_android.cc (right): https://codereview.chromium.org/15741009/diff/6001/content/browser/accessibil... content/browser/accessibility/browser_accessibility_manager_android.cc:151: void BrowserAccessibilityManagerAndroid::FuzzyHitTestImpl( On 2013/06/04 20:27:18, David Trainor wrote: > Could this also be static? Done. https://codereview.chromium.org/15741009/diff/6001/content/browser/accessibil... content/browser/accessibility/browser_accessibility_manager_android.cc:185: static int clamp(int val, int min, int max) { On 2013/06/04 20:27:18, David Trainor wrote: > anonymous namespace instead of static? Done. https://codereview.chromium.org/15741009/diff/6001/content/browser/accessibil... content/browser/accessibility/browser_accessibility_manager_android.cc:186: if (val < min) On 2013/06/04 20:27:18, David Trainor wrote: > Since you're std in most places: std::min(std::max(val, min), max)? Done. https://codereview.chromium.org/15741009/diff/6001/content/browser/accessibil... content/browser/accessibility/browser_accessibility_manager_android.cc:186: if (val < min) On 2013/06/04 20:27:18, David Trainor wrote: > Since you're std in most places: std::min(std::max(val, min), max)? Done. https://codereview.chromium.org/15741009/diff/6001/content/browser/accessibil... content/browser/accessibility/browser_accessibility_manager_android.cc:202: return std::sqrt(dx * dx + dy * dy); On 2013/06/04 20:27:18, David Trainor wrote: > You could always just track MinDistanceSquared instead of MinDistance and never > do the expensive sqrt. Done. https://codereview.chromium.org/15741009/diff/6001/content/browser/accessibil... File content/browser/accessibility/dump_accessibility_tree_browsertest.cc (right): https://codereview.chromium.org/15741009/diff/6001/content/browser/accessibil... content/browser/accessibility/dump_accessibility_tree_browsertest.cc:150: LOG(ERROR) << "HTML CONTENTS:\n" << html_contents; On 2013/06/04 20:27:18, David Trainor wrote: > Remove? Done. https://codereview.chromium.org/15741009/diff/6001/content/browser/renderer_h... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/15741009/diff/6001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_impl.cc:1914: std::map<AccessibilityNodeData::StringAttribute, string16>::const_iterator iter; On 2013/06/04 20:27:18, David Trainor wrote: > line too long. Done. https://codereview.chromium.org/15741009/diff/6001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_impl.cc:1919: if (doc_url == ASCIIToUTF16("about:blank")) On 2013/06/04 20:27:18, David Trainor wrote: > I feel like about:blank a constant somewhere. I don't remember the place > though. Done. https://codereview.chromium.org/15741009/diff/6001/content/browser/renderer_h... content/browser/renderer_host/render_view_host_impl.cc:1920: is_about_blank = true; On 2013/06/04 20:27:18, David Trainor wrote: > break here? Could also just put the logic in here and break. No need to save > the bool? We'd need to break out of one for loop and continue from the other, which I'm not sure would be more readable. https://codereview.chromium.org/15741009/diff/6001/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/15741009/diff/6001/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_android.cc:818: void RenderWidgetHostViewAndroid::OnAccessibilityNotifications( On 2013/06/04 20:27:18, David Trainor wrote: > What causes this event to be thrown? Ideally we'll *never* get these events > unless accessibility is turned on at the Android Framework level. That's correct - they won't even be sent until the renderer is told to start sending accessibility notifications. https://codereview.chromium.org/15741009/diff/6001/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_android.cc:834: LOG(INFO) << "SetAccessibilityFocus"; On 2013/06/04 20:27:18, David Trainor wrote: > Remove Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:663: //mAccessibilityInjector.addOrRemoveAccessibilityApisIfNecessary(); On 2013/06/04 20:27:18, David Trainor wrote: > Uncomment. Done. Alice is fixing it so that either native accessibility is enabled or the injector runs, never both. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1115: //mAccessibilityInjector.addOrRemoveAccessibilityApisIfNecessary(); On 2013/06/04 20:27:18, David Trainor wrote: > Uncomment. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1677: if (mBrowserAccessibilityManager != null) On 2013/06/04 20:27:18, David Trainor wrote: > Is this a proper check to tell if accessibility is enabled and we need to pass > the event to mBrowserAccessibilityManager? I think this one is fine. We should fix the code so that BrowserAccessibilityManager is only created when it should be. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1680: return false; // TODO: call super on ContentView's dispatchHoverEvent? On 2013/06/04 20:27:18, David Trainor wrote: > Probably, since it would have hit the root view in the first place before we > overrode it here. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2217: mBrowserAccessibilityManager.notifyFrameInfoInitialized(); On 2013/06/04 20:27:18, David Trainor wrote: > null check? You do it above. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2620: /* On 2013/06/04 20:27:18, David Trainor wrote: > Uncomment before landing. > > IMO Add a boolean above and key off of that everywhere. Just leave it set to a > certain value and we can add code for setting it later. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2628: public void setBrowserAccessibilityManager(BrowserAccessibilityManager manager) { On 2013/06/04 20:27:18, David Trainor wrote: > Javadoc. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2632: public BrowserAccessibilityManager getBrowserAccessibilityManager() { On 2013/06/04 20:27:18, David Trainor wrote: > Javadoc Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2639: public AccessibilityNodeProvider getAccessibilityNodeProvider(View host) { On 2013/06/05 10:52:40, benm wrote: > Do we need the |host| param here? Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/accessibility/AccessibilityInjector.java (right): https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/AccessibilityInjector.java:232: // injectAccessibilityScriptIntoPage(); On 2013/06/04 20:27:18, David Trainor wrote: > Uncomment. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java (right): https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java:17: public class BrowserAccessibility { On 2013/06/04 20:27:18, David Trainor wrote: > Add a private constructor if it can't be initialized. Also generally we don't > expose the native*** method as public, but rather a Java one that delegates to > the native. Ideally we could then write unit tests and mock the native > component out. > > It might make more sense to make those native methods private, and have > something like: > > populateAccessibilityNodeInfo(AccessibilityNodeInfo info, ContentViewCore core, > int nodePtr)? Private constructor added. I understand about exposing not native methods as public, but I don't think it makes sense here - this class is never instantiated and *only* exposes native methods, so the extra indirection doesn't really buy anything. We could move something like populateAccessibilityNodeInfo here, but there are several methods in BrowserAccessibilityManager.java that require calling some of these methods. I think it's cleaner to just have all of the logic in BrowserAccessibilityManager.java where you can trace it through all at once. What do you think? If you still feel strongly I can change it. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:45: private boolean mUserHasTouchExplored = false; On 2013/06/04 20:27:18, David Trainor wrote: > Don't need "= false;" Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:50: public static BrowserAccessibilityManager create(int nativeBrowserAccessibilityManagerAndroid, On 2013/06/04 20:27:18, David Trainor wrote: > Javadoc or make private Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:51: ContentViewCore contentViewCore) { On 2013/06/04 20:27:18, David Trainor wrote: > Java function format just indents by 8 on the next line. Don't have to make > parameters line up at this point. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:52: Log.i("AX", "BrowserAccessibilityManager java create"); On 2013/06/04 20:27:18, David Trainor wrote: > Remove? Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:57: BrowserAccessibilityManager(int nativeBrowserAccessibilityManagerAndroid, On 2013/06/04 20:27:18, David Trainor wrote: > Javadoc or just make private Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:76: public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) { On 2013/06/04 20:27:18, David Trainor wrote: > Javadoc This one is an interface implementation - I'm assuming @inheritDoc is fine? https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:77: int rootId = nativeGetRootId(mNativeObj); On 2013/06/04 20:27:18, David Trainor wrote: > Check if mNativeObj == 0 before using it everywhere (maybe at the top of all of > the methods). Just to handle the java object potentially living a bit longer > than the native component. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:79: virtualViewId = rootId; On 2013/06/04 20:27:18, David Trainor wrote: > { } around this and all one line if statements. Fixed throughout. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:94: Rect boundsInParent = (Rect)BrowserAccessibility.nativeGetRectInParentJNI(nativeNode); On 2013/06/04 20:27:18, David Trainor wrote: > space after (Rect) Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:97: boundsInParent.offset(0, (int)mRenderCoordinates.getContentOffsetYPix()); On 2013/06/04 20:27:18, David Trainor wrote: > space after (int) Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:100: Rect rect = (Rect)BrowserAccessibility.nativeGetAbsoluteRectJNI(nativeNode); On 2013/06/04 20:27:18, David Trainor wrote: > Space after (Rect) Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:103: rect.offset(-(int)mRenderCoordinates.getScrollX(), On 2013/06/04 20:27:18, David Trainor wrote: > Space after (int) for all of this. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:120: info.setContentDescription(BrowserAccessibility.nativeGetNameJNI(nativeNode)); On 2013/06/04 20:27:18, David Trainor wrote: > Maybe group all of these initial set* parameters in one place up top and just > have the two rect setting components down below. Good idea. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:135: int childCount = BrowserAccessibility.nativeGetChildCountJNI(nativeNode); On 2013/06/04 20:27:18, David Trainor wrote: > Move this down below near the for loop. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:164: public List<AccessibilityNodeInfo> findAccessibilityNodeInfosByText(String text, On 2013/06/04 20:27:18, David Trainor wrote: > Javadoc Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:165: int virtualViewId) { On 2013/06/04 20:27:18, David Trainor wrote: > Just indent by 8. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:169: public boolean dispatchHoverEvent(MotionEvent event) { On 2013/06/04 20:27:18, David Trainor wrote: > Javadoc Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:177: int cssX = (int)(mRenderCoordinates.fromPixToLocalCss(x) + mRenderCoordinates.getScrollX()); On 2013/06/04 20:27:18, David Trainor wrote: > space after (int) Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:177: int cssX = (int)(mRenderCoordinates.fromPixToLocalCss(x) + mRenderCoordinates.getScrollX()); On 2013/06/04 20:27:18, David Trainor wrote: > space after (int) Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:190: // if necessary. TODO: figure out what happens when there's an ACTION_HOVER_EXIT On 2013/06/04 20:27:18, David Trainor wrote: > format of todo's: > > // TODO(dmazzoni): ... Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:197: public void focusAndHideOrShowKeyboardIfNecessary(int virtualViewId) { On 2013/06/04 20:27:18, David Trainor wrote: > Javadoc. I'm also surprised the keyboard doesn't show/hide automatically when > we focus the actual node in the Blink DOM. I think it does show normally - auto-hiding was the main purpose of this method but I think there may have been an occasional corner case where we wanted to force-show it. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:206: (InputMethodManager)mContentViewCore.getContext().getSystemService( On 2013/06/04 20:27:18, David Trainor wrote: > Space after (InputMethodManager). Indent +8 instead of +4. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:210: InputMethodManager.HIDE_IMPLICIT_ONLY); On 2013/06/04 20:27:18, David Trainor wrote: > Just indent +8 instead of lining this up. Same for all methods that wrap. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:229: public boolean performAction(int virtualViewId, int action, Bundle arguments) { On 2013/06/04 20:27:18, David Trainor wrote: > javadoc Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:231: int nativeRootNode = nativeGetNativeNodeById(mNativeObj, rootId); On 2013/06/04 20:27:18, David Trainor wrote: > Can you just have a nativeGetRootNode()? Would save you the JNI which is > actually somewhat expensive. Is there any way we could do this and avoid it having the same function signature as nativeGetRootId? While developing this, having some functions that return nodes and some that return ids made it really easy to introduce bugs, so I'd prefer to make it hard to do so accidentally. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:239: case AccessibilityNodeInfo.ACTION_ACCESSIBILITY_FOCUS: On 2013/06/04 20:27:18, David Trainor wrote: > Fix switch statement indent. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:250: /* sendAccessibilityEvent(virtualViewId, On 2013/06/04 20:27:18, David Trainor wrote: > ? Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:273: event.setClassName(BrowserAccessibility.nativeGetClassNameJNI(nativeNode)); On 2013/06/04 20:27:18, David Trainor wrote: > This is going to be expensive :(. Surprisingly it's okay in practice. Probably the easiest way to cut down on the number of JNI calls would be for Java to initiate, then have C++ call a big uber-method to set a lot of properties at once. It'd help performance slightly but hurt readability. Let's stick with this for a while and then improve performance later once everything else is stable. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:290: nativeNode)); On 2013/06/04 20:27:18, David Trainor wrote: > indent +4 more. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:307: mContentViewCore.getContainerView().invalidate(); On 2013/06/04 20:27:18, David Trainor wrote: > Why do we need to invalidate here? I added a comment. It's a workaround for a known Android framework bug. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:309: mView, event); On 2013/06/04 20:27:18, David Trainor wrote: > +4 indent, should fit on line above though. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:312: public void notifyFrameInfoInitialized() { On 2013/06/04 20:27:18, David Trainor wrote: > javadoc Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:314: return; On 2013/06/04 20:27:18, David Trainor wrote: > put return on same line as if if it fits, etc. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:319: sendAccessibilityEvent(mA11yFocusId, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); On 2013/06/04 20:27:18, David Trainor wrote: > Line too long. Done. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:329: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); On 2013/06/04 20:27:18, David Trainor wrote: > Will this drag focus from anywhere else in the app (the UI) when the page > finishes loading? Currently yes. We think this is usually good - but we might want to not do that if the user already focused something else in the meantime. What do you think? https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:337: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); On 2013/06/04 13:13:12, benm wrote: > This seems to be called despite accessibility being turned off, causing webview > apps to crash: sendAccessibilityEvent now checks. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:337: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); On 2013/06/04 20:27:18, David Trainor wrote: > Also does focusChanged mean focused? It means that page focus changed to the node at the given id. https://codereview.chromium.org/15741009/diff/6001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:378: private void announceObjectShow(int id, boolean force_assertive) { On 2013/06/04 20:27:18, David Trainor wrote: > forceAssertive Done.
https://codereview.chromium.org/15741009/diff/27001/content/browser/accessibi... File content/browser/accessibility/accessibility_tree_formatter_android.cc (right): https://codereview.chromium.org/15741009/diff/27001/content/browser/accessibi... content/browser/accessibility/accessibility_tree_formatter_android.cc:23: const char* BOOL_ATTRIBUTES[] = { should these be in an anonymous namespace? https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentView.java:15: import android.util.Log; nit: not needed https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentView.java:497: public boolean dispatchHoverEvent(MotionEvent event) { Can you add these two methods into android_webview/.../AwContents.java? Then I can add the downstream WebView plumbing needed to get the initial implementation up and running. AwContents is not a View though, so you won't be able to use @Override. https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2623: mBrowserAccessibilityManager = manager; Where do we check if the embedder has the INTERNET permission and the state of JavaScript to decide if we should use the script-injection or native approach? With the CL as-is, will we be enabling both approaches in ContentViewCore? Which one "wins"? https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:15: import android.util.Log; nit: remove
https://codereview.chromium.org/15741009/diff/53001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/15741009/diff/53001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentView.java:499: public boolean dispatchHoverEvent(MotionEvent event) { Similar change still needed in AwContents.java for WebView compatibililty, or were you planning to add that in a separated change?
https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:329: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); On 2013/06/07 20:23:15, Dominic Mazzoni wrote: > On 2013/06/04 17:50:22, alanv wrote: > > Views should not manually move accessibility focus. > > I think this can be a good experience - we specifically don't do anything > if the user has already explored since initiating loading of the page. > > What if we send TYPE_VIEW_FOCUSED instead? You can talk with Casey about allowing apps to automatically place focus, but for not it is not a supported use case and may result in breakage / unexpected behavior. Sending VIEW_FOCUSED would work, but the corresponding node info should have its focused bit set to true. There are some apps that use VIEW_FOCUSED in a way that breaks TalkBack, so it does some simple checks for event validity. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:354: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_TEXT_CHANGED); On 2013/06/07 20:23:15, Dominic Mazzoni wrote: > On 2013/06/04 17:50:22, alanv wrote: > > Does this also raise a TEXT_SELECTION_CHANGED event from the cursor position > > moving? > > If only one changes, it definitely does the right thing now. > > If both change, which one implies which, or is it okay to send both for a single > user action? In the platform, TEXT_CHANGED from typing is always followed by TEXT_SELECTION_CHANGED due to how the system updates cursor position resulting from typed text. So yes, send both if both the text and cursor position change. It probably won't break TalkBack if the SELECTION_CHANGED isn't sent, but it may affect BrailleBack.
Ready for another look! I made the changes to AwContents.java and Alice finished the logic to enable this code path only when scripting is disabled or the internet permission is not available. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:329: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_ACCESSIBILITY_FOCUSED); On 2013/06/17 22:53:28, alanv wrote: > On 2013/06/07 20:23:15, Dominic Mazzoni wrote: > > On 2013/06/04 17:50:22, alanv wrote: > > > Views should not manually move accessibility focus. > > > > I think this can be a good experience - we specifically don't do anything > > if the user has already explored since initiating loading of the page. > > > > What if we send TYPE_VIEW_FOCUSED instead? > > You can talk with Casey about allowing apps to automatically place focus, but > for not it is not a supported use case and may result in breakage / unexpected > behavior. Sending VIEW_FOCUSED would work, but the corresponding node info > should have its focused bit set to true. There are some apps that use > VIEW_FOCUSED in a way that breaks TalkBack, so it does some simple checks for > event validity. OK, I've disabled this for now. Maybe we can make it a user option later if we can get it to work well. https://codereview.chromium.org/15741009/diff/2001/content/public/android/jav... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:354: sendAccessibilityEvent(id, AccessibilityEvent.TYPE_VIEW_TEXT_CHANGED); On 2013/06/17 22:53:28, alanv wrote: > On 2013/06/07 20:23:15, Dominic Mazzoni wrote: > > On 2013/06/04 17:50:22, alanv wrote: > > > Does this also raise a TEXT_SELECTION_CHANGED event from the cursor position > > > moving? > > > > If only one changes, it definitely does the right thing now. > > > > If both change, which one implies which, or is it okay to send both for a > single > > user action? > > In the platform, TEXT_CHANGED from typing is always followed by > TEXT_SELECTION_CHANGED due to how the system updates cursor position resulting > from typed text. So yes, send both if both the text and cursor position change. > It probably won't break TalkBack if the SELECTION_CHANGED isn't sent, but it may > affect BrailleBack. Great, we're already sending the right events then. I think the only major issue affecting BrailleBack is Chrome's implementation of IME APIs, which we'll handle in a separate changelist. https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentView.java:15: import android.util.Log; On 2013/06/10 14:24:24, benm wrote: > nit: not needed Done. https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentView.java:497: public boolean dispatchHoverEvent(MotionEvent event) { On 2013/06/10 14:24:24, benm wrote: > Can you add these two methods into android_webview/.../AwContents.java? Then I > can add the downstream WebView plumbing needed to get the initial implementation > up and running. > > AwContents is not a View though, so you won't be able to use @Override. Done. https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2623: mBrowserAccessibilityManager = manager; On 2013/06/10 14:24:24, benm wrote: > Where do we check if the embedder has the INTERNET permission and the state of > JavaScript to decide if we should use the script-injection or native approach? > > With the CL as-is, will we be enabling both approaches in ContentViewCore? Which > one "wins"? This is all handled correctly in ContentViewCore now. If the "ACCESSIBILITY_SCRIPT_INJECTION" flag is set, and if the INTERNET permission is available, it does script injection - otherwise it uses BrowserAccessibilityManager. (All of this only if accessibility is on at a system level.) This setting is available to users, it's a checkbox in the Accessibility part of system settings. See ContentViewCore.setAccessibilityState for the logic. https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/27001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:15: import android.util.Log; On 2013/06/10 14:24:24, benm wrote: > nit: remove Done. https://codereview.chromium.org/15741009/diff/53001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/15741009/diff/53001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentView.java:499: public boolean dispatchHoverEvent(MotionEvent event) { On 2013/06/17 11:17:49, benm wrote: > Similar change still needed in AwContents.java for WebView compatibililty, or > were you planning to add that in a separated change? Done now, thanks.
+bulach Marcus, I'd appreciate it if you could review this now! I think I've addressed the issues raised by the other reviewers so far. Ben, I need your approval for android_webview if that looks good. Dave, I'd prefer to get an LGTM from you too since you're the de-facto owner of some of this code.
https://codereview.chromium.org/15741009/diff/62002/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/15741009/diff/62002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1088: return super.dispatchHoverEvent(event); AwContents doesn't inherit from View, (doesn't have a parent at all actually). I'll hook up dispatching up the View hierarchy in the framework; should be enough here to just return mContentViewCore.dispatchHoverEvent(). https://codereview.chromium.org/15741009/diff/62002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1252: return super.getAccessibilityNodeProvider(); Same comment - should be enough to just return mCVC.getAccessibilityNodeProvider.
https://codereview.chromium.org/15741009/diff/62002/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/15741009/diff/62002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1088: return super.dispatchHoverEvent(event); On 2013/06/19 09:35:53, benm wrote: > AwContents doesn't inherit from View, (doesn't have a parent at all actually). > I'll hook up dispatching up the View hierarchy in the framework; should be > enough here to just return mContentViewCore.dispatchHoverEvent(). Ah, thanks for clarifying. Done. https://codereview.chromium.org/15741009/diff/62002/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContents.java:1252: return super.getAccessibilityNodeProvider(); On 2013/06/19 09:35:53, benm wrote: > Same comment - should be enough to just return > mCVC.getAccessibilityNodeProvider. Done.
a_w lgtm, thanks!
thanks dominic! some nits and one concert below regarding exposing the native side BrowserAccessibilityAndroid directly to the java side of BrowserAccessibilityManager... I have one suggestion involving a bit more boiler-plate code, but it ensure a slightly clearer separation of native and java.. please let me know your thoughts: https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... File content/browser/accessibility/browser_accessibility_manager_android.cc (right): https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:88: case AccessibilityNotificationLoadComplete: not quite clear where these enum is coming from, but its style is wrong :) feel free to fix as a follow up, but it should be ACCESSIBILITY_NOTIFICATION_LOAD_COMPLETE, etc.. https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:106: env, obj.obj(), node->renderer_id(), JNI_TRUE); nit: missing break, is it intentional? if it is, please comment why the fallthrough is necessary. https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:109: env, obj.obj(), node->renderer_id(), JNI_FALSE); ditto https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:126: break; nit: NOTREACHED() ? https://codereview.chromium.org/15741009/diff/62002/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/15741009/diff/62002/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1554: host_impl->SetAccessibilityMode(AccessibilityModeComplete); here and below, CONSTANT_NAME needs fixing. https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:372: private AccessibilityManager mAccessibilityManager; looks like this can be final. https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1425: // TODO: correct behaviour for AndroidVox nit: TODO(dmazzoni): https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2717: public boolean onDeviceAccessibilityScriptInjectionEnabled() throws NoSuchFieldException, perhaps swallow these exceptions here rather than letting the callers deal with it? also, "on"FooBar, the "on" is normally used for events, or in c++, IPC messages.. this seems more a getter, like getDeviceAccessibilityScriptInjectionEnabled() or isDeviceAccessibilityScriptInjectionEnabled(), wdyt? https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2765: } these would probably be simpler as: boolean nativeAccessibility = false; boolean injectedAccessibility = false; if (state) { boolean onDeviceScriptInjectionEnabled = onDeviceAccessibilityScriptInjectionEnabled(); nativeAccessibility = !onDeviceScriptInjectionEnabled; injectedAccessibility = onDeviceScriptInjectionEnabled; } setNativeAccessibilityState(nativeAccessibility); setInjectedAccessibility(injectedAccessibility); https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3115: int nativeContentViewCoreImpl, boolean enabled); nit: indent https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/AccessibilityInjector.java (right): https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/AccessibilityInjector.java:148: } catch (IllegalAccessException ex) { as above, probably best catching these at onDeviceAccessibilityScriptInjectionEnabled() level. https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java (right): https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java:27: static native void nativeFocusJNI(int nativeBrowserAccessibilityAndroid); a few issues here: 1. naming: there's never a JNI suffix, please remove. 2. visibility: the native interface is almost always private, i.e., an implementation detail of the java class, rather than part of the interface exposed to other java objects. 3. type safety: because these methods are not private, anybody can pass this magic int.. normally, there's a java class containing it, so we know it can be safely casted in the C++ side to a BrowserAccessibilityAndroid*... here, it's crossing boundaries "diagonally" :) that is: BrowserAccessibilityManager in java, gets the native pointer for an entirely different class, then calls a native interface from a different class.. it's normally cleaner to cross boundaries only "vertically" and "horizonally": - in the java side, BrowserAccessibilityManager only talks to BrowserAccessibility in java. - BrowserAccessibility java then talks to its native counter part.. This leads to better decoupling, and all the classes are safer to further refactorings.... I know it's more boiler plate, but it'll save future headache: - Make it a single factory function, BrowserAccessibility.createFromId(int bamaPtr), with a comment saying that bamaPtr has to be a pointer to BrowserAccessibilityManagerAndroid.... this will be single point of contact where type safety is not guaranteed, but that's easier to track... this factory function would call into native, cast the bamaPtr to BrowserAccessibilityManagerAndroid* and return reinterpret_cast<jint>(GetFromRendererID(id)).. then back in java, there'd be a private constructor taking an int and stashing it in a member of this object. - have all the methods non-static and private. - have java methods that just delegate to the native counterparts: so we can ensure they'll always be called with the right pointer obtained from above.. how does it sound?
https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java (right): https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibility.java:27: static native void nativeFocusJNI(int nativeBrowserAccessibilityAndroid); On 2013/06/19 10:26:53, bulach wrote: > it's normally cleaner to cross boundaries only "vertically" and "horizonally": > - in the java side, BrowserAccessibilityManager only talks to > BrowserAccessibility in java. > - BrowserAccessibility java then talks to its native counter part.. Understood. Actually initially I had all of these as methods on BrowserAccessibilityManager, but taking a first argument as an int led to all sorts of bugs, because there were a couple of functions that took an ID as an argument and the rest took an int that represented a pointer. Switching to this pattern made the code extremely clear and prevented accidental programming errors, without changing performance. I'm a bit concerned about the performance implications of making BrowserAccessibility an actual object that gets instantiated. Creating this object would be very hot, we'd have several thousand of them sometimes, and the same one might be accessed dozens of times in a row from different call stacks. I wonder if there's another way we could get a bit more safety / clarity without constructing a bunch of Java objects we don't actually need.
thanks dominic! suggestion below.. not sure where the critical-path is, but let's try to optimize it! :) https://codereview.chromium.org/15741009/diff/71001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/71001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:108: final AccessibilityNodeInfo info = AccessibilityNodeInfo.obtain(mView); a potential alternative would be to invert the design here.. rather than these various calls to BrowserAccessibility.nativeIs / nativeGet in order to construct the info, would it be possible to call a single native method right here that would then call back a single @CalledByNative createAccessibilityNodeInfo(package, className, source, checkable, ....) ? it'd avoid criss-crossing the layers these many times, wdyt?
To expand on this some more, here are some alternatives and their tradeoffs: 1. Current approach - BAM.java calls BAM (C++) to get a pointer that's only valid temporarily, but this isn't enforced. BAM.java calls static methods in BA.java, passing a pointer. Doesn't create unnecessary objects, however passing the pointer is an odd "diagonal" call. This approach is quite efficient, it avoids extra object creation or extra hash lookups. 2. BAM.java calls BAM (C++) and requests properties of objects by id only. This was our original approach, the only downside is that BAM (C++) does a lot of extra hash map lookups from id to pointer. Maybe we could just optimize this approach by saving the most recent id -> pointer mapping and saving subsequent hash lookups for the same id? 3. BAM.java calls a single BAM (C++) function to fill in an entire data structure. C++ code calls various Java helpers to construct and fill in, e.g., an AccessibilityNodeInfo, and return it. The only potential downside would be the ugliness of C++ calling Java helper methods with dozens of arguments, or having dozens of helper methods to create one object. 4. BA.java is a lightweight wrapper around a C++ pointer, with no ownership guarantee (bulach proposal). This has the downside of creating lots of extra Java objects. It also means that a BA.java object might hold an invalid C++ object pointer before it's garbage-collected, since there'd be nothing to manage it. 5. BA.java is owned by the C++ BrowserAccessibilityAndroid. When the C++ object is deleted, it nullifies its owned BA.java. This still has the downside of creating a lot of Java objects, though it seems safer than #4.
On 2013/06/19 14:07:07, bulach wrote: > thanks dominic! suggestion below.. not sure where the critical-path is, but > let's try to optimize it! :) Imagine roughly one BrowserAccessibilityAndroid (C++ object) per node in the DOM! We've done a lot of work to make the C++ side fast and compact, and we'll be optimizing it more next quarter. The Java side will be fastest if it doesn't maintain its own objects, it just builds a Java data structure (AccessibilityNodeInfo) on-demand for any node the Android system wants to explore. > rather than these various calls to BrowserAccessibility.nativeIs / nativeGet in > order to construct the info, would it be possible to call a single native method > right here that would then call back a single @CalledByNative > createAccessibilityNodeInfo(package, className, source, checkable, ....) ? > > it'd avoid criss-crossing the layers these many times, wdyt? Ah, this is basically my idea #3 above. What's better, a single createAccessibilityNodeInfo with more than a dozen arguments, or dozens of helpers to set each field of AccessibilityNodeInfo, or something in-between? I'm happy to try this approach.
On 2013/06/19 15:03:01, Dominic Mazzoni wrote: > On 2013/06/19 14:07:07, bulach wrote: > > thanks dominic! suggestion below.. not sure where the critical-path is, but > > let's try to optimize it! :) > > Imagine roughly one BrowserAccessibilityAndroid (C++ object) per node > in the DOM! We've done a lot of work to make the C++ side fast and compact, > and we'll be optimizing it more next quarter. The Java side will be fastest > if it doesn't maintain its own objects, it just builds a Java data structure > (AccessibilityNodeInfo) on-demand for any node the Android system > wants to explore. > > > rather than these various calls to BrowserAccessibility.nativeIs / nativeGet > in > > order to construct the info, would it be possible to call a single native > method > > right here that would then call back a single @CalledByNative > > createAccessibilityNodeInfo(package, className, source, checkable, ....) ? > > > > it'd avoid criss-crossing the layers these many times, wdyt? > > Ah, this is basically my idea #3 above. > > What's better, a single createAccessibilityNodeInfo with more than a dozen > arguments, or dozens of helpers to set each field of AccessibilityNodeInfo, > or something in-between? > > I'm happy to try this approach. something in between :) I think there's a limit somehow on the amount of parameters (16? 32? no idea...), but if we could at least group them somehow, that would be better! also, you mentioned the ugliness of c++ calling several java methods: well, the java methods are calling a bunch of c++ methods already, so this won't be materially worse. :) and just for completeness: I think criss-crossing boundaries these many times would probably outweight the java-object creation anyways... so let's optimize _and_ make it safer! :)
On 2013/06/19 15:10:09, bulach wrote: > something in between :) Sounds good, I'll try grouping them logically!
https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... File content/browser/accessibility/browser_accessibility_manager_android.cc (right): https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:88: case AccessibilityNotificationLoadComplete: On 2013/06/19 10:26:53, bulach wrote: > not quite clear where these enum is coming from, but its style is wrong :) > feel free to fix as a follow up, but it should be > ACCESSIBILITY_NOTIFICATION_LOAD_COMPLETE, etc.. It's been around for a while and that used to be an allowed style. I'll refactor it in a separate change. https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:106: env, obj.obj(), node->renderer_id(), JNI_TRUE); On 2013/06/19 10:26:53, bulach wrote: > nit: missing break, is it intentional? if it is, please comment why the > fallthrough is necessary. Thanks, this was a bug. https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:109: env, obj.obj(), node->renderer_id(), JNI_FALSE); On 2013/06/19 10:26:53, bulach wrote: > ditto Done. https://codereview.chromium.org/15741009/diff/62002/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:126: break; On 2013/06/19 10:26:53, bulach wrote: > nit: NOTREACHED() ? I don't want this, but I added a comment. https://codereview.chromium.org/15741009/diff/62002/content/browser/android/c... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/15741009/diff/62002/content/browser/android/c... content/browser/android/content_view_core_impl.cc:1554: host_impl->SetAccessibilityMode(AccessibilityModeComplete); On 2013/06/19 10:26:53, bulach wrote: > here and below, CONSTANT_NAME needs fixing. Will do in a separate change - this used to be allowed in the style guide and it's used in a lot of source files. https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:372: private AccessibilityManager mAccessibilityManager; On 2013/06/19 10:26:53, bulach wrote: > looks like this can be final. Done. https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1425: // TODO: correct behaviour for AndroidVox On 2013/06/19 10:26:53, bulach wrote: > nit: TODO(dmazzoni): Actually, I can delete this. I wasn't sure when I first encountered this code, but it's fine as-is. https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2717: public boolean onDeviceAccessibilityScriptInjectionEnabled() throws NoSuchFieldException, On 2013/06/19 10:26:53, bulach wrote: > perhaps swallow these exceptions here rather than letting the callers deal with > it? > > also, "on"FooBar, the "on" is normally used for events, or in c++, IPC > messages.. > this seems more a getter, like getDeviceAccessibilityScriptInjectionEnabled() or > isDeviceAccessibilityScriptInjectionEnabled(), wdyt? Swallowed exceptions and renamed to isDeviceAccessibilityScriptInjectionEnabled https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2765: } On 2013/06/19 10:26:53, bulach wrote: > these would probably be simpler as: > > boolean nativeAccessibility = false; > boolean injectedAccessibility = false; > if (state) { > boolean onDeviceScriptInjectionEnabled = > onDeviceAccessibilityScriptInjectionEnabled(); > nativeAccessibility = !onDeviceScriptInjectionEnabled; > injectedAccessibility = onDeviceScriptInjectionEnabled; > } > setNativeAccessibilityState(nativeAccessibility); > setInjectedAccessibility(injectedAccessibility); Done. https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3115: int nativeContentViewCoreImpl, boolean enabled); On 2013/06/19 10:26:53, bulach wrote: > nit: indent Done. https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/AccessibilityInjector.java (right): https://codereview.chromium.org/15741009/diff/62002/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/AccessibilityInjector.java:148: } catch (IllegalAccessException ex) { On 2013/06/19 10:26:53, bulach wrote: > as above, probably best catching these at > onDeviceAccessibilityScriptInjectionEnabled() level. Done. https://codereview.chromium.org/15741009/diff/71001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/71001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:108: final AccessibilityNodeInfo info = AccessibilityNodeInfo.obtain(mView); On 2013/06/19 14:07:07, bulach wrote: > a potential alternative would be to invert the design here.. > rather than these various calls to BrowserAccessibility.nativeIs / nativeGet in > order to construct the info, would it be possible to call a single native method > right here that would then call back a single @CalledByNative > createAccessibilityNodeInfo(package, className, source, checkable, ....) ? > > it'd avoid criss-crossing the layers these many times, wdyt? Done.
lgtm for the JNI integration, thanks! some nit and suggestions, and important cleanups required below: https://codereview.chromium.org/15741009/diff/83001/content/browser/accessibi... File content/browser/accessibility/browser_accessibility_manager_android.cc (right): https://codereview.chromium.org/15741009/diff/83001/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:24: ANDROID_ACCESSIBILITY_EVENT_VIEW_TEXT_CHANGED = 16, where are these values coming from? maybe a comment saying these values need to be in sync with ___ ? https://codereview.chromium.org/15741009/diff/83001/content/browser/accessibi... File content/browser/accessibility/browser_accessibility_manager_android.h (right): https://codereview.chromium.org/15741009/diff/83001/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.h:56: //jint GetNativeNodeById(JNIEnv* env, jobject obj, jint id); nit: remove? https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:416: getContext().getSystemService(Context.ACCESSIBILITY_SERVICE); nit: indent another 4 (like line 411 above) https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:84: /** @inheritDoc */ nit: maybe @Override ? https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:107: /*** subtle :) please remove this whole block. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:224: nativeFocus(mNativeObj, nativeGetRootId(mNativeObj)); nit: maybe better to have a "nativeClearFocus(mNativeObj);", then it can GetRootId() directly in the native side.. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:292: /** nuke it from orbit, it's the only way to be sure. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:331: // TODO(dmazzoni): remove this if/when Android framework fixes bug. there was some discussions about "postInvalidate()" being the preferred way, would it be possible to use it here?
https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2734: return false; We should also check whether javascript is enabled via mContentSettings, I think?
nits, but lgtm. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentView.java:600: return provider; need {} around these for java style. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:665: String ACCESSIBILITY_SCRIPT_INJECTION = (String) field.get(null); No all caps. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2236: mBrowserAccessibilityManager.notifyFrameInfoInitialized(); {} around this https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2721: return false; {} https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2725: String ACCESSIBILITY_SCRIPT_INJECTION = (String) field.get(null); No all caps. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2755: injectedAccessibility = true; {} {} https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:103: return info; {} {} https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:331: // TODO(dmazzoni): remove this if/when Android framework fixes bug. On 2013/06/20 09:18:42, bulach wrote: > there was some discussions about "postInvalidate()" being the preferred way, > would it be possible to use it here? That is probably correct. Also just to make sure, check ApiCompatibilityUtils#postInvalidateOnAnimation() to see if that's what you should be using (even though it doesn't seem like it).
All feedback addressed! I'm going to do some final testing to make sure I didn't accidentally break something, then land this shortly. Thanks for the thorough review! https://codereview.chromium.org/15741009/diff/83001/content/browser/accessibi... File content/browser/accessibility/browser_accessibility_manager_android.cc (right): https://codereview.chromium.org/15741009/diff/83001/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.cc:24: ANDROID_ACCESSIBILITY_EVENT_VIEW_TEXT_CHANGED = 16, On 2013/06/20 09:18:42, bulach wrote: > where are these values coming from? maybe a comment saying these values need to > be in sync with ___ ? These are enums from android.view.accessibility.AccessibilityEvent. I added a comment - or is there another convention for getting Java enum values? https://codereview.chromium.org/15741009/diff/83001/content/browser/accessibi... File content/browser/accessibility/browser_accessibility_manager_android.h (right): https://codereview.chromium.org/15741009/diff/83001/content/browser/accessibi... content/browser/accessibility/browser_accessibility_manager_android.h:56: //jint GetNativeNodeById(JNIEnv* env, jobject obj, jint id); On 2013/06/20 09:18:42, bulach wrote: > nit: remove? Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentView.java:600: return provider; On 2013/06/20 16:57:11, David Trainor wrote: > need {} around these for java style. Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:416: getContext().getSystemService(Context.ACCESSIBILITY_SERVICE); On 2013/06/20 09:18:42, bulach wrote: > nit: indent another 4 (like line 411 above) Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:665: String ACCESSIBILITY_SCRIPT_INJECTION = (String) field.get(null); On 2013/06/20 16:57:11, David Trainor wrote: > No all caps. Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2236: mBrowserAccessibilityManager.notifyFrameInfoInitialized(); On 2013/06/20 16:57:11, David Trainor wrote: > {} around this Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2721: return false; On 2013/06/20 16:57:11, David Trainor wrote: > {} Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2725: String ACCESSIBILITY_SCRIPT_INJECTION = (String) field.get(null); On 2013/06/20 16:57:11, David Trainor wrote: > No all caps. Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2734: return false; On 2013/06/20 10:00:42, benm wrote: > We should also check whether javascript is enabled via mContentSettings, I > think? Good idea, done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2755: injectedAccessibility = true; On 2013/06/20 16:57:11, David Trainor wrote: > {} {} Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java (right): https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:84: /** @inheritDoc */ On 2013/06/20 09:18:42, bulach wrote: > nit: maybe @Override ? Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:103: return info; On 2013/06/20 16:57:11, David Trainor wrote: > {} {} Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:107: /*** On 2013/06/20 09:18:42, bulach wrote: > subtle :) > please remove this whole block. Oops! https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:224: nativeFocus(mNativeObj, nativeGetRootId(mNativeObj)); On 2013/06/20 09:18:42, bulach wrote: > nit: maybe better to have a "nativeClearFocus(mNativeObj);", then it can > GetRootId() directly in the native side.. Good idea, done - I'll call it nativeBlur to match what we call it on the C++ side https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:292: /** On 2013/06/20 09:18:42, bulach wrote: > nuke it from orbit, it's the only way to be sure. Done. https://codereview.chromium.org/15741009/diff/83001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:331: // TODO(dmazzoni): remove this if/when Android framework fixes bug. On 2013/06/20 09:18:42, bulach wrote: > there was some discussions about "postInvalidate()" being the preferred way, > would it be possible to use it here? I'll try it with postInvalidate but leave the TODO because ideally we shouldn't have to do anything, I still think this is really an Android bug.
One small comment although we can follow up with this change if needed Thanks https://codereview.chromium.org/15741009/diff/105001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/105001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1687: public boolean dispatchHoverEvent(MotionEvent event) { Can you use onHoverEvent rather than dispatchHoverEvent? The latter is intended primarily for distributing events around the views in a view group, the formed is intended for actually handling the event. (AIUI). https://codereview.chromium.org/15741009/diff/105001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1691: // ContentView.dispatchHoverEvent will call super. Remember ContentView is not the only View class that ContentViewCore is used with. Maybe better to provide this comment up in the javadoc as part of the contract (especially as this is a departure from the View#dispatchHoverEvent() function referenced) - the convention elsewhere is to make a mContainerViewInternals.super_dispatchXxx() call, to make this more explicit.
https://codereview.chromium.org/15741009/diff/105001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/15741009/diff/105001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1687: public boolean dispatchHoverEvent(MotionEvent event) { On 2013/06/21 17:19:38, joth wrote: > Can you use onHoverEvent rather than dispatchHoverEvent? The latter is intended > primarily for distributing events around the views in a view group, the formed > is intended for actually handling the event. (AIUI). This is on purpose, this is what the Android accessibility folks recommend. When accessibility is on, we want to catch touch events at the earliest possible stage and use them for touch exploration and make sure there's no way they could be sent to the app. https://codereview.chromium.org/15741009/diff/105001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1691: // ContentView.dispatchHoverEvent will call super. On 2013/06/21 17:19:38, joth wrote: > Remember ContentView is not the only View class that ContentViewCore is used > with. Maybe better to provide this comment up in the javadoc as part of the > contract (especially as this is a departure from the View#dispatchHoverEvent() > function referenced) Good point, I updated the comment. > - the convention elsewhere is to make a > mContainerViewInternals.super_dispatchXxx() call, to make this more explicit. It's not obvious to me that I could use this convention without breaking WebView (until WebView implements super_dispatchHoverEvent too). Let's do this in a subsequent change if needed.
Message was sent while issue was closed.
Committed patchset #16 manually as r207875 (presubmit successful).
I had to make a few changes so this doesn't crash on ICS. (Some of the APIs used are JellyBean-only.) Anyone want to take another quick look? I saw the code pattern of ClassName and JellyBeanClassName, but decided that (1) JellyBeanBrowserAccessibilityManager would basically make it impossible to fit JNI calls in 80 columns in C++, and (2) it doesn't make as much sense when JB is the only version that's actually used. So instead I created BaseBrowserAccessibilityManager, an interface that is safe to include in ICS code. When it's instantiated, it's always a real BrowserAccessibilityManager.
sorry about the ICS hassle... https://codereview.chromium.org/15741009/diff/121001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_manager_android.cc (right): https://codereview.chromium.org/15741009/diff/121001/content/browser/accessib... content/browser/accessibility/browser_accessibility_manager_android.cc:64: if (base::android::BuildInfo::GetInstance()->sdk_int() < kJellyBeanSdkLevel) seems unfortunate to duplicate the check here as well as ContentViewCore.setNativeAccessibilityState(). I can't get a picture how the callstack looks that results in us getting here, but if there is some way we can either pass in this setting as a param, or fetch out the manager.IsAvailable() state back to java it would at least put the logic in one place. https://codereview.chromium.org/15741009/diff/138001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/15741009/diff/138001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1313: public BaseBrowserAccessibilityManager getBrowserAccessibilityManager() { Can this return AccessibilityNodeProvider? It will avoid leaking this content/ internal thing up into the webview embedding layer. (although see next comment...) https://codereview.chromium.org/15741009/diff/138001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/JellyBeanContentView.java (right): https://codereview.chromium.org/15741009/diff/138001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JellyBeanContentView.java:37: (AccessibilityNodeProvider) getContentViewCore().getBrowserAccessibilityManager(); the casting is slightly awkward. My understanding is it would be fine to have ContentViewCore.getBrowserAccessibilityManager() return the AccessibilityNodeProvider type so long as we don't ever call it on pre-JB device. (which moving the call to this JB subclass achieves). So I guess my question is, can we just skip having the BaseBrowserAccessibilityManager altogether, and just be careful not to use this path on platforms it doesn't exist on?
https://codereview.chromium.org/15741009/diff/121001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_manager_android.cc (right): https://codereview.chromium.org/15741009/diff/121001/content/browser/accessib... content/browser/accessibility/browser_accessibility_manager_android.cc:64: if (base::android::BuildInfo::GetInstance()->sdk_int() < kJellyBeanSdkLevel) On 2013/06/21 23:35:39, joth wrote: > seems unfortunate to duplicate the check here as well as > ContentViewCore.setNativeAccessibilityState(). I can't get a picture how the > callstack looks that results in us getting here, but if there is some way we can > either pass in this setting as a param, or fetch out the manager.IsAvailable() > state back to java it would at least put the logic in one place. Actually the Java logic does take care of it...this is actually just to allow the accessibility content_browsertests to run on ICS rather than crash. :) I'll add a comment. https://codereview.chromium.org/15741009/diff/138001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/JellyBeanContentView.java (right): https://codereview.chromium.org/15741009/diff/138001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JellyBeanContentView.java:37: (AccessibilityNodeProvider) getContentViewCore().getBrowserAccessibilityManager(); On 2013/06/21 23:35:39, joth wrote: > the casting is slightly awkward. My understanding is it would be fine to have > ContentViewCore.getBrowserAccessibilityManager() return the > AccessibilityNodeProvider type so long as we don't ever call it on pre-JB > device. (which moving the call to this JB subclass achieves). > So I guess my question is, can we just skip having the > BaseBrowserAccessibilityManager altogether, and just be careful not to use this > path on platforms it doesn't exist on? I tried that, but it's failing on content_unittests. Would love to see this a bit cleaner, but let's land it like this and then experiment with other solutions in small follow-on patches.
lgtm https://codereview.chromium.org/15741009/diff/146001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/15741009/diff/146001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1313: public BaseBrowserAccessibilityManager getBrowserAccessibilityManager() { Thoughts on doing the cast and returning AccessibilityNodeProvider here?
This is still causing problems...I think I see why, the JNI C++ code is trying to access BrowserAccessibilitlyManager, even on ICS - just initializing JNI is enough to cause a problem. I think I have a better idea - perhaps BrowserAccessibilitlyManager shouldn't be a subclass of AccessibilityNodeProvider - instead a tiny subclass of BrowserAccessibilitlyManager can create an anonymous AccessibilityNodeProvider and delegate back to it, on JB. I should have thought of that before. I was hung up on the idea that BrowserAccessibilitlyManager must be an AccessibilityNodeProvider. On 2013/06/22 19:30:46, joth wrote: > lgtm > > https://codereview.chromium.org/15741009/diff/146001/android_webview/java/src... > File android_webview/java/src/org/chromium/android_webview/AwContents.java > (right): > > https://codereview.chromium.org/15741009/diff/146001/android_webview/java/src... > android_webview/java/src/org/chromium/android_webview/AwContents.java:1313: > public BaseBrowserAccessibilityManager getBrowserAccessibilityManager() { > Thoughts on doing the cast and returning AccessibilityNodeProvider here?
This looks promising! Down to just two test failures. Any idea about the failure in AwContentsTest.testCreateAndGcManyTimes? Does the test output include any information about how or why it failed?
On 2013/06/24 03:07:47, Dominic Mazzoni wrote: > This looks promising! > > Down to just two test failures. > > Any idea about the failure in AwContentsTest.testCreateAndGcManyTimes? Does the > test output include any information about how or why it failed? Related to overriding dispatchhoverevent instead of onhoverevent: Can you also test that accessibility works as expected with infobars?
On Mon, Jun 24, 2013 at 9:21 AM, <dtrainor@chromium.org> wrote: > Related to overriding dispatchhoverevent instead of onhoverevent: > > Can you also test that accessibility works as expected with infobars? > Good thought. Touch exploration for infobars wasn't working. I switched to using onHoverEvent - it required a couple of minor changes for this to work correctly, but now infobars are working correctly as well as touch exploration. - Dominic
I think I addressed all of the test failures, crossing my fingers I can land it this time...
On 2013/06/24 22:36:12, Dominic Mazzoni wrote: > I think I addressed all of the test failures, crossing my fingers I can land it > this time... We still need a method in AwContents to be able to forward the onHoverEvent into ContentViewCore... The Android framework is only able to see AwContents.
On Mon, Jun 24, 2013 at 3:39 PM, <benm@chromium.org> wrote: > On 2013/06/24 22:36:12, Dominic Mazzoni wrote: > >> I think I addressed all of the test failures, crossing my fingers I can >> land >> > it > >> this time... >> > > We still need a method in AwContents to be able to forward the > onHoverEvent into > ContentViewCore... The Android framework is only able to see AwContents. > Already there - AwContents.onHoverEvent just returns mContentViewCore.onHoverEvent
On 2013/06/24 22:39:37, benm wrote: > On 2013/06/24 22:36:12, Dominic Mazzoni wrote: > > I think I addressed all of the test failures, crossing my fingers I can land > it > > this time... > > We still need a method in AwContents to be able to forward the onHoverEvent into > ContentViewCore... The Android framework is only able to see AwContents. Ah great, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/15741009/166001
Message was sent while issue was closed.
Change committed as 208379 |
