Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578303003:
a11y: Add a11y information to views::Tab and manually ignore its a11y children. (Closed)
Descriptiona11y: Add a11y information to views::Tab and manually ignore its a11y children.
Currently, Views tabs have no a11y information specific to their class. The
fallback then is to use the views::Label inside them for a11y information
instead. This means they report their role as text. Fix this by adding a11y
information to views::Tab to report its role as ui::AX_ROLE_TAB, and handle the
ui::AX_ACTION_SET_SELECTED action and plumb through for Mac.
views::Tab is also a special case where only the selected tab is allowed to be
keyboard focusable. Because of this behavior, the views::Label inside is exposed
to the accessibility tree. Since views::Tab now has its own a11y information,
the views::Label should be hidden from users (see Issue 610589). Make a class
views::TabLabel to manually set its a11y role to ui::AX_ROLE_IGNORED.
BUG=657593, 610589
Review-Url: https://codereview.chromium.org/2578303003
Cr-Commit-Position: refs/heads/master@{#443837}
Committed: https://chromium.googlesource.com/chromium/src/+/dc7d397153bcbccbc21e12f2ea3568f594699220
Patch Set 1 #Patch Set 2 : Move a11y info to views::Tab instead of views::MdTab and manually ignore Label children. #Patch Set 3 : Rebase back onto origin/master. #
Total comments: 7
Patch Set 4 : Mimic NSTabViewItem value behaviour and set NSAccessibilitySelectedAttribute (not yet user-writable… #Patch Set 5 : Review comments allow writing tab selected state. #Patch Set 6 : Typo #Patch Set 7 : Fix DumpAccessibilityTreeTest.AccessibilityAriaTabPanel. #
Total comments: 24
Patch Set 8 : Review comments + move IDS_ACCNAME_TAB to ui/resources from chrome/app. #Patch Set 9 : Cleanup. #Patch Set 10 : Add tests. #
Total comments: 54
Patch Set 11 : Review comments. #
Total comments: 44
Patch Set 12 : Review comments. #
Total comments: 7
Patch Set 13 : Review comments. #
Total comments: 2
Patch Set 14 : Revert AXTab subrole. #Patch Set 15 : Rebase #Messages
Total messages: 100 (84 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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...
Description was changed from ========== a11y: Add a11y information to views::Tab and fix up its FocusBehavior. Currently, Views tabs have no a11y information specific to their class, which means the label used inside them are used for a11y information instead. This means they report their role as text. This change reports their role as ui::AX_ROLE_TAB. Also fix up their FocusBehavior to stay the same depending on whether the tab is currently selected or not - FocusBehavior should be reporting whether it is possible to have keyboard focus on that view, not whether it currently does or not. This will also fix the views::Label inside the views::Tab being exposed to accessibility clients when https://www.crrev.com/2119413004 is landed. BUG=657593, 610589 ========== to ========== a11y: Add a11y information to views::Tab and manually ignore its a11y children. Currently, Views tabs have no a11y information specific to their class. The fallback then is to use the views::Label inside them for a11y information instead. This means they report their role as text. Fix this by adding a11y information to views::Tab to report its role as ui::AX_ROLE_TAB. views::Tab is also a special case where only the selected tab is allowed to be keyboard focusable. Because of this behavior, the views::Label inside is exposed to the accessibility tree. Since views::Tab now has its own a11y information, the views::Label should be hidden from users (see Issue 610589). Make a class views::IgnoredLabel to manually set its a11y role to ui::AX_ROLE_IGNORED. BUG=657593, 610589 ==========
Description was changed from ========== a11y: Add a11y information to views::Tab and manually ignore its a11y children. Currently, Views tabs have no a11y information specific to their class. The fallback then is to use the views::Label inside them for a11y information instead. This means they report their role as text. Fix this by adding a11y information to views::Tab to report its role as ui::AX_ROLE_TAB. views::Tab is also a special case where only the selected tab is allowed to be keyboard focusable. Because of this behavior, the views::Label inside is exposed to the accessibility tree. Since views::Tab now has its own a11y information, the views::Label should be hidden from users (see Issue 610589). Make a class views::IgnoredLabel to manually set its a11y role to ui::AX_ROLE_IGNORED. BUG=657593, 610589 ========== to ========== a11y: Add a11y information to views::Tab and manually ignore its a11y children. Currently, Views tabs have no a11y information specific to their class. The fallback then is to use the views::Label inside them for a11y information instead. This means they report their role as text. Fix this by adding a11y information to views::Tab to report its role as ui::AX_ROLE_TAB. views::Tab is also a special case where only the selected tab is allowed to be keyboard focusable. Because of this behavior, the views::Label inside is exposed to the accessibility tree. Since views::Tab now has its own a11y information, the views::Label should be hidden from users (see Issue 610589). Make a class views::IgnoredLabel to manually set its a11y role to ui::AX_ROLE_IGNORED. BUG=657593, 610589 ==========
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 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 #3 (id:40001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hi Trent, PTAL. Combined with https://codereview.chromium.org/2119413004/ (Ignored a11y elements), it works (i.e., views::Label inside are ignored). Trybots pass (see patchset 2). https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:52: class IgnoredLabel : public Label { Alternatively, we could add a method onto NativeViewAccessibility that sets a flag saying an a11y element should be ignored. But I'm not sure if we'll need to use that for any other cases than this, so not sure if it's that useful. Which option do you prefer?
https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:52: class IgnoredLabel : public Label { On 2016/12/22 00:20:30, Patti Lor wrote: > Alternatively, we could add a method onto NativeViewAccessibility that sets a > flag saying an a11y element should be ignored. But I'm not sure if we'll need to > use that for any other cases than this, so not sure if it's that useful. Which > option do you prefer? This looks good. https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:54: IgnoredLabel(const base::string16& text, const gfx::FontList& font_list) Can we call this TabLabel? And move the font_list initializer here. i.e. Do explicit TabLabel(const std::string& tab_title) : Label(tab_title, ui::ResourceBundle.... ) {} https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:260: } Are there more things we should add that NSSegmentedControl reports? E.g. AX_STATE_CHECKED? AX_STATE_DISABLED (when !enabled()) or AX_STATE_PRESSED (when the active tab), AX_SUPPORTED_ACTION_PRESS
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 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 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...
Patchset #5 (id:100001) has been deleted
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: 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:52: class IgnoredLabel : public Label { On 2016/12/22 00:50:09, tapted wrote: > On 2016/12/22 00:20:30, Patti Lor wrote: > > Alternatively, we could add a method onto NativeViewAccessibility that sets a > > flag saying an a11y element should be ignored. But I'm not sure if we'll need > to > > use that for any other cases than this, so not sure if it's that useful. Which > > option do you prefer? > > This looks good. Ok, thanks! :) https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:54: IgnoredLabel(const base::string16& text, const gfx::FontList& font_list) On 2016/12/22 00:50:08, tapted wrote: > Can we call this TabLabel? And move the font_list initializer here. i.e. Do > > explicit TabLabel(const std::string& tab_title) > : Label(tab_title, ui::ResourceBundle.... ) {} Done. https://codereview.chromium.org/2578303003/diff/60001/ui/views/controls/tabbe... ui/views/controls/tabbed_pane/tabbed_pane.cc:260: } On 2016/12/22 00:50:08, tapted wrote: > Are there more things we should add that NSSegmentedControl reports? > > E.g. AX_STATE_CHECKED? AX_STATE_DISABLED (when !enabled()) or AX_STATE_PRESSED > (when the active tab), AX_SUPPORTED_ACTION_PRESS Good point - I've added ui::AX_STATE_SELECTED => AXSelected / NSAccessibilitySelectedAttribute, but it makes no difference to VoiceOver (the Cocoa version says "selected" when on a selected tab), and accessibilitySelectedAttribute doesn't show up in the Accessibility Inspector (checked Sierra + El Capitan). I've checked all the other NSAccessibility attributes, and doesn't seem to be a similar NSAccessibility attribute that looks like I can use to say "this tab of radio button role is already selected" to get VO to behave properly. :( Instead of AXSelectedAttribute, Cocoa is using AXValue to show whether the tab is selected (it shows 0 or 1), which I have copied. Cocoa's version also doesn't allow you to edit the AXValue of a tab (they let you click one using "NSAccessibilityPressAction", but we don't have any NSAccessibility actions implemented for Views atm), but I've deviated from that and implemented it for unselected tabs (so trying to edit the AXValue is always going to select the tab). I'm not sure if that makes sense though, so let me know if you think we should copy Cocoa and just not allow AXValue of tabs to be editable / wait for Views NSAccessibility actions. As for AX_STATE_PRESSED, I think it's only for when the mouse is depressed on a button and hasn't mouse upped yet. And AX_STATE_DISABLED is set by NativeViewAccessibility::GetData() :D https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:488: return l10n_util::GetNSStringWithFixup(IDS_ACCNAME_TAB); There's no "NSAccessibilityTabRole" or similar, so using the same approach as chrome/browser/ui/cocoa/tabs/tab_view.mm and setting role description manually. https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:578: - (NSNumber*)AXSelected { Not sure if we should leave this in; I can't tell if it does anything. It is definitely called, though. WDYT?
Can we add some tests? tabeed_pane_unittest.cc is probably the right place. Although we should have a mac-specific test with an NSTabView as well in case the AXRadioButton weirdness changes :/ https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/BUILD.gn File ui/accessibility/BUILD.gn (right): https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/BUILD... ui/accessibility/BUILD.gn:76: "//chrome/app:generated_resources", //ui can't depend on stuff in //chrome :( - but see below this might be easy to fix https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/DEPS File ui/accessibility/DEPS (right): https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/DEPS#... ui/accessibility/DEPS:2: "+chrome/grit", We can't add this -- see https://www.chromium.org/developers/design-documents/cookbook for an overview of layering in chrome (and below for possible fix) https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/DEPS#... ui/accessibility/DEPS:4: "+ui/base/l10n", (this is probably OK) https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:384: // Tabs are a special case. For whatever reason, The reason might be that in Cocoa, using role=AXRadioButton for NSTabView and NSSegmentedControl is a bit of a "hack". E.g. "value" maps logically to a radio button, but not so much to a tab. If I understand correctly, I think we can mention that. I.e. "Since tabs use the Radio Button role on Mac, the standard way to set them is via the value attributed rather than the selected attribute". https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:486: switch (role) { nit: switch (node_->GetData().role) { https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:488: return l10n_util::GetNSStringWithFixup(IDS_ACCNAME_TAB); Can we move IDS_ACCNAME_TAB to ui_resources.grd? I think that will fix the layering stuff. Also can we add a note to the case e.g. // There is no NSAccessibilityTabRole or similar (AXRadioButton is used instead). Do the same as NSTabView and put "tab" in the description. (that's assuming what I said there is correct :) https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:578: - (NSNumber*)AXSelected { On 2016/12/30 00:35:02, Patti Lor wrote: > Not sure if we should leave this in; I can't tell if it does anything. It is > definitely called, though. WDYT? This looks good. I think it's fine to add anything from AXAttributeConstants.h https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:48: // views::Tab shouldn't expose any of its children in the a11y tree. Instead, it nit: move comment into GetAccessibleNodeData. the comment here should just be something like "The View containing the text for each tab in the tab strip." https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:55: TabLabel(const base::string16& tab_title) nit; explicit https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:62: void GetAccessibleNodeData(ui::AXNodeData* data) override { nit: // Label: https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:269: if (!selected()) Can we just ignore requests to SET_SELECTION to 0/false? Then I would remove this check (just call SelectTab(this). The hpothetical risk is that if we have Tab(0) selected, something might decide to do a sequence like: Select(Tab(1)) && Unselect(Tab(0))
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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.
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...
Description was changed from ========== a11y: Add a11y information to views::Tab and manually ignore its a11y children. Currently, Views tabs have no a11y information specific to their class. The fallback then is to use the views::Label inside them for a11y information instead. This means they report their role as text. Fix this by adding a11y information to views::Tab to report its role as ui::AX_ROLE_TAB. views::Tab is also a special case where only the selected tab is allowed to be keyboard focusable. Because of this behavior, the views::Label inside is exposed to the accessibility tree. Since views::Tab now has its own a11y information, the views::Label should be hidden from users (see Issue 610589). Make a class views::IgnoredLabel to manually set its a11y role to ui::AX_ROLE_IGNORED. BUG=657593, 610589 ========== to ========== a11y: Add a11y information to views::Tab and manually ignore its a11y children. Currently, Views tabs have no a11y information specific to their class. The fallback then is to use the views::Label inside them for a11y information instead. This means they report their role as text. Fix this by adding a11y information to views::Tab to report its role as ui::AX_ROLE_TAB, and handle the ui::AX_ACTION_SET_SELECTED action and plumb through for Mac. views::Tab is also a special case where only the selected tab is allowed to be keyboard focusable. Because of this behavior, the views::Label inside is exposed to the accessibility tree. Since views::Tab now has its own a11y information, the views::Label should be hidden from users (see Issue 610589). Make a class views::TabLabel to manually set its a11y role to ui::AX_ROLE_IGNORED. BUG=657593, 610589 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the wait, added some tests for this as suggested. PTAL, thank you! https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/BUILD.gn File ui/accessibility/BUILD.gn (right): https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/BUILD... ui/accessibility/BUILD.gn:76: "//chrome/app:generated_resources", On 2017/01/03 00:29:27, tapted wrote: > //ui can't depend on stuff in //chrome :( - but see below this might be easy to > fix Acknowledged. https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/DEPS File ui/accessibility/DEPS (right): https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/DEPS#... ui/accessibility/DEPS:2: "+chrome/grit", On 2017/01/03 00:29:27, tapted wrote: > We can't add this -- see > https://www.chromium.org/developers/design-documents/cookbook for an overview of > layering in chrome (and below for possible fix) Thanks :) https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/DEPS#... ui/accessibility/DEPS:4: "+ui/base/l10n", On 2017/01/03 00:29:27, tapted wrote: > (this is probably OK) Acknowledged. https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:384: // Tabs are a special case. For whatever reason, On 2017/01/03 00:29:27, tapted wrote: > The reason might be that in Cocoa, using role=AXRadioButton for NSTabView and > NSSegmentedControl is a bit of a "hack". E.g. "value" maps logically to a radio > button, but not so much to a tab. If I understand correctly, I think we can > mention that. I.e. "Since tabs use the Radio Button role on Mac, the standard > way to set them is via the value attributed rather than the selected attribute". Done. https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:486: switch (role) { On 2017/01/03 00:29:27, tapted wrote: > nit: switch (node_->GetData().role) { Done. https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:488: return l10n_util::GetNSStringWithFixup(IDS_ACCNAME_TAB); On 2017/01/03 00:29:27, tapted wrote: > Can we move IDS_ACCNAME_TAB to ui_resources.grd? I think that will fix the > layering stuff. Also can we add a note to the case e.g. > > // There is no NSAccessibilityTabRole or similar (AXRadioButton is used > instead). Do the same as NSTabView and put "tab" in the description. > > (that's assuming what I said there is correct :) A recent patch (https://codereview.chromium.org/2518183002) conveniently added ui/strings/ui_strings.h to ui/accessibility/DEPS, so I moved it there instead, hopefully that's OK :) https://codereview.chromium.org/2578303003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:578: - (NSNumber*)AXSelected { On 2017/01/03 00:29:27, tapted wrote: > On 2016/12/30 00:35:02, Patti Lor wrote: > > Not sure if we should leave this in; I can't tell if it does anything. It is > > definitely called, though. WDYT? > > This looks good. I think it's fine to add anything from AXAttributeConstants.h Acknowledged. https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:48: // views::Tab shouldn't expose any of its children in the a11y tree. Instead, it On 2017/01/03 00:29:27, tapted wrote: > nit: move comment into GetAccessibleNodeData. the comment here should just be > something like "The View containing the text for each tab in the tab strip." Done. https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:55: TabLabel(const base::string16& tab_title) On 2017/01/03 00:29:27, tapted wrote: > nit; explicit Done. https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:62: void GetAccessibleNodeData(ui::AXNodeData* data) override { On 2017/01/03 00:29:27, tapted wrote: > nit: // Label: Done. https://codereview.chromium.org/2578303003/diff/160001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:269: if (!selected()) On 2017/01/03 00:29:27, tapted wrote: > Can we just ignore requests to SET_SELECTION to 0/false? Then I would remove > this check (just call SelectTab(this). > > The hpothetical risk is that if we have Tab(0) selected, something might decide > to do a sequence like: Select(Tab(1)) && Unselect(Tab(0)) Yep - so on Mac, I've done this by reporting selected tabs's AXValue as 'uneditable', so as long as the a11y client respects that the "if" here won't be needed. I mostly just left that in there kinda "just in case". Have deleted it now.
https://codereview.chromium.org/2578303003/diff/220001/ui/strings/ui_strings.grd File ui/strings/ui_strings.grd (right): https://codereview.chromium.org/2578303003/diff/220001/ui/strings/ui_strings.... ui/strings/ui_strings.grd:275: <message name="IDS_ACCNAME_TAB" desc="A generic description of a tab button's role" meaning="UI element"> I think this should be grouped with "Accessible name strings" on line 325. and for the name itself, we should probably update it to something more descriptive. E.g. IDS_ACCNAME_TAB_ROLE_DESCRIPTION https://codereview.chromium.org/2578303003/diff/220001/ui/views/BUILD.gn File ui/views/BUILD.gn (right): https://codereview.chromium.org/2578303003/diff/220001/ui/views/BUILD.gn#newc... ui/views/BUILD.gn:848: "controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm", nit: tabbed_pane_accessibility_mac_unittest.mm - this is closer to some other files, and subjects it to some existing regex/globs doing filtering https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (left): https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:68: Tab* GetTabAt(int index); huh. I guess this got removed at some point and the declaration was left in. To do it in the test files, it looks like we can do Tab* TestHarnessWithFriendAccess::GetTabAt(int index) { return static_cast<Tab*>(tabbed_pane_->tab_strip_->child_at(index)); } There's a comment on TabbedPane::tab_strip_ which suggests the static casting is "ok" https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (right): https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:63: friend class TabbedPaneMacAccessibilityTest; this should have a views::test:: namespace on it. It will need forward-declare as well (it currently gets a views:: one since it's implicitly declared here in a views { namespace) https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit: 2017 :) https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:29: View* tab1_contents = new View(); nit: move `new View()` into the AddTab calls - no need for these tabX_contents vars https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:36: tabbed_pane_->SelectTabAt(0); why not tab1_ = tabbed_pane_->GetTabAt(0); tab2_ = tabbed_pane_->GetTabAt(1); https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:54: id AttributeValueAtPoint(NSString* attribute, gfx::Point point) { nit: const-ref point https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:60: id A11yElementAtPoint(gfx::Point point) { const-ref https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:66: Widget* widget_; these should be protected: https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:68: Tab* tab1_; Actually I don't think we need these - can we add a helper method Tab* GetTabAt(int index) { return tabbed_pane_->GetTabAt(index); } https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:70: gfx::Point tab1_center_; same here, gfx::Point TabCenterPoint(int index) { return GetTabAt(index)->GetBoundsInScreen().CenterPoint(); } https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:78: TEST_F(TabbedPaneMacAccessibilityTest, Attributes) { Maybe Attributes -> AppKitAtributes -- i.e. something to indicate we are comparing against native controls. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:80: NSTabView* cocoa_tab_group = any `alloc` typically needs to go into a scoped_nsobject straight away (currently these are memory leaks). E.g. scoped_nsobject<NSTabView> cocoa_tab_group([[NSTabView alloc] init..]); https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:82: NSTabViewItem* cocoa_tab1 = [[NSTabViewItem alloc] init]; we'd need scoped_nsobject for these too, but instead of that I'd suggest something like NSArray* cocoa_tabs = @[ [[[NSTabViewItem alloc] init] autorelease], [[[NSTabViewItem alloc] init] autorelease], ]; Then you can add a loop below https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:88: [cocoa_tab_group selectFirstTabViewItem:nil]; was this necessary? perhaps either a comment // Cocoa always <does thing that requires us to select>. or EXPECT_EQ([cocoa_tab_group selectedTabViewItem], cocoa_tab0]); https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:93: AttributeValueAtPoint(NSAccessibilityRoleAttribute, tab1_center_)); we should probably guard against the possibility that these _both_ return nil unexpectedly. Perhaps a check in AttributeValueAtPoint that does EXPECT_NE(nil, value); before returning https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:104: // Cocoa uses an int, so convert it back into a NSString. I'm not sure the conversion step makes sense. I.e. If the types are different, why can't Views use an intValue as well? (could having different types cause problems for us?). Should [AXPlatformNodeCocoa AXValue] actually return an (NSValue*) rather than an (NSString*)? https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:105: EXPECT_NSEQ( We should check the actual value too e.g. for (int i : {0, 1}) { NSString* value = [cocoa_tab[i] accessibilityAttributeValue:NSAccessibilityValueAttribute] stringValue]; EXPECT_NSEQ(i ? @"0" : @"1", value); EXPECT_NSEQ(value, AttributeValueAtPoint(NSAccessibilityValueAttribute, GetTabAt(i))); } https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:159: } Can we also test the AXAction "press" - this seems to be what Cocoa makes available in NSTabView for changing tabs (maybe add this to the AppKitAttributes test case and compare with NSTabView behaviour) https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:39: base::string16 kTabTitle = ASCIIToUTF16("tab"); This will make a static initializer, which will get the CL reverted. Perhaps add an anonymous namespace that includes FixedSizeView, and have a helper method like base::string16 DefaultTabTitle() { return ASCIIToUTF16("tab"); } (or in the SelectTabWithAccessibleAction test, just add a local const base::string16 tab_title = ASCIIToUTF16("tab"); ) (note we only use kFooStyle naming for compile-time constants, so not all things --e.g. strings-- declared `const` should be kFooStyle) https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:41: typedef ViewsTestBase TabbedPaneTest; I think it's time to upgrade this to a proper test harness. then you can add a helper method instead of using `std::vector<Tab*> tab_list;` Also, we can remove all the FRIEND_TEST_ALL_PREFIXES in tabbed_pane.h and just friend the test harness https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:147: const int num_tabs = 2; constexpr int kNumTabs = 2; (constexpr helps guarantee that it is a compile-time constant, and is preferred when that's the case now) https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:149: for (int i = 0; i <= num_tabs; ++i) { this ends up making 3 tabs, which I see is intentional, but it's a bit confusing when the constant is `2`. We should make the constant 3 and update the code below https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:165: EXPECT_EQ(i == num_tabs, data.HasStateFlag(ui::AX_STATE_SELECTED)); e.g. i == kNumTabs - 1 https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:171: NativeViewAccessibility::Create(tab_list[0]) I think this is leaked. It's possible you are actually the first person to write a test for `AccessibilityPerformAction`, so it's good that we are fixing that. In think in another CL you're changing Create to return a std::unique_ptr, so that would resolve the leak if we want to wait for that. Alternatively, see things done in NativeViewAccessibilityTest A cleaner way to get at the NativeViewAccessibility to test things like AccessiblityPerformAction might be nice too... but I don't see a clear way to do that "nicely" in a way that is also cross-platform. Something to think about though. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:184: widget->Close(); nit: CloseNow()
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.
Thanks Trent, PTAL. https://codereview.chromium.org/2578303003/diff/220001/ui/strings/ui_strings.grd File ui/strings/ui_strings.grd (right): https://codereview.chromium.org/2578303003/diff/220001/ui/strings/ui_strings.... ui/strings/ui_strings.grd:275: <message name="IDS_ACCNAME_TAB" desc="A generic description of a tab button's role" meaning="UI element"> On 2017/01/06 04:25:11, tapted wrote: > I think this should be grouped with "Accessible name strings" on line 325. > > and for the name itself, we should probably update it to something more > descriptive. E.g. > > IDS_ACCNAME_TAB_ROLE_DESCRIPTION Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/BUILD.gn File ui/views/BUILD.gn (right): https://codereview.chromium.org/2578303003/diff/220001/ui/views/BUILD.gn#newc... ui/views/BUILD.gn:848: "controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm", On 2017/01/06 04:25:11, tapted wrote: > nit: tabbed_pane_accessibility_mac_unittest.mm - this is closer to some other > files, and subjects it to some existing regex/globs doing filtering Done & renamed the test to match. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (left): https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:68: Tab* GetTabAt(int index); On 2017/01/06 04:25:11, tapted wrote: > huh. I guess this got removed at some point and the declaration was left in. To > do it in the test files, it looks like we can do > > Tab* TestHarnessWithFriendAccess::GetTabAt(int index) { > return static_cast<Tab*>(tabbed_pane_->tab_strip_->child_at(index)); > } > > There's a comment on TabbedPane::tab_strip_ which suggests the static casting is > "ok" To do this, I also had to move the class declaration for TabStrip into tabbed_pane.h, let me know if that's not OK. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (right): https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:63: friend class TabbedPaneMacAccessibilityTest; On 2017/01/06 04:25:11, tapted wrote: > this should have a views::test:: namespace on it. It will need forward-declare > as well (it currently gets a views:: one since it's implicitly declared here in > a views { namespace) Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/06 04:25:12, tapted wrote: > nit: 2017 :) :O Thanks! https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:29: View* tab1_contents = new View(); On 2017/01/06 04:25:11, tapted wrote: > nit: move `new View()` into the AddTab calls - no need for these tabX_contents > vars Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:36: tabbed_pane_->SelectTabAt(0); On 2017/01/06 04:25:12, tapted wrote: > why not > > tab1_ = tabbed_pane_->GetTabAt(0); > tab2_ = tabbed_pane_->GetTabAt(1); Deleted as suggested below. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:54: id AttributeValueAtPoint(NSString* attribute, gfx::Point point) { On 2017/01/06 04:25:12, tapted wrote: > nit: const-ref point Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:60: id A11yElementAtPoint(gfx::Point point) { On 2017/01/06 04:25:12, tapted wrote: > const-ref Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:66: Widget* widget_; On 2017/01/06 04:25:12, tapted wrote: > these should be protected: Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:68: Tab* tab1_; On 2017/01/06 04:25:12, tapted wrote: > Actually I don't think we need these - can we add a helper method > > Tab* GetTabAt(int index) { > return tabbed_pane_->GetTabAt(index); > } Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:70: gfx::Point tab1_center_; On 2017/01/06 04:25:12, tapted wrote: > same here, > > gfx::Point TabCenterPoint(int index) { > return GetTabAt(index)->GetBoundsInScreen().CenterPoint(); > } Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:78: TEST_F(TabbedPaneMacAccessibilityTest, Attributes) { On 2017/01/06 04:25:11, tapted wrote: > Maybe Attributes -> AppKitAtributes -- i.e. something to indicate we are > comparing against native controls. Went for AttributesMatchAppKit, let me know if you think that makes sense. AppKitAttributes I think just sounds like "the Cocoa attributes". https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:80: NSTabView* cocoa_tab_group = On 2017/01/06 04:25:11, tapted wrote: > any `alloc` typically needs to go into a scoped_nsobject straight away > (currently these are memory leaks). E.g. > > scoped_nsobject<NSTabView> cocoa_tab_group([[NSTabView alloc] init..]); Used autorelease as suggested below. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:82: NSTabViewItem* cocoa_tab1 = [[NSTabViewItem alloc] init]; On 2017/01/06 04:25:12, tapted wrote: > we'd need scoped_nsobject for these too, but instead of that I'd suggest > something like > > NSArray* cocoa_tabs = @[ > [[[NSTabViewItem alloc] init] autorelease], > [[[NSTabViewItem alloc] init] autorelease], > ]; > > Then you can add a loop below Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:88: [cocoa_tab_group selectFirstTabViewItem:nil]; On 2017/01/06 04:25:12, tapted wrote: > was this necessary? perhaps either a comment > > // Cocoa always <does thing that requires us to select>. > > or > > EXPECT_EQ([cocoa_tab_group selectedTabViewItem], cocoa_tab0]); Oops, no - removed now. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:93: AttributeValueAtPoint(NSAccessibilityRoleAttribute, tab1_center_)); On 2017/01/06 04:25:12, tapted wrote: > we should probably guard against the possibility that these _both_ return nil > unexpectedly. Perhaps a check in AttributeValueAtPoint that does EXPECT_NE(nil, > value); before returning Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:104: // Cocoa uses an int, so convert it back into a NSString. On 2017/01/06 04:25:12, tapted wrote: > I'm not sure the conversion step makes sense. I.e. If the types are different, > why can't Views use an intValue as well? (could having different types cause > problems for us?). > > Should [AXPlatformNodeCocoa AXValue] actually return an (NSValue*) rather than > an (NSString*)? Ooh, thanks for mentioning this. I'd already checked this wondering how this was possible, but I must have got mixed up between the type of "NSAccessibilityValueAttribute" and the type of object this attribute is meant to be associated with it - the header file says APPKIT_EXTERN NSString *const NSAccessibilityValueAttribute; //(id) - element's value So this is meant to be attached to an id type (turns out it says so in the online docs too). I've fixed this here for AXValue in ax_platform_node_mac.mm; but there are some other ones that are wrong. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=678898 to fix the others in a separate CL. Anyway, the converting to int and to string stuff is gone now :) https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:105: EXPECT_NSEQ( On 2017/01/06 04:25:12, tapted wrote: > We should check the actual value too > > e.g. > > for (int i : {0, 1}) { > NSString* value = [cocoa_tab[i] > accessibilityAttributeValue:NSAccessibilityValueAttribute] stringValue]; > EXPECT_NSEQ(i ? @"0" : @"1", value); > EXPECT_NSEQ(value, AttributeValueAtPoint(NSAccessibilityValueAttribute, > GetTabAt(i))); > } Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_mac_accessibility_unittest.mm:159: } On 2017/01/06 04:25:12, tapted wrote: > Can we also test the AXAction "press" - this seems to be what Cocoa makes > available in NSTabView for changing tabs (maybe add this to the > AppKitAttributes test case and compare with NSTabView behaviour) NSAccessibilityActions aren't hooked up yet to Views. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=679247 to track it (I thought this existed already, but I couldn't find it), but I can add a disabled test to check it if you'd like to have it all in the same CL? https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:39: base::string16 kTabTitle = ASCIIToUTF16("tab"); On 2017/01/06 04:25:12, tapted wrote: > This will make a static initializer, which will get the CL reverted. Perhaps add > an anonymous namespace that includes FixedSizeView, and have a helper method > like > > base::string16 DefaultTabTitle() { > return ASCIIToUTF16("tab"); > } > > > (or in the SelectTabWithAccessibleAction test, just add a local > > const base::string16 tab_title = ASCIIToUTF16("tab"); > > ) > > (note we only use kFooStyle naming for compile-time constants, so not all things > --e.g. strings-- declared `const` should be kFooStyle) Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:41: typedef ViewsTestBase TabbedPaneTest; On 2017/01/06 04:25:12, tapted wrote: > I think it's time to upgrade this to a proper test harness. then you can add a > helper method instead of using `std::vector<Tab*> tab_list;` > > Also, we can remove all the FRIEND_TEST_ALL_PREFIXES in tabbed_pane.h and just > friend the test harness Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:147: const int num_tabs = 2; On 2017/01/06 04:25:12, tapted wrote: > constexpr int kNumTabs = 2; > > (constexpr helps guarantee that it is a compile-time constant, and is preferred > when that's the case now) Done, thanks I didn't know about constexpr. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:149: for (int i = 0; i <= num_tabs; ++i) { On 2017/01/06 04:25:12, tapted wrote: > this ends up making 3 tabs, which I see is intentional, but it's a bit confusing > when the constant is `2`. We should make the constant 3 and update the code > below Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:165: EXPECT_EQ(i == num_tabs, data.HasStateFlag(ui::AX_STATE_SELECTED)); On 2017/01/06 04:25:12, tapted wrote: > e.g. i == kNumTabs - 1 Done. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:171: NativeViewAccessibility::Create(tab_list[0]) On 2017/01/06 04:25:12, tapted wrote: > I think this is leaked. It's possible you are actually the first person to write > a test for `AccessibilityPerformAction`, so it's good that we are fixing that. > > In think in another CL you're changing Create to return a std::unique_ptr, so > that would resolve the leak if we want to wait for that. Alternatively, see > things done in NativeViewAccessibilityTest > > A cleaner way to get at the NativeViewAccessibility to test things like > AccessiblityPerformAction might be nice too... but I don't see a clear way to do > that "nicely" in a way that is also cross-platform. Something to think about > though. Yep, I am - I think this change might land first though, so I'll use the same approach as NativeViewAccessibilityTest to clean up and rebase the other change when this lands. https://codereview.chromium.org/2578303003/diff/220001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:184: widget->Close(); On 2017/01/06 04:25:12, tapted wrote: > nit: CloseNow() Done.
https://codereview.chromium.org/2578303003/diff/240001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2578303003/diff/240001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:416: : data.action = ui::AX_ACTION_SET_VALUE; the single `=` doesn't look right on this line, although it probably achieves the same thing (I think: delete `data.action = ` from this line) https://codereview.chromium.org/2578303003/diff/240001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:420: // type-specific information as necessary for actions set above. It looks easy to introduce codepaths where the stuff below may override the data.action set above - is that intentional? The comment should mention that if so (e.g. "Note the data.action set above may be overridden depending on the type of |attribute|."). Otherwise, perhaps it would be better to move all the [attribute isEqualToString] checks above, and just check [value isKindOfClass] in the set of if statements here? https://codereview.chromium.org/2578303003/diff/240001/ui/strings/ui_strings.grd File ui/strings/ui_strings.grd (right): https://codereview.chromium.org/2578303003/diff/240001/ui/strings/ui_strings.... ui/strings/ui_strings.grd:348: <message name="IDS_ACCNAME_TAB_ROLE_DESCRIPTION" desc="A generic description of a tab button's role" meaning="UI element"> nit: full stop after `role` https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:243: // It's not clear what should happen if a tab is 'deselected', so having the nit: blank line before, delete "having" https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:244: // AX_ACTION_SET_SELECTION action always selects the tab. nit: selects -> select https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm (right): https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:16: nit: remove blank line https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:23: void SetUp() override { nit: // WidgetTest: (or testing::Test, but pkasting rolls differently) https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:45: return static_cast<Tab*>(tabbed_pane_->tab_strip_->child_at(index)); So there's something a bit uglier that should work and avoid having to move TabStrip into the header. // Note this requires View to be the first superclass of TabStrip. View* tab_strip = reinterpret_cast<View*>(tabbed_pane_->tab_strip_); return static_cast<Tab*>(tab_strip->child_at(index)); But there's not really a precedent for that under ui/views. I think moving TabStrip to the header is the right approach. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:66: Widget* widget_; nit: = nullptr (since it's not set in the constructor - it's not a bug, but if, say, someone forgets to call super::SetUp() they will get nicer errors) https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:67: TabbedPane* tabbed_pane_; = nullptr https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:76: NSTabView* cocoa_tab_group = this one still needs a scoped_nsobject (you could put an autorelease on the next line, but by convention we tend to prefer scoped_nsobject -- there are extra runtime overheads for autorelease, but it still gets used if the code is neater with autorelease, like in the array initialization). https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:100: // Check value attribute matches up with whether tabs are actually selected. nit: Compare against native Cocoa values, and check the value attribute... https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:104: EXPECT_EQ(i ? 0 : 1, [cocoa_value intValue]); nit: comment like // Verify that only the second tab is selected. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:144: [tab1_a11y accessibilitySetValue:@"" I find this a bit counterintuitive. E.g. maybe setting an empty string, or the string "0" should just be ignored. But I can see why we don't. I think that should be called out here. E.g. // It doesn't make sense to "deselect" a tab (i.e. without specifying another to select). So any value passed to -accessibilitySetValue: should select that tab. Try an empty string. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:158: nit: remove blank line https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:23: nit: remove blank line https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:25: nit: remove blank line https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:54: tabbed_pane_ = new TabbedPane(); There's a lifetime problem here -- the existing tests would want to `delete tabbed_pane_;` since they don't add it to the widget (which would do that instead). I think the nicest fix is to remove SetUp() and TearDown(), then in the constructor do TabbedPaneTest() { tabbed_pane_ = base::MakeUnique<TabbedPane>(); tabbed_pane_->set_owned_by_client(); } https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:79: TabbedPane* tabbed_pane_; this needs to be a unique_ptr https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:87: View* child1 = new FixedSizeView(gfx::Size(20, 10)); Since we're refactoring.. I think instead of `FixedSizeView` this could just use `StaticSizedView` from test_views.h (and delete `class FixedSizeView`) https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:186: NativeViewAccessibility::Create(GetTabAt(i))->GetData(); ooh - i think the result of `Create` here is leaked currently - we need a Destroy() for it somewhere. I've lost track of the leak-detector trybots (they were once valgrind, then lsan, now... msan? fired one off to see..). https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:215: nit: remove blank line
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2578303003/diff/240001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2578303003/diff/240001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:416: : data.action = ui::AX_ACTION_SET_VALUE; On 2017/01/10 16:22:18, tapted wrote: > the single `=` doesn't look right on this line, although it probably achieves > the same thing (I think: delete `data.action = ` from this line) Oops, thank you - joined the line but didn't edit it. https://codereview.chromium.org/2578303003/diff/240001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:420: // type-specific information as necessary for actions set above. On 2017/01/10 16:22:18, tapted wrote: > It looks easy to introduce codepaths where the stuff below may override the > data.action set above - is that intentional? The comment should mention that if > so (e.g. "Note the data.action set above may be overridden depending on the type > of |attribute|."). Otherwise, perhaps it would be better to move all the > [attribute isEqualToString] checks above, and just check [value isKindOfClass] > in the set of if statements here? Not intentional - I was trying to make a nice way to account for special cases like the value (where you might potentially receive any possible type for the same action) as well as the focus (where you need to know the attribute as well as the type before setting the action). I implemented your second suggestion, but it still needs a type check for setting focus/blur actions, so the attribute/type checking isn't completely separate. I'm not sure which way is preferable. https://codereview.chromium.org/2578303003/diff/240001/ui/strings/ui_strings.grd File ui/strings/ui_strings.grd (right): https://codereview.chromium.org/2578303003/diff/240001/ui/strings/ui_strings.... ui/strings/ui_strings.grd:348: <message name="IDS_ACCNAME_TAB_ROLE_DESCRIPTION" desc="A generic description of a tab button's role" meaning="UI element"> On 2017/01/10 16:22:18, tapted wrote: > nit: full stop after `role` Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.cc (right): https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:243: // It's not clear what should happen if a tab is 'deselected', so having the On 2017/01/10 16:22:18, tapted wrote: > nit: blank line before, delete "having" Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.cc:244: // AX_ACTION_SET_SELECTION action always selects the tab. On 2017/01/10 16:22:19, tapted wrote: > nit: selects -> select Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm (right): https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:16: On 2017/01/10 16:22:19, tapted wrote: > nit: remove blank line Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:23: void SetUp() override { On 2017/01/10 16:22:19, tapted wrote: > nit: // WidgetTest: > > (or testing::Test, but pkasting rolls differently) Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:45: return static_cast<Tab*>(tabbed_pane_->tab_strip_->child_at(index)); On 2017/01/10 16:22:19, tapted wrote: > So there's something a bit uglier that should work and avoid having to move > TabStrip into the header. > > // Note this requires View to be the first superclass of TabStrip. > View* tab_strip = reinterpret_cast<View*>(tabbed_pane_->tab_strip_); > return static_cast<Tab*>(tab_strip->child_at(index)); > > > But there's not really a precedent for that under ui/views. I think moving > TabStrip to the header is the right approach. Yeah - this method is also needed in TabbedViewTest, so I agree. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:66: Widget* widget_; On 2017/01/10 16:22:19, tapted wrote: > nit: = nullptr (since it's not set in the constructor - it's not a bug, but if, > say, someone forgets to call super::SetUp() they will get nicer errors) Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:67: TabbedPane* tabbed_pane_; On 2017/01/10 16:22:19, tapted wrote: > = nullptr Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:76: NSTabView* cocoa_tab_group = On 2017/01/10 16:22:19, tapted wrote: > this one still needs a scoped_nsobject (you could put an autorelease on the next > line, but by convention we tend to prefer scoped_nsobject -- there are extra > runtime overheads for autorelease, but it still gets used if the code is neater > with autorelease, like in the array initialization). Done, thanks for the explanation! https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:100: // Check value attribute matches up with whether tabs are actually selected. On 2017/01/10 16:22:19, tapted wrote: > nit: Compare against native Cocoa values, and check the value attribute... Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:104: EXPECT_EQ(i ? 0 : 1, [cocoa_value intValue]); On 2017/01/10 16:22:19, tapted wrote: > nit: comment like > // Verify that only the second tab is selected. Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:144: [tab1_a11y accessibilitySetValue:@"" On 2017/01/10 16:22:19, tapted wrote: > I find this a bit counterintuitive. E.g. maybe setting an empty string, or the > string "0" should just be ignored. But I can see why we don't. I think that > should be called out here. E.g. > > // It doesn't make sense to "deselect" a tab (i.e. without specifying another to > select). So any value passed to -accessibilitySetValue: should select that tab. > Try an empty string. Done - I also updated the comment on line 129, too, to say why we're using NSString to test a field that "should" be a integer/bool. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_accessibility_mac_unittest.mm:158: On 2017/01/10 16:22:19, tapted wrote: > nit: remove blank line Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:23: On 2017/01/10 16:22:19, tapted wrote: > nit: remove blank line Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:25: On 2017/01/10 16:22:19, tapted wrote: > nit: remove blank line Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:54: tabbed_pane_ = new TabbedPane(); On 2017/01/10 16:22:19, tapted wrote: > There's a lifetime problem here -- the existing tests would want to `delete > tabbed_pane_;` since they don't add it to the widget (which would do that > instead). I think the nicest fix is to remove SetUp() and TearDown(), then in > the constructor do > > TabbedPaneTest() { > tabbed_pane_ = base::MakeUnique<TabbedPane>(); > tabbed_pane_->set_owned_by_client(); > } > Done, thanks! https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:79: TabbedPane* tabbed_pane_; On 2017/01/10 16:22:19, tapted wrote: > this needs to be a unique_ptr Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:87: View* child1 = new FixedSizeView(gfx::Size(20, 10)); On 2017/01/10 16:22:19, tapted wrote: > Since we're refactoring.. I think instead of `FixedSizeView` this could just use > `StaticSizedView` from test_views.h (and delete `class FixedSizeView`) Done. https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:186: NativeViewAccessibility::Create(GetTabAt(i))->GetData(); On 2017/01/10 16:22:19, tapted wrote: > ooh - i think the result of `Create` here is leaked currently - we need a > Destroy() for it somewhere. I've lost track of the leak-detector trybots (they > were once valgrind, then lsan, now... msan? fired one off to see..). Oops, yeah you are right - thanks for picking them all up. (Also let me know when you figure out which one it is :D) https://codereview.chromium.org/2578303003/diff/240001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:215: On 2017/01/10 16:22:19, tapted wrote: > nit: remove blank line Done.
tapted@chromium.org changed reviewers: + dmazzoni@chromium.org
lgtm % nits; +sky, +dmazzoni sky: +cc for advice on moving the TabStrip declaration to the .h (or any other concerns ;). dmazzoni: for a11y OWNERS (to save patti a round trip) - ax_platform_node_mac.mm and the content data change https://codereview.chromium.org/2578303003/diff/260001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2578303003/diff/260001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:426: if ([value isKindOfClass:[NSString class]]) { nit: no curlies needed https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (left): https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:68: Tab* GetTabAt(int index); @sky: this was an orphaned declaration. Friended test harnesses currently implement it as Tab* GetTabAt(int index) { return static_cast<Tab*>(tabbed_pane_->tab_strip_->child_at(index)); } but that requires the the compiler to either see the TabStrip* declaration [current approach], or at least to know it can upcast to a views::View*. Alternatives/Extensions: - a reinterpret_cast from TabStrip* to View* could work, but is kinda ugly (e.g. multiple inheritance could break it) - Provide GetTabAt() in tabbed_pane.cc [would only be used for tests] - tmp = GetSelectedTabIndex(); SelectTabAt(index); tab = GetSelectedTab(); SelectTabAt(tmp); return tab; [would probably work, but has side effects.] - move TabStrip to an internal namespace or separate file https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (right): https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:25: class TabStrip : public View { nit: move this down, after the declaration for `class Tab` (after, since the definitions appear in the .cc after the Tab:: definitions).
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (left): https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:68: Tab* GetTabAt(int index); On 2017/01/12 16:33:55, tapted wrote: > @sky: this was an orphaned declaration. Friended test harnesses currently > implement it as > > Tab* GetTabAt(int index) { > return static_cast<Tab*>(tabbed_pane_->tab_strip_->child_at(index)); > } > > but that requires the the compiler to either see the TabStrip* declaration > [current approach], or at least to know it can upcast to a views::View*. > > Alternatives/Extensions: > - a reinterpret_cast from TabStrip* to View* could work, but is kinda ugly > (e.g. multiple inheritance could break it) > - Provide GetTabAt() in tabbed_pane.cc [would only be used for tests] > - tmp = GetSelectedTabIndex(); SelectTabAt(index); tab = GetSelectedTab(); > SelectTabAt(tmp); return tab; [would probably work, but has side effects.] > - move TabStrip to an internal namespace or separate file I'm ok with exposing the declaration.
The CQ bit was checked by patricialor@chromium.org to run a CQ dry run
Patchset #13 (id:280001) 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 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.
Thanks for the reviews! https://codereview.chromium.org/2578303003/diff/260001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2578303003/diff/260001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:426: if ([value isKindOfClass:[NSString class]]) { On 2017/01/12 16:33:55, tapted wrote: > nit: no curlies needed Done. https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (left): https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:68: Tab* GetTabAt(int index); On 2017/01/12 16:39:21, sky wrote: > On 2017/01/12 16:33:55, tapted wrote: > > @sky: this was an orphaned declaration. Friended test harnesses currently > > implement it as > > > > Tab* GetTabAt(int index) { > > return static_cast<Tab*>(tabbed_pane_->tab_strip_->child_at(index)); > > } > > > > but that requires the the compiler to either see the TabStrip* declaration > > [current approach], or at least to know it can upcast to a views::View*. > > > > Alternatives/Extensions: > > - a reinterpret_cast from TabStrip* to View* could work, but is kinda ugly > > (e.g. multiple inheritance could break it) > > - Provide GetTabAt() in tabbed_pane.cc [would only be used for tests] > > - tmp = GetSelectedTabIndex(); SelectTabAt(index); tab = GetSelectedTab(); > > SelectTabAt(tmp); return tab; [would probably work, but has side effects.] > > - move TabStrip to an internal namespace or separate file > > I'm ok with exposing the declaration. Thanks Trent and sky, leaving as-is then :) https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane.h (right): https://codereview.chromium.org/2578303003/diff/260001/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane.h:25: class TabStrip : public View { On 2017/01/12 16:33:55, tapted wrote: > nit: move this down, after the declaration for `class Tab` (after, since the > definitions appear in the .cc after the Tab:: definitions). Done, also added forward declaration for its use in TabbedPane private members.
lgtm https://codereview.chromium.org/2578303003/diff/300001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2578303003/diff/300001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:189: {ui::AX_ROLE_TAB, @"AXTab"}, Any documentation on this, or did you get it from Accessibility Inspector? I'm just curious because WebKit doesn't seem to use this subrole. See - (NSString*)subrole in: https://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/mac/WebAcc...
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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.
https://codereview.chromium.org/2578303003/diff/300001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2578303003/diff/300001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:189: {ui::AX_ROLE_TAB, @"AXTab"}, On 2017/01/13 18:11:23, dmazzoni wrote: > Any documentation on this, or did you get it from > Accessibility Inspector? > > I'm just curious because WebKit doesn't seem to use > this subrole. See - (NSString*)subrole in: > https://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/mac/WebAcc... Whoops, you're right - deleted. I think I originally had this because it wasn't clear on the newer accessibility inspector where the 'Tab' string role description came from and then I must have forgot :/ Thanks!
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/2578303003/#ps340001 (title: "Rebase")
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": 340001, "attempt_start_ts": 1484535987856610,
"parent_rev": "75e3b207a503c0ad4ad141ca78b7cf4bd06da4ae", "commit_rev":
"dc7d397153bcbccbc21e12f2ea3568f594699220"}
Message was sent while issue was closed.
Description was changed from ========== a11y: Add a11y information to views::Tab and manually ignore its a11y children. Currently, Views tabs have no a11y information specific to their class. The fallback then is to use the views::Label inside them for a11y information instead. This means they report their role as text. Fix this by adding a11y information to views::Tab to report its role as ui::AX_ROLE_TAB, and handle the ui::AX_ACTION_SET_SELECTED action and plumb through for Mac. views::Tab is also a special case where only the selected tab is allowed to be keyboard focusable. Because of this behavior, the views::Label inside is exposed to the accessibility tree. Since views::Tab now has its own a11y information, the views::Label should be hidden from users (see Issue 610589). Make a class views::TabLabel to manually set its a11y role to ui::AX_ROLE_IGNORED. BUG=657593, 610589 ========== to ========== a11y: Add a11y information to views::Tab and manually ignore its a11y children. Currently, Views tabs have no a11y information specific to their class. The fallback then is to use the views::Label inside them for a11y information instead. This means they report their role as text. Fix this by adding a11y information to views::Tab to report its role as ui::AX_ROLE_TAB, and handle the ui::AX_ACTION_SET_SELECTED action and plumb through for Mac. views::Tab is also a special case where only the selected tab is allowed to be keyboard focusable. Because of this behavior, the views::Label inside is exposed to the accessibility tree. Since views::Tab now has its own a11y information, the views::Label should be hidden from users (see Issue 610589). Make a class views::TabLabel to manually set its a11y role to ui::AX_ROLE_IGNORED. BUG=657593, 610589 Review-Url: https://codereview.chromium.org/2578303003 Cr-Commit-Position: refs/heads/master@{#443837} Committed: https://chromium.googlesource.com/chromium/src/+/dc7d397153bcbccbc21e12f2ea35... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:340001) as https://chromium.googlesource.com/chromium/src/+/dc7d397153bcbccbc21e12f2ea35... |
