Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2948513002:
Forward BrowserAccessibility::accHitTest to AXPlatformNode. (Closed)
|
Created:
3 years, 6 months ago by dougt Modified:
3 years, 5 months ago Reviewers:
dmazzoni CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, jam, yuzo+watch_chromium.org, dougt+watch_chromium.org, darin-cc_chromium.org, aleventhal+watch_chromium.org, dtseng+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionForward BrowserAccessibility::accHitTest to AXPlatformNode.
BUG=703369
Review-Url: https://codereview.chromium.org/2948513002
Cr-Commit-Position: refs/heads/master@{#482398}
Committed: https://chromium.googlesource.com/chromium/src/+/07c299e9c4839adca2fb75822fe21c4513b4bdd6
Patch Set 1 #Patch Set 2 : Add tests, delegate. #Patch Set 3 : Adjust the test to match the spec #
Total comments: 10
Patch Set 4 : add simple hit test #
Total comments: 2
Patch Set 5 : spelling #
Messages
Total messages: 36 (26 generated)
The CQ bit was checked by dougt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dougt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dougt@chromium.org changed reviewers: + dmazzoni@chromium.org
dmazzoni, please take a look. Also, if there is something you know that uses accHitTest that I could test in addition to ZoomText, please let me know. It doesn't look like this API is used much.
For testing, open Inspect.exe and click on "Watch Cursor" in the toolbar, then hover over anything for 1 second
For manual testing I'd suggest testing an html page with overflow or a z-index, something where a naive browser-side-only hit test couldn't possibly return the right result. https://codereview.chromium.org/2948513002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/2948513002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility.cc:1133: auto* accessible = ApproximateHitTest(gfx::Point(x, y)); This patch would replace CachingAsyncHitTest (which is pretty accurate) with ApproximateHitTest, which is not. I think you should just call CachingAsyncHitTest here. https://codereview.chromium.org/2948513002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_com_win.cc (left): https://codereview.chromium.org/2948513002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility_com_win.cc:452: if (!owner()->GetScreenBoundsRect().Contains(point)) { Where did this check go? It looks like it was lost https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right): https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:130: root.location = gfx::RectF(10, 40, 800, 600); Can you make this unit test work with more than one object? https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:141: // This is expected to return a id, not a dispatch. Shouldn't it return a dispatch if it's anything other than "self"? https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/test_ax_node_wrapper.cc (right): https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/test_ax_node_wrapper.cc:116: if (!bounds.Contains(x, y)) Let's have this iterate over children too, to do a really simple nested-rectangle hit test, that will allow for fully unit-testing the Windows API
On 2017/06/23 18:42:26, dmazzoni wrote: > For testing, open Inspect.exe and click on "Watch Cursor" in the toolbar, then > hover over anything for 1 second I do not see any difference with my changeset. Somewhat related, both Chrome and Firefox do not change the |Name| attribute in Inspect.exe when mousing around. However Edge does.
On 2017/06/23 19:15:01, dougt wrote: > On 2017/06/23 18:42:26, dmazzoni wrote: > > For testing, open Inspect.exe and click on "Watch Cursor" in the toolbar, then > > hover over anything for 1 second > > I do not see any difference with my changeset. Somewhat related, both Chrome > and Firefox do not change the |Name| attribute in Inspect.exe when mousing > around. However Edge does. After going to chrome://accessibility and flipping all the bits on, I see the |Name| attributes changing when I mouse over things.
The CQ bit was checked by dougt@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2948513002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/2948513002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility.cc:1133: auto* accessible = ApproximateHitTest(gfx::Point(x, y)); On 2017/06/23 18:51:12, dmazzoni wrote: > This patch would replace CachingAsyncHitTest (which is pretty > accurate) with ApproximateHitTest, which is not. I think you > should just call CachingAsyncHitTest here. Done. https://codereview.chromium.org/2948513002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_com_win.cc (left): https://codereview.chromium.org/2948513002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility_com_win.cc:452: if (!owner()->GetScreenBoundsRect().Contains(point)) { On 2017/06/23 18:51:12, dmazzoni wrote: > Where did this check go? It looks like it was lost Done. https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right): https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:130: root.location = gfx::RectF(10, 40, 800, 600); On 2017/06/23 18:51:12, dmazzoni wrote: > Can you make this unit test work with more than one object? Done. https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:141: // This is expected to return a id, not a dispatch. On 2017/06/23 18:51:12, dmazzoni wrote: > Shouldn't it return a dispatch if it's anything other than "self"? Done. This was because our hit test testing code would just return self. I've updated test to look for a dispatch (and test that we got the right now), and updated the testing code to be more sophisticated. https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/test_ax_node_wrapper.cc (right): https://codereview.chromium.org/2948513002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/test_ax_node_wrapper.cc:116: if (!bounds.Contains(x, y)) On 2017/06/23 18:51:12, dmazzoni wrote: > Let's have this iterate over children too, to do a > really simple nested-rectangle hit test, that will > allow for fully unit-testing the Windows API SG. This impl isn't perfect and will not be able to test for all things. I suspect we can just follow up if we need to test things that are more complicated.
lgtm https://codereview.chromium.org/2948513002/diff/80001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right): https://codereview.chromium.org/2948513002/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:158: // We got something back, make sure that it has the corerct name. corerct -> correct
https://codereview.chromium.org/2948513002/diff/80001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_win_unittest.cc (right): https://codereview.chromium.org/2948513002/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_win_unittest.cc:158: // We got something back, make sure that it has the corerct name. On 2017/06/26 16:52:58, dmazzoni wrote: > corerct -> correct Done.
The CQ bit was checked by dougt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2948513002/#ps100001 (title: "spelling")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498504345552860,
"parent_rev": "1be030652a3b79cfae1ea6192b0f63144e4d6888", "commit_rev":
"07c299e9c4839adca2fb75822fe21c4513b4bdd6"}
Message was sent while issue was closed.
Description was changed from ========== Forward BrowserAccessibility::accHitTest to AXPlatformNode. BUG=703369 ========== to ========== Forward BrowserAccessibility::accHitTest to AXPlatformNode. BUG=703369 Review-Url: https://codereview.chromium.org/2948513002 Cr-Commit-Position: refs/heads/master@{#482398} Committed: https://chromium.googlesource.com/chromium/src/+/07c299e9c4839adca2fb75822fe2... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/07c299e9c4839adca2fb75822fe2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
