Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1119923002:
Notify BrowserAccessibilityManager of frame info after already loaded (Closed)
DescriptionNotify BrowserAccessibilityManager of frame info after already loaded
BrowserAccessibilityManager is not notified that frame info is ready
when accessibility wasn't turned on when the frame info was updated in
ContentViewCore. As a result, calls to getAccessibilityNodeProvider()
will not return the node information of the page in this situation.
This arrises when accessibility is turned on after the page was loaded,
such as by UIAutomator. Because BrowserAccessibility is initialized
asynchronously, there is no guarantee that it will complete in time for
the call to getAccessibilityNodeProvider. However, experiments have
shown that many smaller pages (very common for WebView applications)
return accessibility node information successfully after this change.
BUG=482750
Committed: https://crrev.com/1743bc765f18f8d439f0e9e7f69b8761a654a2ad
Cr-Commit-Position: refs/heads/master@{#328333}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Moving deferred notification to setBrowserAccessibilityManager #
Total comments: 1
Patch Set 3 : Only defer notification once #
Total comments: 2
Patch Set 4 : Moving frame state to RenderCoordinates #
Total comments: 2
Patch Set 5 : Fixing comment nits #Patch Set 6 : Added null check for setBrowserAccessibilityManager #Patch Set 7 : Fixing conflicts after rebase #
Messages
Total messages: 30 (11 generated)
demarem@amazon.com changed reviewers: + dmazzoni@chromium.org, jdduke@chromium.org
jdduke, please review as owner. dmazzoni, we spoke via email about making changes to improve UIAutomator functionality. This is the last part of that fix. Thanks!
Nice work! The diagnosis of the issue makes perfect sense, but I have an idea for a slightly different fix: https://codereview.chromium.org/1119923002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1119923002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2886: mBrowserAccessibilityManager = manager; Would it work to check mAccessibilityNotificationDeferred here instead? If mAccessibilityNotificationDeferred is true at this point, call mBrowserAccessibilityManager.notifyFrameInfoInitialized(). I'm worried that doing it in getAccessibilityNodeProvider still leaves open a race condition, where calling getAccessibilityNodeProvider() the first time triggers creation of the BrowserAccessibilityManager, but it could take a while to get created asynchronously - and if UI Automator (or TalkBack) finishes and block and doesn't call getAccessibilityNodeProvider again, it won't ever get the notification.
On 2015/05/01 at 06:24:09, dmazzoni wrote: > Nice work! The diagnosis of the issue makes perfect sense, but I have an idea for a slightly different fix: > > https://codereview.chromium.org/1119923002/diff/1/content/public/android/java... > File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): > > https://codereview.chromium.org/1119923002/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2886: mBrowserAccessibilityManager = manager; > Would it work to check mAccessibilityNotificationDeferred here instead? If mAccessibilityNotificationDeferred is true at this point, call mBrowserAccessibilityManager.notifyFrameInfoInitialized(). > > I'm worried that doing it in getAccessibilityNodeProvider still leaves open a race condition, where calling getAccessibilityNodeProvider() the first time triggers creation of the BrowserAccessibilityManager, but it could take a while to get created asynchronously - and if UI Automator (or TalkBack) finishes and block and doesn't call getAccessibilityNodeProvider again, it won't ever get the notification. That makes sense. Experimenting with this change, there are still some pages that require multiple UIAutomator dump calls, but I think this should increase the amount of time that BrowserAccessibilityManager has to initialize. I had to make a small change in BrowserAccessibilityManager.java to fix runtime initialization problems of calling notify before most of the setup in the constructor completed. Thanks!
lgtm
lgtm https://codereview.chromium.org/1119923002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1119923002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2890: mAccessibilityNotificationDeferred = false; Maybe call this |mAccessibilityFrameNotificationDeferred|? I guess this bool is really just tracking whether or not we've gotten a single frame update, ever? In that case, we could also just call it |mHasRecievedFrameInfoUpdate| and never reset the bool, but what you have is fine.
On 2015/05/01 at 20:25:44, jdduke wrote: > lgtm > > https://codereview.chromium.org/1119923002/diff/20001/content/public/android/... > File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): > > https://codereview.chromium.org/1119923002/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2890: mAccessibilityNotificationDeferred = false; > Maybe call this |mAccessibilityFrameNotificationDeferred|? > > I guess this bool is really just tracking whether or not we've gotten a single frame update, ever? In that case, we could also just call it |mHasRecievedFrameInfoUpdate| and never reset the bool, but what you have is fine. I agree that your suggestion is simpler. Thanks!
https://codereview.chromium.org/1119923002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1119923002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2888: mBrowserAccessibilityManager.notifyFrameInfoInitialized(); Hmm, but we may not have received a frame info update yet. I was thinking you could just set mHasReceivedFrameInfoUpdate to true unconditionally in updateFrameInfo, then notify the new mBrowserAccessibilityManager if that is true. Alternatively, and perhaps even preferably, we could move the boolean to a RenderCoordinates.mHasUpdatedFrameInfo (that gets set in RenderCoordinates.updateFrameInfo() and reset in RenderCoordinates.reset()), and we can just query that value here as to whether or not to notify the new manager.
On 2015/05/04 at 17:41:50, jdduke wrote: > https://codereview.chromium.org/1119923002/diff/40001/content/public/android/... > File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): > > https://codereview.chromium.org/1119923002/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2888: mBrowserAccessibilityManager.notifyFrameInfoInitialized(); > Hmm, but we may not have received a frame info update yet. I was thinking you could just set mHasReceivedFrameInfoUpdate to true unconditionally in updateFrameInfo, then notify the new mBrowserAccessibilityManager if that is true. > > Alternatively, and perhaps even preferably, we could move the boolean to a RenderCoordinates.mHasUpdatedFrameInfo (that gets set in RenderCoordinates.updateFrameInfo() and reset in RenderCoordinates.reset()), and we can just query that value here as to whether or not to notify the new manager. jdduke, PTAL. Thanks!
lgtm with nit. https://codereview.chromium.org/1119923002/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/1119923002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:39: // ContentViewCore has received frame info. Nit: We can probably get by without this comment (see my point below). https://codereview.chromium.org/1119923002/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:352: * @return Whether ContentViewCore has received frame info. Nit: Let's remove the ContentViewCore reference, you can just say "@return Whether a frame info update has been received."
The CQ bit was checked by demarem@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, jdduke@chromium.org Link to the patchset: https://codereview.chromium.org/1119923002/#ps80001 (title: "Fixing comment nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119923002/80001
The CQ bit was unchecked by demarem@amazon.com
The CQ bit was checked by demarem@amazon.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/1119923002/#ps100001 (title: "Added null check for setBrowserAccessibilityManager")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119923002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by demarem@amazon.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/1119923002/#ps110004 (title: "Fixing conflicts after rebase")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119923002/110004
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jdduke, Do I need another look through after fixing try job and rebasing issues? Thanks! https://codereview.chromium.org/1119923002/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1119923002/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2888: mBrowserAccessibilityManager.notifyFrameInfoInitialized(); On 2015/05/04 17:41:50, jdduke wrote: > Hmm, but we may not have received a frame info update yet. I was thinking you > could just set mHasReceivedFrameInfoUpdate to true unconditionally in > updateFrameInfo, then notify the new mBrowserAccessibilityManager if that is > true. > > Alternatively, and perhaps even preferably, we could move the boolean to a > RenderCoordinates.mHasUpdatedFrameInfo (that gets set in > RenderCoordinates.updateFrameInfo() and reset in RenderCoordinates.reset()), and > we can just query that value here as to whether or not to notify the new > manager. Done.
On 2015/05/05 06:48:35, demarem wrote: > jdduke, Do I need another look through after fixing try job and rebasing issues? > Thanks! still lgtm.
The CQ bit was checked by demarem@amazon.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119923002/110004
Message was sent while issue was closed.
Committed patchset #7 (id:110004)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1743bc765f18f8d439f0e9e7f69b8761a654a2ad Cr-Commit-Position: refs/heads/master@{#328333} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
