Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2944083004:
MacViews a11y: Support the "Show menu" action in Textfield and Combobox. (Closed)
|
Created:
3 years, 6 months ago by tapted Modified:
3 years, 5 months ago CC:
chromium-reviews, aboxhall+watch_chromium.org, tfarina, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dougt+watch_chromium.org, aleventhal+watch_chromium.org, dtseng+watch_chromium.org, mac-reviews_chromium.org, dmazzoni+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews a11y: Support the "Show menu" action in Textfield and Combobox.
Currently Combobox and Textfield show no actions available to
accessibility.
For Mac, fix by mapping the default action (a click) to
NSAccessibilityPressAction and map AX_ACTION_SHOW_CONTEXT_MENU to
NSAccessibilityShowMenuAction. For comboboxes, both
NSAccessibilityPressAction and NSAccessibilityShowMenuAction show a
menu. For textfields, just the latter.
On Mac, automatically add and handle NSAccessibilityShowMenuAction for
controls that show a menu on press and don't have a separate context
menu.
For Textfields (and other controls using context menus), automatically
add ui::AX_ACTION_SHOW_CONTEXT_MENU for views that have a context menu
controller.
Comboboxes are trickier since the View that usually handles clicks is a
"TransparentButton". So handle these actions explicitly.
For menus triggered by a11y actions, there may be no mouse event. Cocoa
needs one to position the menu, so generate a dummy event for this.
BUG=679247, 663536, 732655
Review-Url: https://codereview.chromium.org/2944083004
Cr-Commit-Position: refs/heads/master@{#483939}
Committed: https://chromium.googlesource.com/chromium/src/+/926329391ace2c7470f55b275988a09f4f1fd06b
Patch Set 1 #Patch Set 2 : AXRaise? #Patch Set 3 : Fix menu location #Patch Set 4 : More coverage #Patch Set 5 : base off crrev/2946783003 #
Total comments: 5
Patch Set 6 : rebase to master #Patch Set 7 : Map -> Vector.. #Patch Set 8 : rebase #Patch Set 9 : Mac-specific #
Total comments: 14
Patch Set 10 : respond to comments #
Total comments: 2
Patch Set 11 : more interesting test #Messages
Total messages: 58 (41 generated)
Description was changed from ========== Automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for Views with a context_menu_controller BUG=679247 ========== to ========== Automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for Views with a context_menu_controller BUG=679247, 663536, 732655 ==========
The CQ bit was checked by tapted@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 checked by tapted@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 tapted@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 checked by tapted@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...
Description was changed from ========== Automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for Views with a context_menu_controller BUG=679247, 663536, 732655 ========== to ========== MacViews a11y: Support the "Show menu" action in Textfield and Combobox. Currently Combobox and Textfield show no actions available to accessibility. On Mac, the default action (a click) is mapped to NSAccessibilityPressAction and AX_ACTION_SHOW_CONTEXT_MENU is mapped to NSAccessibilityShowMenuAction. For comboboxes, both of these show a menu. For textfields, just the latter. For Textfields (and other controls using context menus), automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for views that have a context menu controller. Comboboxes are trickier since the View that usually handles clicks is a "TransparentButton". So handle these actions explicitly. For menus triggered by a11y actions, there may be no mouse event. Cocoa needs one to position the menu, so generate a dummy event for this. BUG=679247, 663536, 732655 ==========
tapted@chromium.org changed reviewers: + dmazzoni@chromium.org
The CQ bit was checked by tapted@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...
Description was changed from ========== MacViews a11y: Support the "Show menu" action in Textfield and Combobox. Currently Combobox and Textfield show no actions available to accessibility. On Mac, the default action (a click) is mapped to NSAccessibilityPressAction and AX_ACTION_SHOW_CONTEXT_MENU is mapped to NSAccessibilityShowMenuAction. For comboboxes, both of these show a menu. For textfields, just the latter. For Textfields (and other controls using context menus), automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for views that have a context menu controller. Comboboxes are trickier since the View that usually handles clicks is a "TransparentButton". So handle these actions explicitly. For menus triggered by a11y actions, there may be no mouse event. Cocoa needs one to position the menu, so generate a dummy event for this. BUG=679247, 663536, 732655 ========== to ========== MacViews a11y: Support the "Show menu" action in Textfield and Combobox. Currently Combobox and Textfield show no actions available to accessibility. For Mac, fix by mapping the default action (a click) to NSAccessibilityPressAction and AX_ACTION_SHOW_CONTEXT_MENU to NSAccessibilityShowMenuAction. For comboboxes, both of these show a menu. For textfields, just the latter. For Textfields (and other controls using context menus), automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for views that have a context menu controller. Comboboxes are trickier since the View that usually handles clicks is a "TransparentButton". So handle these actions explicitly. For menus triggered by a11y actions, there may be no mouse event. Cocoa needs one to position the menu, so generate a dummy event for this. BUG=679247, 663536, 732655 ==========
Hi Dominic, could you take a first look? I'll run this by another ui/views OWNER too. Note this is branched off crrev/2946783003.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Should the default action for a text field maybe be focus instead of click?
https://codereview.chromium.org/2944083004/diff/80001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2944083004/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:345: for (const ActionMap::value_type& entry : GetActionMap()) { It doesn't look like you're ever using ActionMap as a map, i.e. looking up a key and getting a value. If you reversed it, you could look up the Mac action and retrieve the Chrome action. Alternatively, if you made it a vector of pairs, you could just enforce that kActionForPress is first and not have to have any special cases in accessibilityActionNames https://codereview.chromium.org/2944083004/diff/80001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2944083004/diff/80001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:777: action_data.action == ui::AX_ACTION_SHOW_CONTEXT_MENU)) { Wait, so the SHOW_CONTEXT_MENU action also drops down the menu for a combo box? That doesn't sound right. It should do the same thing as right-clicking, which is probably nothing. (For a text field, it should open a menu with things like Cut, Copy, Paste.)
On 2017/06/21 05:59:27, dmazzoni wrote: > Should the default action for a text field maybe be focus > instead of click? On Mac it does "confirm" for most text fields (e.g. presses Enter). For multiline textfields it doesn't focus or click - VO+Space seems to do the same things as VO+Down. Perhaps I can explore this in a follow-up, since I'm not touching textfield directly in this CL :) https://codereview.chromium.org/2944083004/diff/80001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2944083004/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:345: for (const ActionMap::value_type& entry : GetActionMap()) { On 2017/06/21 06:09:35, dmazzoni wrote: > It doesn't look like you're ever using ActionMap as a map, i.e. > looking up a key and getting a value. > > If you reversed it, you could look up the Mac action and retrieve > the Chrome action. > > Alternatively, if you made it a vector of pairs, you could just enforce > that kActionForPress is first and not have to have any special cases > in accessibilityActionNames Indeed! good catch. and it's as easy as changing the typedef (I had tried, but was on the fence). I settled for a map in case we wanted a +(NSString*)nativeActionFromAXAction:(ui::AXAction)action; to go with the others in future. I had that on earlier CLs but I couldn't find a use for that now in browser_accessibility_mac so deleted it. I'll put up a CL using vector, and with the press action first. https://codereview.chromium.org/2944083004/diff/80001/ui/views/controls/combo... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2944083004/diff/80001/ui/views/controls/combo... ui/views/controls/combobox/combobox.cc:777: action_data.action == ui::AX_ACTION_SHOW_CONTEXT_MENU)) { On 2017/06/21 06:09:35, dmazzoni wrote: > Wait, so the SHOW_CONTEXT_MENU action also drops down the menu for a > combo box? That doesn't sound right. It should do the same thing as > right-clicking, which is probably nothing. Cocoa offers both "press" and "show menu" actions for combox/popup menus. press is the default. "show menu" also shows the menu (although positioned differently). Cocoa doesn't use the word "context" menu, but there isn't a separate AX enum for a non-context menu. I'm not sure what approach is. Renaming AX_SHOW_CONTEXT_MENU to AX_SHOW_MENU might be bad (e.g. menu-like items in the bookmarks toolbar can expand into submenus and also expose a right-click menu). I could just leave off SHOW_CONTEXT_MENU for now, and explore in a follow-up. Or only handle add it above between #if mac. > > (For a text field, it should open a menu with things like Cut, Copy, Paste.) yup - this CL achieves this for text field.
https://codereview.chromium.org/2944083004/diff/80001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2944083004/diff/80001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:345: for (const ActionMap::value_type& entry : GetActionMap()) { On 2017/06/21 06:52:55, tapted wrote: > On 2017/06/21 06:09:35, dmazzoni wrote: > > It doesn't look like you're ever using ActionMap as a map, i.e. > > looking up a key and getting a value. > > > > If you reversed it, you could look up the Mac action and retrieve > > the Chrome action. > > > > Alternatively, if you made it a vector of pairs, you could just enforce > > that kActionForPress is first and not have to have any special cases > > in accessibilityActionNames > > Indeed! good catch. and it's as easy as changing the typedef (I had tried, but > was on the fence). I settled for a map in case we wanted a > +(NSString*)nativeActionFromAXAction:(ui::AXAction)action; to go with the others > in future. I had that on earlier CLs but I couldn't find a use for that now in > browser_accessibility_mac so deleted it. I'll put up a CL using vector, and with > the press action first. Well.. everything _compiled_ fine with just the typedef change, but it needed a bit more. Not as elegant as I was hoping - happy to try other things.
dmazzoni: ping? The main question is around whether SHOW_CONTEXT_MENU should show the Combobox menu. On mac "show menu" does appear and does show a menu. But there is no SHOW_MENU AXAction enum. The press/default action also appears. If we want that part to be mac-specific we could secretly add NSAccessibilityShowMenuAction for any AX_ROLE_POP_UP_BUTTON and map it back to ui::AX_ACTION_DO_DEFAULT *IF* the AX_ROLE_POP_UP_BUTTON doesn't *also* have its own SHOW_CONTEXT_MENU. E.g. extension action buttons on the toolbar are AX_ROLE_POP_UP_BUTTON and they have both press actions and right-click menus. So we can't assume NSAccessibilityShowMenuAction is always a press for AX_ROLE_POP_UP_BUTTON. Or we could diverge from what native Cocoa controls do and just skip revealing the "show menu" action. WDYT?
On 2017/06/23 01:03:18, tapted wrote: > dmazzoni: ping? > > The main question is around whether SHOW_CONTEXT_MENU should show the Combobox > menu. > > On mac "show menu" does appear and does show a menu. But there is no SHOW_MENU > AXAction enum. The press/default action also appears. > > If we want that part to be mac-specific we could secretly add > NSAccessibilityShowMenuAction for any AX_ROLE_POP_UP_BUTTON and map it back to > ui::AX_ACTION_DO_DEFAULT *IF* the AX_ROLE_POP_UP_BUTTON doesn't *also* have its > own SHOW_CONTEXT_MENU. E.g. extension action buttons on the toolbar are > AX_ROLE_POP_UP_BUTTON and they have both press actions and right-click menus. So > we can't assume NSAccessibilityShowMenuAction is always a press for > AX_ROLE_POP_UP_BUTTON. > > Or we could diverge from what native Cocoa controls do and just skip revealing > the "show menu" action. > > WDYT? OK, I played with this a bit more. In VoiceOver you can trigger "Show Menu" with VO+Shift+M. In a native menu button, both VO+Space and VO+Shift+M open the menu, but they don't behave quite identically. VO+Space focuses the current selected item, while VO+Shift+M opens the menu without focusing anything. In Safari, VO+Shift+M always opens the page context menu, which I think makes sense because it should be available on any element. I think that's how Chrome already behaves. My preference would be to implement support for AXShowMenuAction for MacViews, but to make it more like a Mac-specific alias for "do default action" that we expose only for this particular role. Alternatively I'd be fine with adding a new internal action enum for "show menu", if you wanted to try to emulate the Mac behavior of opening the menu without focusing the current selected item. The main thing I don't want to do is conflate "open context menu" with "open menu" in our internal enums and methods.
The CQ bit was checked by tapted@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.
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by tapted@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...
Description was changed from ========== MacViews a11y: Support the "Show menu" action in Textfield and Combobox. Currently Combobox and Textfield show no actions available to accessibility. For Mac, fix by mapping the default action (a click) to NSAccessibilityPressAction and AX_ACTION_SHOW_CONTEXT_MENU to NSAccessibilityShowMenuAction. For comboboxes, both of these show a menu. For textfields, just the latter. For Textfields (and other controls using context menus), automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for views that have a context menu controller. Comboboxes are trickier since the View that usually handles clicks is a "TransparentButton". So handle these actions explicitly. For menus triggered by a11y actions, there may be no mouse event. Cocoa needs one to position the menu, so generate a dummy event for this. BUG=679247, 663536, 732655 ========== to ========== MacViews a11y: Support the "Show menu" action in Textfield and Combobox. Currently Combobox and Textfield show no actions available to accessibility. For Mac, fix by mapping the default action (a click) to NSAccessibilityPressAction and map AX_ACTION_SHOW_CONTEXT_MENU to NSAccessibilityShowMenuAction. For comboboxes, both NSAccessibilityPressAction and NSAccessibilityShowMenuAction show a menu. For textfields, just the latter. On Mac, automatically add and handle NSAccessibilityShowMenuAction for controls that show a menu on press and don't have a separate context menu. For Textfields (and other controls using context menus), automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for views that have a context menu controller. Comboboxes are trickier since the View that usually handles clicks is a "TransparentButton". So handle these actions explicitly. For menus triggered by a11y actions, there may be no mouse event. Cocoa needs one to position the menu, so generate a dummy event for this. BUG=679247, 663536, 732655 ==========
On 2017/06/23 19:17:24, dmazzoni wrote: > On 2017/06/23 01:03:18, tapted wrote: > > dmazzoni: ping? > > > > The main question is around whether SHOW_CONTEXT_MENU should show the Combobox > > menu. > > > > On mac "show menu" does appear and does show a menu. But there is no SHOW_MENU > > AXAction enum. The press/default action also appears. > > > > If we want that part to be mac-specific we could secretly add > > NSAccessibilityShowMenuAction for any AX_ROLE_POP_UP_BUTTON and map it back to > > ui::AX_ACTION_DO_DEFAULT *IF* the AX_ROLE_POP_UP_BUTTON doesn't *also* have > its > > own SHOW_CONTEXT_MENU. E.g. extension action buttons on the toolbar are > > AX_ROLE_POP_UP_BUTTON and they have both press actions and right-click menus. > So > > we can't assume NSAccessibilityShowMenuAction is always a press for > > AX_ROLE_POP_UP_BUTTON. > > > > Or we could diverge from what native Cocoa controls do and just skip revealing > > the "show menu" action. > > > > WDYT? > > OK, I played with this a bit more. > > In VoiceOver you can trigger "Show Menu" with VO+Shift+M. > > In a native menu button, both VO+Space and VO+Shift+M open the menu, but they > don't > behave quite identically. VO+Space focuses the current selected item, while > VO+Shift+M opens the menu without focusing anything. > > In Safari, VO+Shift+M always opens the page context menu, which I think makes > sense because it should be available on any element. I think that's how Chrome > already behaves. > > My preference would be to implement support for AXShowMenuAction for MacViews, > but to make it more like a Mac-specific alias for "do default action" that we > expose only for this particular role. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Please note that in Voiceover, to get the list of actions that an item supports press Ctrl-Option-Cmd-Space. At some point I'd like us to sort them in order of priority as well. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
tapted@chromium.org changed reviewers: + msw@chromium.org
+msw - mainly to check whether it looks as though I'm holding ui/views/controls/combobox right - thanks!
Mostly lg with some minor nits/qs https://codereview.chromium.org/2944083004/diff/180001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2944083004/diff/180001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:342: for (size_t i = 0; i < action_list.size(); ++i) { optional nit: for (const ui::AXAction action : action_list) https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox.cc:772: // for Combobox are handled by |arrow_button_|. Handle the action explicitly. nit: It's not clear from this comment why allowing |arrow_button_| to handle the default generated mouse event would be problematic. Can you rephrase this comment to explain why we handle the action explicitly? ie. "Handle the default action explicitly by showing the menu, otherwise |arrow_button_| would handle the event by <doing xyz>." https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox.cc:773: if (enabled() && action_data.action == ui::AX_ACTION_DO_DEFAULT) { Do we need some logic for handling the event when the menu is already showing? https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox_unittest.cc:655: EXPECT_EQ(1, menu_show_count_); nit: test handling the action while the menu is showing? https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox_unittest.cc:661: data.action = ui::AX_ACTION_BLUR; Should this also test AX_ACTION_SHOW_CONTEXT_MENU, and increment/decrement? https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox_unittest.cc:662: combobox_->HandleAccessibleAction(data); Maybe test other actions, like blur, before disabling the combobox? https://codereview.chromium.org/2944083004/diff/180001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2944083004/diff/180001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:743: TestComboboxModel() {} optional nit: = default;
The CQ bit was checked by tapted@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/2944083004/diff/180001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2944083004/diff/180001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:342: for (size_t i = 0; i < action_list.size(); ++i) { On 2017/06/29 19:26:11, msw wrote: > optional nit: for (const ui::AXAction action : action_list) Done (although with for (const auto item : action_list), since it's a pair) https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... File ui/views/controls/combobox/combobox.cc (right): https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox.cc:772: // for Combobox are handled by |arrow_button_|. Handle the action explicitly. On 2017/06/29 19:26:12, msw wrote: > nit: It's not clear from this comment why allowing |arrow_button_| to handle the > default generated mouse event would be problematic. Can you rephrase this > comment to explain why we handle the action explicitly? ie. "Handle the default > action explicitly by showing the menu, otherwise |arrow_button_| would handle > the event by <doing xyz>." The problem is that |arrow_button_| isn't exposed to a11y -- it's an implementation detail, so hidden from the a11y tree. Updated the comment: // The action handling in View would generate a mouse event and send it to // |this|. However, mouse events for Combobox are handled by |arrow_button_|, // which is hidden from the a11y tree (so can't expose actions). Rather than // forwarding AX_ACTION_DO_DEFAULT to View and then forwarding the mouse event // it generates to |arrow_button_| to have it forward back to |this| (as its // ButtonListener), just handle the action explicitly here and bypass View. https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox.cc:773: if (enabled() && action_data.action == ui::AX_ACTION_DO_DEFAULT) { On 2017/06/29 19:26:12, msw wrote: > Do we need some logic for handling the event when the menu is already showing? I don't think so. I am not a VoiceOver expert but it seems to be impossible to enter a situation where this is needed. Even with voiceover navigation set to "Mouse pointer -> follows-mouse-cursor" it's impossible for the voiceover cursor to "escape" a menu while it's still open -- the menu must be dismissed first (and so there is no way to perform an action on something other than a menu item while any menu is showing). https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox_unittest.cc:655: EXPECT_EQ(1, menu_show_count_); On 2017/06/29 19:26:12, msw wrote: > nit: test handling the action while the menu is showing? doing this requires an interactive_ui_test (menus are tricksy). Combobox::ShowDropDownMenu() has handling for ensuring the old menu is closed for the non-accessible actions. https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox_unittest.cc:661: data.action = ui::AX_ACTION_BLUR; On 2017/06/29 19:26:12, msw wrote: > Should this also test AX_ACTION_SHOW_CONTEXT_MENU, and increment/decrement? AX_ACTION_SHOW_CONTEXT_MENU was actually tested in earlier patchsets, but we decided context menus should not be a concept that views comboboxes should handle -- it should do nothing. (For <select> elements in webcontent AX_ACTION_SHOW_CONTEXT_MENU should show the regular page context menu.) Except, for Mac, we want to expose NSAccessibilityShowMenuAction since it's not exclusively used for context menus on native controls. (if this is a bug on other platforms we can explore in a follow-up) I guess it doesn't hurt to send AX_ACTION_SHOW_CONTEXT_MENU anyway, and verify there's no change (done). However, a control doesn't usually expect to see requests to handle actions it didn't first expose in its AXNodeData. increment/decrement aren't exposed as actions on native Cocoa controls, so I haven't added handling for it. https://codereview.chromium.org/2944083004/diff/180001/ui/views/controls/comb... ui/views/controls/combobox/combobox_unittest.cc:662: combobox_->HandleAccessibleAction(data); On 2017/06/29 19:26:12, msw wrote: > Maybe test other actions, like blur, before disabling the combobox? Done. https://codereview.chromium.org/2944083004/diff/180001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2944083004/diff/180001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:743: TestComboboxModel() {} On 2017/06/29 19:26:12, msw wrote: > optional nit: = default; Done.
lgtm with a test comment https://codereview.chromium.org/2944083004/diff/200001/ui/views/controls/comb... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2944083004/diff/200001/ui/views/controls/comb... ui/views/controls/combobox/combobox_unittest.cc:671: data.action = ui::AX_ACTION_BLUR; Change this to data.action = ui::AX_ACTION_DO_DEFAULT; (it's already blur)
The CQ bit was checked by tapted@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.
Thanks all! https://codereview.chromium.org/2944083004/diff/200001/ui/views/controls/comb... File ui/views/controls/combobox/combobox_unittest.cc (right): https://codereview.chromium.org/2944083004/diff/200001/ui/views/controls/comb... ui/views/controls/combobox/combobox_unittest.cc:671: data.action = ui::AX_ACTION_BLUR; On 2017/06/30 15:40:07, msw wrote: > Change this to data.action = ui::AX_ACTION_DO_DEFAULT; (it's already blur) Done.
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2944083004/#ps220001 (title: "more interesting test")
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": 220001, "attempt_start_ts": 1499055168104460,
"parent_rev": "98e223c3c437db7740a771918a69869a91d79f91", "commit_rev":
"926329391ace2c7470f55b275988a09f4f1fd06b"}
Message was sent while issue was closed.
Description was changed from ========== MacViews a11y: Support the "Show menu" action in Textfield and Combobox. Currently Combobox and Textfield show no actions available to accessibility. For Mac, fix by mapping the default action (a click) to NSAccessibilityPressAction and map AX_ACTION_SHOW_CONTEXT_MENU to NSAccessibilityShowMenuAction. For comboboxes, both NSAccessibilityPressAction and NSAccessibilityShowMenuAction show a menu. For textfields, just the latter. On Mac, automatically add and handle NSAccessibilityShowMenuAction for controls that show a menu on press and don't have a separate context menu. For Textfields (and other controls using context menus), automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for views that have a context menu controller. Comboboxes are trickier since the View that usually handles clicks is a "TransparentButton". So handle these actions explicitly. For menus triggered by a11y actions, there may be no mouse event. Cocoa needs one to position the menu, so generate a dummy event for this. BUG=679247, 663536, 732655 ========== to ========== MacViews a11y: Support the "Show menu" action in Textfield and Combobox. Currently Combobox and Textfield show no actions available to accessibility. For Mac, fix by mapping the default action (a click) to NSAccessibilityPressAction and map AX_ACTION_SHOW_CONTEXT_MENU to NSAccessibilityShowMenuAction. For comboboxes, both NSAccessibilityPressAction and NSAccessibilityShowMenuAction show a menu. For textfields, just the latter. On Mac, automatically add and handle NSAccessibilityShowMenuAction for controls that show a menu on press and don't have a separate context menu. For Textfields (and other controls using context menus), automatically add ui::AX_ACTION_SHOW_CONTEXT_MENU for views that have a context menu controller. Comboboxes are trickier since the View that usually handles clicks is a "TransparentButton". So handle these actions explicitly. For menus triggered by a11y actions, there may be no mouse event. Cocoa needs one to position the menu, so generate a dummy event for this. BUG=679247, 663536, 732655 Review-Url: https://codereview.chromium.org/2944083004 Cr-Commit-Position: refs/heads/master@{#483939} Committed: https://chromium.googlesource.com/chromium/src/+/926329391ace2c7470f55b275988... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/926329391ace2c7470f55b275988... |
