Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2671563002:
MacViews: Implement NSAccessibilityPressAction for Views with clickable roles. (Closed)
|
Created:
3 years, 10 months ago by Patti Lor Modified:
3 years, 10 months ago CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, jam, yuzo+watch_chromium.org, je_julie, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Implement NSAccessibilityPressAction for Views with clickable roles.
Currently, no NSAccessibilityActions are supported for MacViews. Refactor out
code supporting this for clickable controls inside Blink and re-use the code for
clickable Views.
BUG=679247
Review-Url: https://codereview.chromium.org/2671563002
Cr-Commit-Position: refs/heads/master@{#449519}
Committed: https://chromium.googlesource.com/chromium/src/+/e8f8ea9d2076848d998001dea1e7b90d7beb83a8
Patch Set 1 #Patch Set 2 : Actually add utils files. #Patch Set 3 : Add test. #Patch Set 4 : Use method instead of private member. #
Total comments: 19
Patch Set 5 : Review comments. #Patch Set 6 : Fix android. #
Total comments: 8
Patch Set 7 : Add TODO for "feed" ARIA role. #
Dependent Patchsets: Messages
Total messages: 59 (48 generated)
The CQ bit was checked by patricialor@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by patricialor@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: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by patricialor@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by patricialor@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.
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hi Trent, PTAL! Thanks.
On 2017/02/03 06:43:07, Patti Lor wrote: > Hi Trent, PTAL! Thanks. Sorry - this got buried under a pile of sheriff email - I'll take a look tomorrow.
https://codereview.chromium.org/2671563002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/2671563002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility.cc:1030: return ui::IsRoleClickable((ui::AXRole)GetRole()); we aren't allowed to use C-style casts in Chromium, but it looks easy to change GetRole() to return a ui::AXRole instead of int32_t. https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/BUILD.gn File ui/accessibility/BUILD.gn (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/BUILD.... ui/accessibility/BUILD.gn:44: "ax_utils.cc", I've seen feedback that we we try to avoid "foo_utils" headers since they easily become a grab-bag of unrelated things. Maybe ax_role_properties.h? https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... File ui/accessibility/ax_utils.cc (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... ui/accessibility/ax_utils.cc:11: case ui::AX_ROLE_BUTTON: ui:: prefixes not needed https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... File ui/accessibility/ax_utils.h (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... ui/accessibility/ax_utils.h:13: AX_EXPORT bool IsRoleClickable(AXRole role); nit: needs a comment https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:313: [axActions addObject:NSAccessibilityPressAction]; Do we expect much more to appear in future? E.g. we could just have return ui::IsRoleClickable(node_->GetData().role) ? @[NSAccessibilityPressAction] : @[]; (do we need to worry about NSAccessibilityShowMenuAction NSAccessibilityPickAction NSAccessibilityConfirmAction -- should these all map to DO_DEFAULT?) Is it worth mentioning that ui::AX_ACTION_SET_VALUE doesn't need to be reported here for Mac? https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:321: if ([action isEqualToString:NSAccessibilityPressAction]) { nit: curlies not required https://codereview.chromium.org/2671563002/diff/60001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2671563002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:47: bool mouse_was_pressed() { return mouse_was_pressed_; } nit: const method https://codereview.chromium.org/2671563002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:92: id value = [A11yElementAtMidpoint() accessibilityAttributeValue:attribute]; nit: just return? (i.e. |value| not required)
The CQ bit was checked by patricialor@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: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by patricialor@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: 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 patricialor@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: 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 patricialor@chromium.org to run a CQ dry run
Patchset #6 (id:120001) has been deleted
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.
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... File ui/accessibility/ax_utils.cc (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... ui/accessibility/ax_utils.cc:9: bool IsRoleClickable(AXRole role) { This is fine for now, but @aboxhall may have some thoughts. We've been talking about the idea of having a json5 file with a taxonomy of all roles and auto-generating helper functions like this one.
https://codereview.chromium.org/2671563002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/2671563002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility.cc:1030: return ui::IsRoleClickable((ui::AXRole)GetRole()); On 2017/02/09 02:45:18, tapted wrote: > we aren't allowed to use C-style casts in Chromium, but it looks easy to change > GetRole() to return a ui::AXRole instead of int32_t. Done. https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/BUILD.gn File ui/accessibility/BUILD.gn (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/BUILD.... ui/accessibility/BUILD.gn:44: "ax_utils.cc", On 2017/02/09 02:45:18, tapted wrote: > I've seen feedback that we we try to avoid "foo_utils" headers since they easily > become a grab-bag of unrelated things. > > Maybe ax_role_properties.h? Fair point, done! Thanks for the suggestion https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... File ui/accessibility/ax_utils.cc (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... ui/accessibility/ax_utils.cc:9: bool IsRoleClickable(AXRole role) { On 2017/02/09 20:24:43, dmazzoni wrote: > This is fine for now, but @aboxhall may have some thoughts. > We've been talking about the idea of having a json5 file > with a taxonomy of all roles and auto-generating helper > functions like this one. Oh, that sounds interesting, thanks for the FYI! I'm assuming she can already see this because of the automatic CC list :) https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... ui/accessibility/ax_utils.cc:11: case ui::AX_ROLE_BUTTON: On 2017/02/09 02:45:18, tapted wrote: > ui:: prefixes not needed Oops, thanks! https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... File ui/accessibility/ax_utils.h (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/ax_uti... ui/accessibility/ax_utils.h:13: AX_EXPORT bool IsRoleClickable(AXRole role); On 2017/02/09 02:45:18, tapted wrote: > nit: needs a comment Done. https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:313: [axActions addObject:NSAccessibilityPressAction]; On 2017/02/09 02:45:18, tapted wrote: > Do we expect much more to appear in future? E.g. we could just have > > return ui::IsRoleClickable(node_->GetData().role) ? > @[NSAccessibilityPressAction] : @[]; > > (do we need to worry about NSAccessibilityShowMenuAction > NSAccessibilityPickAction NSAccessibilityConfirmAction -- should these all map > to DO_DEFAULT?) > > Is it worth mentioning that ui::AX_ACTION_SET_VALUE doesn't need to be reported > here for Mac? I think so, yes - "ConfirmAction" simulates pressing the return key, so clicking on the view where we've invoked it != doing the default thing for the dialog the view belongs to. "ShowMenu" does a right click? Not sure about "PickAction" but "DeleteAction" "CancelAction" etc are probably within scope to implement too. I'm not sure what you mean about AX_ACTION_SET_VALUE? I'm going to guess you wanted a comment explaining why some things are Views ui::AX_ACTIONs but not on Mac, added it now :) https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:321: if ([action isEqualToString:NSAccessibilityPressAction]) { On 2017/02/09 02:45:18, tapted wrote: > nit: curlies not required Done. https://codereview.chromium.org/2671563002/diff/60001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2671563002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:47: bool mouse_was_pressed() { return mouse_was_pressed_; } On 2017/02/09 02:45:18, tapted wrote: > nit: const method Done. https://codereview.chromium.org/2671563002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:92: id value = [A11yElementAtMidpoint() accessibilityAttributeValue:attribute]; On 2017/02/09 02:45:18, tapted wrote: > nit: just return? (i.e. |value| not required) Done, thanks! https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... File content/browser/accessibility/browser_accessibility_android.cc (right): https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. All the changes in this file were required to make the compiler happy after changing GetRole() to return a ui::AXRole. Is adding the default case to the ones that only handle a few roles OK, or if we should be explicitly listing all roles?
https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... File content/browser/accessibility/browser_accessibility_android.cc (right): https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/02/09 23:11:23, Patti Lor wrote: > All the changes in this file were required to make the compiler happy after > changing GetRole() to return a ui::AXRole. Is adding the default case to the > ones that only handle a few roles OK, or if we should be explicitly listing all > roles? Default case is fine for small switches. Thanks. https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility_android.cc:633: // No role description. We should add this one, it's a relatively new addition to ARIA. Make this a TODO?
lgtm
lgtm % nit & dmazzoni's comment https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2671563002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:313: [axActions addObject:NSAccessibilityPressAction]; On 2017/02/09 23:11:23, Patti Lor wrote: > On 2017/02/09 02:45:18, tapted wrote: > > Do we expect much more to appear in future? E.g. we could just have > > > > return ui::IsRoleClickable(node_->GetData().role) ? > > @[NSAccessibilityPressAction] : @[]; > > > > (do we need to worry about NSAccessibilityShowMenuAction > > NSAccessibilityPickAction NSAccessibilityConfirmAction -- should these all map > > to DO_DEFAULT?) > > > > Is it worth mentioning that ui::AX_ACTION_SET_VALUE doesn't need to be > reported > > here for Mac? > > I think so, yes - "ConfirmAction" simulates pressing the return key, so clicking > on the view where we've invoked it != doing the default thing for the dialog the > view belongs to. "ShowMenu" does a right click? Not sure about "PickAction" but > "DeleteAction" "CancelAction" etc are probably within scope to implement too. Ack. > > I'm not sure what you mean about AX_ACTION_SET_VALUE? I'm going to guess you > wanted a comment explaining why some things are Views ui::AX_ACTIONs but not on > Mac, added it now :) ah - yep that's exactly what I meant https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... File content/browser/accessibility/browser_accessibility_android.cc (right): https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/02/09 23:11:23, Patti Lor wrote: > All the changes in this file were required to make the compiler happy after > changing GetRole() to return a ui::AXRole. Is adding the default case to the > ones that only handle a few roles OK, or if we should be explicitly listing all > roles? what you've done here lg imo, but I don't know much about android a11y https://codereview.chromium.org/2671563002/diff/140001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2671563002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:325: // are already implemented in - accessibilitySetValue:forAttribute:, nit remove space between "- accessibilitySetValue"
The CQ bit was checked by patricialor@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...
Thanks tapted@ and dmazzoni@ for the reviews! https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... File content/browser/accessibility/browser_accessibility_android.cc (right): https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility_android.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2017/02/09 23:25:05, tapted wrote: > On 2017/02/09 23:11:23, Patti Lor wrote: > > All the changes in this file were required to make the compiler happy after > > changing GetRole() to return a ui::AXRole. Is adding the default case to the > > ones that only handle a few roles OK, or if we should be explicitly listing > all > > roles? > > what you've done here lg imo, but I don't know much about android a11y Thanks both :) https://codereview.chromium.org/2671563002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility_android.cc:633: // No role description. On 2017/02/09 23:21:09, dmazzoni wrote: > We should add this one, it's a relatively new addition to ARIA. > > Make this a TODO? Done, also followed this up in https://codereview.chromium.org/2690473002/. https://codereview.chromium.org/2671563002/diff/140001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2671563002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:325: // are already implemented in - accessibilitySetValue:forAttribute:, On 2017/02/09 23:25:05, tapted wrote: > nit remove space between "- accessibilitySetValue" Done.
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 patricialor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2671563002/#ps160001 (title: "Add TODO for "feed" ARIA role.")
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": 160001, "attempt_start_ts": 1486692977135280,
"parent_rev": "6ec79a392f9a632e2d4939fc2a75b56dc9c9a481", "commit_rev":
"e8f8ea9d2076848d998001dea1e7b90d7beb83a8"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement NSAccessibilityPressAction for Views with clickable roles. Currently, no NSAccessibilityActions are supported for MacViews. Refactor out code supporting this for clickable controls inside Blink and re-use the code for clickable Views. BUG=679247 ========== to ========== MacViews: Implement NSAccessibilityPressAction for Views with clickable roles. Currently, no NSAccessibilityActions are supported for MacViews. Refactor out code supporting this for clickable controls inside Blink and re-use the code for clickable Views. BUG=679247 Review-Url: https://codereview.chromium.org/2671563002 Cr-Commit-Position: refs/heads/master@{#449519} Committed: https://chromium.googlesource.com/chromium/src/+/e8f8ea9d2076848d998001dea1e7... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/e8f8ea9d2076848d998001dea1e7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
