Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2016243002:
Mac a11y: Add RoleDescription and Value attributes to accessibility information. (Closed)
|
Created:
4 years, 6 months ago by Patti Lor Modified:
4 years, 6 months ago CC:
chromium-reviews, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac a11y: Add RoleDescription, Value, Subrole accessibility attributes & tests.
Populate NSAccessibilityRoleDescriptionAttribute,
NSAccessibilityValueAttribute, and NSAccessibilitySubroleAttribute fields for
views::View elements on Mac, with tests.
Also add unit tests for the attributes currently supported, including a test
specifically for textfields.
BUG=610591
Committed: https://crrev.com/244fc7d3829a5f62b28c5655badb2df290676e2d
Cr-Commit-Position: refs/heads/master@{#400859}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add tests for Mac accessibility attributes. #Patch Set 3 : TODO for comparing against Cocoa. #
Total comments: 47
Patch Set 4 : Address review comments. #
Total comments: 28
Patch Set 5 : Rebase. #
Total comments: 12
Patch Set 6 : Combine textfield tests into one, other review comments. #Patch Set 7 : Rebase again. #
Total comments: 20
Patch Set 8 : Address review comments. #
Dependent Patchsets: Messages
Total messages: 23 (9 generated)
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hey Trent, PTAL! This is for http://crbug.com/610591, but I have split up the fix into multiple CLs.
Can you add a test? There's some stuff in
NativeWidgetMacTest.AccessibilityIntegration to build on -- it tests a
views::Label. We should have a test for a views::TextField too, but we should
make a new test harness. I think the best approach would be a separate file like
ui/views/widget/native_widget_mac_accessibility_unittest.mm
since the a11y test doesn't get much from NativeWidgetMacTest. Then something
like
class NativeWidgetMacAccessibilityTest : public WidgetTest {
..
SetUp() {
widget_ = CreateTopLevelPlatformWidget();
etc.
}
TearDown() {
widget_->CloseNow();
}
id AttributeValueAtMidpoint() {
..
}
etc.
then the test cases themselves just add the control to the Widget and test some
properties.
We'll later need to add things like hit-testing in that padding area, and some
comparisons with native textfields. Also things like selection ranges.
https://codereview.chromium.org/2016243002/diff/1/ui/accessibility/platform/a...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):
https://codereview.chromium.org/2016243002/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/ax_platform_node_mac.mm:278: -
(NSString*)retrieveAXStringAttribute:(ui::AXStringAttribute)attribute {
Since this essentially wraps node_->GetStringAttribute, let's call this
getStringAttribute. Then, since this forms part of the internal API, this should
be redeclared in a private category in this file.
before @implementation AXPlatformNodeCocoa above, do
@interface AXPlatformNodeCocoa ()
- (NSString*)getStringAttribute:(ui::AXStringAttribute)attribute;
@end
Then things declared in private categories, should be defined just after the
public categories (so move this up to come after boundsInScreen)
Description was changed from ========== Mac a11y: Add RoleDescription and Value attributes to accessibility information. Populate NSAccessibilityRoleDescriptionAttribute and NSAccessibilityValueAttribute fields for elements on Mac. BUG=610591 ========== to ========== Mac a11y: Add RoleDescription, Value accessibility attributes with tests. Populate NSAccessibilityRoleDescriptionAttribute and NSAccessibilityValueAttribute fields for elements on Mac. Also add unit tests for the attributes currently supported. BUG=610591 ==========
Hi Trent, have added tests for all the attributes now. Two of the tests fail at the moment, see TODOs for more info. Also, I wasn't sure about whether the changes to coordinate_conversion.* was worth it, so let me know what you think about that too and if it's ok I'll move it out into a separate CL. https://codereview.chromium.org/2016243002/diff/1/ui/accessibility/platform/a... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/1/ui/accessibility/platform/a... ui/accessibility/platform/ax_platform_node_mac.mm:278: - (NSString*)retrieveAXStringAttribute:(ui::AXStringAttribute)attribute { On 2016/05/30 11:30:21, tapted wrote: > Since this essentially wraps node_->GetStringAttribute, let's call this > getStringAttribute. Then, since this forms part of the internal API, this should > be redeclared in a private category in this file. > > before @implementation AXPlatformNodeCocoa above, do > > @interface AXPlatformNodeCocoa () > - (NSString*)getStringAttribute:(ui::AXStringAttribute)attribute; > @end > > Then things declared in private categories, should be defined just after the > public categories (so move this up to come after boundsInScreen) Done.
https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:271: NSAccessibilityValueAttribute, So I poked around a bit - I think doing this for RoleDescriptionAttribute is fine, but not much provides valueAttribute. we should act on that TODO below -- switch on node_->GetData().role and append extra things to the array, based on the role. https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:324: return [self getStringAttribute:ui::AX_ATTR_DESCRIPTION]; So if this returns nil, I think we should then try doing NSString* role = [self AXRole]; NSString* subrole = nil; // TODO(..): Support subroles. return role ? NSAccessibilityRoleDescription(role, subrole) : nil; NSAccessibilityRoleDescription gets localized descriptions for the standard roles from AppKit. It might be worth simply trying to pass a nil |role| as well - -maybe AppKit provides something better than merely `nil` (or maybe it crashes - we should check) https://codereview.chromium.org/2016243002/diff/40001/ui/gfx/mac/coordinate_c... File ui/gfx/mac/coordinate_conversion.h (right): https://codereview.chromium.org/2016243002/diff/40001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion.h:27: GFX_EXPORT NSSize ScreenSizeToNSSize(const Size& size); Something like `NSSize ns_size = size.ToCGSize()` should work -- CGSize and NSSize are sorta the same, and I think the compiler just allows this to happen. Similarly, I think `gfx::Size gfx_size(NSMakeSize(x, y));` Just Works. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:11: #include "testing/gtest_mac.h" nit: import https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:23: namespace test { remove `test` -- we can have an anonymous namespace, which will give the classes internal linkage https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:25: class AXIgnoredTestView : public View { nit: remove the `AX` prefix - IgnoredTestView is good (the AX prefix implies other stuff) https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:27: state->role = ui::AX_ROLE_IGNORED; #include "ui/accessibility/ax_enums.h" for IWYU https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:29: }; nit: private:\n DISALLOW_COPY_AND_ASSIGN - you'll need a constructor https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:31: class AXTestView : public Label { TestLabel? But also it's weird for Labels to have a value - can you just use a views::Textfield and set the text on it? https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:49: NSRect ns_widget_bounds = gfx::ScreenRectToNSRect(widget_bounds_); nit: perhaps just midpoint_in_screen_ = gfx::ScreenPointToNSPoint( widget_->GetWindowBoundsInScreen().CenterPoint())); https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:66: Label* AddChildLabel(gfx::Size size) { const gfx::Size& size https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:74: gfx::Rect widget_bounds() { return widget_bounds_; } we should remove |widget_bounds_| -- we can call a method on widget_ here instead. I think most useful would be ContentBounds() -> widget_->GetClientAreaBoundsInScreen https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:84: anonymous namespace should be closed here https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:85: TEST_F(NativeWidgetMacAccessibilityTest, ChildrenAttribute) { All tests should have a short description describing their purpose https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:90: const unsigned int num_children = 3; we don't use `unsigned int` except in rare cases - see go/cppguide#Integer_Types . Probably size_t here will avoid the warnings on the EXPECT_EQ calls also for compile-time constants, we use kFooStyle -- see go/cppguide#Constant_Names https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:93: AddChildLabel(gfx::Size(1, 1)); does gfx::Size() work? (i.e. empty?). Otherwise we need to ask whether the ignored view is ignored because it its size is empty, or because it is ignored. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:95: widget()->Show(); can this be done in SetUp()? https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:107: Label* label = AddChildLabel(widget_bounds().size()); the widget bounds should include the titlebar - perhaps widget()->GetContentsView()->bounds() is better https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:111: // sometimes returns a gfx::NativeView and other times a AXPlatformNodeCocoa. sometimes? (is it random?) Perhaps we need to wait for the widget to also activate. There's some stuff in widget_interactive_uitest.cc for this. Or WindowedNSNotificationObserver in ui/base/test, but only if it's needed https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:112: EXPECT_NSEQ(widget()->GetNativeWindow(), comparing types directly (particularly an NSView) is odd for this. Can we compare an attribute instead? E.g. setWindowTitle on the widget, then get the title. Or ignore the window completely -- just add another child of |label| https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:115: // Test an ignored role parent is skipped in favor of the grandparent widget. remove " widget". (i.e .grandparent doesn't have to be a widget. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:177: // TODO(patricialor): Fix - AX_ATTR_DESCRIPTION doesn't seem to be set So this is bad, we should provide AX_ATTR_DESCRIPTION for everything. But my earlier suggestion should fix this.
Description was changed from ========== Mac a11y: Add RoleDescription, Value accessibility attributes with tests. Populate NSAccessibilityRoleDescriptionAttribute and NSAccessibilityValueAttribute fields for elements on Mac. Also add unit tests for the attributes currently supported. BUG=610591 ========== to ========== Mac a11y: Add RoleDescription, Value, Subrole accessibility attributes & tests. Populate NSAccessibilityRoleDescriptionAttribute, NSAccessibilityValueAttribute, and NSAccessibilitySubroleAttribute fields for views::View elements on Mac, with tests. Also add unit tests for the attributes currently supported. BUG=610591 ==========
Patchset #5 (id:80001) has been deleted
Thanks, PTAL! https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:271: NSAccessibilityValueAttribute, On 2016/06/06 04:09:51, tapted wrote: > So I poked around a bit - I think doing this for RoleDescriptionAttribute is > fine, but not much provides valueAttribute. we should act on that TODO below -- > switch on node_->GetData().role and append extra things to the array, based on > the role. Have added switch to add Value attributes on a bunch of user input fields for now, or do you think I should just add it for TEXT_FIELDs only and leave it? https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:324: return [self getStringAttribute:ui::AX_ATTR_DESCRIPTION]; On 2016/06/06 04:09:51, tapted wrote: > So if this returns nil, I think we should then try doing > > NSString* role = [self AXRole]; > NSString* subrole = nil; // TODO(..): Support subroles. > return role ? NSAccessibilityRoleDescription(role, subrole) : nil; > > NSAccessibilityRoleDescription gets localized descriptions for the standard > roles from AppKit. > > It might be worth simply trying to pass a nil |role| as well - -maybe AppKit > provides something better than merely `nil` (or maybe it crashes - we should > check) Works with nil values! Done. Have also added the subrole attribute to this CL, but to do this I copied some logic from browser_accessibility_cocoa.mm's subrole method which I am not sure about. https://codereview.chromium.org/2016243002/diff/40001/ui/gfx/mac/coordinate_c... File ui/gfx/mac/coordinate_conversion.h (right): https://codereview.chromium.org/2016243002/diff/40001/ui/gfx/mac/coordinate_c... ui/gfx/mac/coordinate_conversion.h:27: GFX_EXPORT NSSize ScreenSizeToNSSize(const Size& size); On 2016/06/06 04:09:51, tapted wrote: > Something like `NSSize ns_size = size.ToCGSize()` should work -- CGSize and > NSSize are sorta the same, and I think the compiler just allows this to happen. > > Similarly, I think `gfx::Size gfx_size(NSMakeSize(x, y));` Just Works. Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:11: #include "testing/gtest_mac.h" On 2016/06/06 04:09:52, tapted wrote: > nit: import Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:23: namespace test { On 2016/06/06 04:09:52, tapted wrote: > remove `test` -- we can have an anonymous namespace, which will give the classes > internal linkage Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:25: class AXIgnoredTestView : public View { On 2016/06/06 04:09:52, tapted wrote: > nit: remove the `AX` prefix - IgnoredTestView is good (the AX prefix implies > other stuff) Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:27: state->role = ui::AX_ROLE_IGNORED; On 2016/06/06 04:09:52, tapted wrote: > #include "ui/accessibility/ax_enums.h" for IWYU Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:29: }; On 2016/06/06 04:09:52, tapted wrote: > nit: private:\n DISALLOW_COPY_AND_ASSIGN - you'll need a constructor Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:31: class AXTestView : public Label { On 2016/06/06 04:09:52, tapted wrote: > TestLabel? But also it's weird for Labels to have a value - can you just use a > views::Textfield and set the text on it? Have removed this test class entirely, using textfields means the title attribute can be set using SetAccessibleName and the value is set in the Textfield class already. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:49: NSRect ns_widget_bounds = gfx::ScreenRectToNSRect(widget_bounds_); On 2016/06/06 04:09:52, tapted wrote: > nit: perhaps just > > midpoint_in_screen_ = gfx::ScreenPointToNSPoint( > widget_->GetWindowBoundsInScreen().CenterPoint())); Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:66: Label* AddChildLabel(gfx::Size size) { On 2016/06/06 04:09:52, tapted wrote: > const gfx::Size& size Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:74: gfx::Rect widget_bounds() { return widget_bounds_; } On 2016/06/06 04:09:52, tapted wrote: > we should remove |widget_bounds_| -- we can call a method on widget_ here > instead. > > I think most useful would be ContentBounds() -> > widget_->GetClientAreaBoundsInScreen Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:84: On 2016/06/06 04:09:52, tapted wrote: > anonymous namespace should be closed here Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:85: TEST_F(NativeWidgetMacAccessibilityTest, ChildrenAttribute) { On 2016/06/06 04:09:52, tapted wrote: > All tests should have a short description describing their purpose Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:90: const unsigned int num_children = 3; On 2016/06/06 04:09:51, tapted wrote: > we don't use `unsigned int` except in rare cases - see go/cppguide#Integer_Types > . Probably size_t here will avoid the warnings on the EXPECT_EQ calls > > also for compile-time constants, we use kFooStyle -- see > go/cppguide#Constant_Names Ooh, thanks for the links - thought the k thing was for member variables only. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:93: AddChildLabel(gfx::Size(1, 1)); On 2016/06/06 04:09:52, tapted wrote: > does gfx::Size() work? (i.e. empty?). Otherwise we need to ask whether the > ignored view is ignored because it its size is empty, or because it is ignored. Yep, it works, have changed to empty sizes. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:95: widget()->Show(); On 2016/06/06 04:09:52, tapted wrote: > can this be done in SetUp()? Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:107: Label* label = AddChildLabel(widget_bounds().size()); On 2016/06/06 04:09:51, tapted wrote: > the widget bounds should include the titlebar - perhaps > widget()->GetContentsView()->bounds() is better Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:111: // sometimes returns a gfx::NativeView and other times a AXPlatformNodeCocoa. On 2016/06/06 04:09:52, tapted wrote: > sometimes? (is it random?) Perhaps we need to wait for the widget to also > activate. There's some stuff in widget_interactive_uitest.cc for this. Or > WindowedNSNotificationObserver in ui/base/test, but only if it's needed No, not random - it returns a gfx::NativeView / NSWindow when the view has a widget as its direct parent and a AXPlatformNodeCocoa when it has another view as a parent. I think it's because NSWindow will respond to all the accessibility methods, but a views::View won't, which is why it needs to return a AXPlatformNodeCocoa instead. Regardless of the implementation details I have changed the expectations to test against the ParentAttribute of the parent as per your comment below, so it shouldn't matter any more :) https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:112: EXPECT_NSEQ(widget()->GetNativeWindow(), On 2016/06/06 04:09:52, tapted wrote: > comparing types directly (particularly an NSView) is odd for this. Can we > compare an attribute instead? E.g. setWindowTitle on the widget, then get the > title. Or ignore the window completely -- just add another child of |label| Changed to compare the expected role of the parent instead. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:115: // Test an ignored role parent is skipped in favor of the grandparent widget. On 2016/06/06 04:09:52, tapted wrote: > remove " widget". (i.e .grandparent doesn't have to be a widget. Done. https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:177: // TODO(patricialor): Fix - AX_ATTR_DESCRIPTION doesn't seem to be set On 2016/06/06 04:09:52, tapted wrote: > So this is bad, we should provide AX_ATTR_DESCRIPTION for everything. But my > earlier suggestion should fix this. Done, thanks for that!
https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:324: return [self getStringAttribute:ui::AX_ATTR_DESCRIPTION]; On 2016/06/10 01:36:09, Patti Lor wrote: > On 2016/06/06 04:09:51, tapted wrote: > > So if this returns nil, I think we should then try doing > > > > NSString* role = [self AXRole]; > > NSString* subrole = nil; // TODO(..): Support subroles. > > return role ? NSAccessibilityRoleDescription(role, subrole) : nil; > > > > NSAccessibilityRoleDescription gets localized descriptions for the standard > > roles from AppKit. > > > > It might be worth simply trying to pass a nil |role| as well - -maybe AppKit > > provides something better than merely `nil` (or maybe it crashes - we should > > check) > > Works with nil values! Done. Have also added the subrole attribute to this CL, > but to do this I copied some logic from browser_accessibility_cocoa.mm's subrole > method which I am not sure about. What string does it return for nil? The API is NSAccessibilityRoleDescription(NSString *role, NSString * __nullable subrole) So, passing nil to subrole is unambiguously OK. but |role| is neither __nullable nor __nonnull (nor __null_unspecified). If an actual string is returned, then that's good, but if it returns nil, then we can't be sure we're using the api right (and should perhaps check for that) https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:111: // sometimes returns a gfx::NativeView and other times a AXPlatformNodeCocoa. On 2016/06/10 01:36:11, Patti Lor wrote: > On 2016/06/06 04:09:52, tapted wrote: > > sometimes? (is it random?) Perhaps we need to wait for the widget to also > > activate. There's some stuff in widget_interactive_uitest.cc for this. Or > > WindowedNSNotificationObserver in ui/base/test, but only if it's needed > > No, not random - it returns a gfx::NativeView / NSWindow when the view has a > widget as its direct parent and a AXPlatformNodeCocoa when it has another view > as a parent. I think it's because NSWindow will respond to all the accessibility > methods, but a views::View won't, which is why it needs to return a > AXPlatformNodeCocoa instead. Regardless of the implementation details I have > changed the expectations to test against the ParentAttribute of the parent as > per your comment below, so it shouldn't matter any more :) Acknowledged. https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/ax_vie... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/ax_vie... ui/accessibility/ax_view_state.h:27: static uint32_t AddStateFlag(uint32_t state, ui::AXState state_flag); do we need AddStateFlag? I think it's fine to leave this just as a member function https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/ax_vie... ui/accessibility/ax_view_state.h:28: static bool HasStateFlag(uint32_t state, ui::AXState state_flag); We shouldn't have some overloads static and some non-static. Perhaps `IsStateFlagSet` or just IsFlagSet since `State` will already be in the scope qualifier. I think it makes sense for an instance to "have" something, but not as much for a class interface. also nit: static methods after constructor/destructor (reorder in .cc too) https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:259: NSMutableArray* axAttributes = [NSMutableArray we should keep the array literals as constants. E.g. NSArray* const kAllRoleAttributes[] = @[ ... ]; NSArray* const kValueAttributes[] = @[ ... ]; Then just start with scoped_nsobject<NSMutableArray> axAttributes([[NSMutableArray alloc] init]); [axAttributes addObjectsFromArray:... swtich(..) { [axAttributes addObjectsFromArray:... } return axAttributes.autorelease(); https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:347: } break; break would go inside the curlies, but I don't think we need curlies -- typically they're just needed when declaring variables inside a `case` https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:35: FlexibleRoleTestView(ui::AXRole role) : role_(role) {} nit: explicit https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:36: void SetRole(ui::AXRole role) { role_ = role; } SetRole -> set_role for basic setters https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:44: void AddChildView(View* view) { This isn't virtual, so we should give it a new name, Maybe AddChildAdoptSize? or SetBoundsToNewChild? https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:46: // Allow the child to take its full size for accurate hit tests. This is making the parent take the size of the child https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:52: DISALLOW_COPY_AND_ASSIGN(FlexibleRoleTestView); nit: blank line before https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:92: const base::string16 kTestTextfieldTitle = since these aren't compile time constants, we should use test_textfield_title. Same with kTestString. But also it's unusual to have these as non-static data members. let's put NSString* const kTestStringValue = @"Test string value"; NSString* const kTestTitle = @"Test title"; at the top, just after `namespace {` we need to convert them to NSString a bunch in more places, so this avoids that. (we just need a different converter when calling SetText/SetAccessibleName https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:96: Widget* widget_; whoops - these should be initialized = nullptr https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:98: NSPoint midpoint_in_screen_; = {0, 0} https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:99: }; nit: DISALLOW_COPY... You'll need a default constructor https://codereview.chromium.org/2016243002/diff/100001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:339: - (NSString*)AXSubrole { nit: move after AXRole https://codereview.chromium.org/2016243002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:358: - (NSString*)AXRoleDescription { nit: move after AXSubrole https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:89: gfx::Rect widget_bounds() { return widget_->GetClientAreaBoundsInScreen(); } nit: GetWidgetBounds https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:103: // Test for NSAccessibilityChildrenAttribute, including with ignored children in including with -> to ensure it excludes? https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:131: // Views with widget parents will have a NSWindow parent. nit widget -> Widget. more below https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:170: TEST_F(NativeWidgetMacAccessibilityTest, RoleAttribute) { I think we can merge the rest of the tests into one: "Tests for accessibility attributes on a views::Textfield". (in a follow-up, we would compare the results against an NSTextField)
Thanks Trent, PTAL! https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/40001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:324: return [self getStringAttribute:ui::AX_ATTR_DESCRIPTION]; On 2016/06/10 03:17:17, tapted wrote: > On 2016/06/10 01:36:09, Patti Lor wrote: > > On 2016/06/06 04:09:51, tapted wrote: > > > So if this returns nil, I think we should then try doing > > > > > > NSString* role = [self AXRole]; > > > NSString* subrole = nil; // TODO(..): Support subroles. > > > return role ? NSAccessibilityRoleDescription(role, subrole) : nil; > > > > > > NSAccessibilityRoleDescription gets localized descriptions for the standard > > > roles from AppKit. > > > > > > It might be worth simply trying to pass a nil |role| as well - -maybe AppKit > > > provides something better than merely `nil` (or maybe it crashes - we should > > > check) > > > > Works with nil values! Done. Have also added the subrole attribute to this CL, > > but to do this I copied some logic from browser_accessibility_cocoa.mm's > subrole > > method which I am not sure about. > > What string does it return for nil? The API is > > NSAccessibilityRoleDescription(NSString *role, NSString * __nullable subrole) > > So, passing nil to subrole is unambiguously OK. > > but |role| is neither __nullable nor __nonnull (nor __null_unspecified). If an > actual string is returned, then that's good, but if it returns nil, then we > can't be sure we're using the api right (and should perhaps check for that) > Having a nil role returns an empty NSString, so I've left it as is. https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/ax_vie... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/ax_vie... ui/accessibility/ax_view_state.h:27: static uint32_t AddStateFlag(uint32_t state, ui::AXState state_flag); On 2016/06/10 03:17:17, tapted wrote: > do we need AddStateFlag? I think it's fine to leave this just as a member > function Done. https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/ax_vie... ui/accessibility/ax_view_state.h:28: static bool HasStateFlag(uint32_t state, ui::AXState state_flag); On 2016/06/10 03:17:17, tapted wrote: > We shouldn't have some overloads static and some non-static. Perhaps > `IsStateFlagSet` or just IsFlagSet since `State` will already be in the scope > qualifier. I think it makes sense for an instance to "have" something, but not > as much for a class interface. also nit: static methods after > constructor/destructor (reorder in .cc too) Done. https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:259: NSMutableArray* axAttributes = [NSMutableArray On 2016/06/10 03:17:17, tapted wrote: > we should keep the array literals as constants. E.g. > > NSArray* const kAllRoleAttributes[] = @[ > ... > ]; > > NSArray* const kValueAttributes[] = @[ > ... > ]; > > > Then just start with > > scoped_nsobject<NSMutableArray> axAttributes([[NSMutableArray alloc] init]); > > [axAttributes addObjectsFromArray:... > swtich(..) { > [axAttributes addObjectsFromArray:... > } > > return axAttributes.autorelease(); Done. Thanks for the const tip, I hadn't known about that! https://codereview.chromium.org/2016243002/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_platform_node_mac.mm:347: } break; On 2016/06/10 03:17:17, tapted wrote: > break would go inside the curlies, but I don't think we need curlies -- > typically they're just needed when declaring variables inside a `case` Done. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:35: FlexibleRoleTestView(ui::AXRole role) : role_(role) {} On 2016/06/10 03:17:18, tapted wrote: > nit: explicit Done. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:36: void SetRole(ui::AXRole role) { role_ = role; } On 2016/06/10 03:17:18, tapted wrote: > SetRole -> set_role for basic setters Done. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:44: void AddChildView(View* view) { On 2016/06/10 03:17:17, tapted wrote: > This isn't virtual, so we should give it a new name, Maybe AddChildAdoptSize? or > SetBoundsToNewChild? Changed it to 'FitBoundsToNewChild'. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:46: // Allow the child to take its full size for accurate hit tests. On 2016/06/10 03:17:17, tapted wrote: > This is making the parent take the size of the child That was the intention, do you think it makes more sense to do it the other way around? https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:52: DISALLOW_COPY_AND_ASSIGN(FlexibleRoleTestView); On 2016/06/10 03:17:17, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:92: const base::string16 kTestTextfieldTitle = On 2016/06/10 03:17:18, tapted wrote: > since these aren't compile time constants, we should use test_textfield_title. > Same with kTestString. But also it's unusual to have these as non-static data > members. let's put > > NSString* const kTestStringValue = @"Test string value"; > NSString* const kTestTitle = @"Test title"; > > at the top, just after `namespace {` > > we need to convert them to NSString a bunch in more places, so this avoids that. > (we just need a different converter when calling SetText/SetAccessibleName Done. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:96: Widget* widget_; On 2016/06/10 03:17:18, tapted wrote: > whoops - these should be initialized = nullptr Done. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:96: Widget* widget_; On 2016/06/10 03:17:18, tapted wrote: > whoops - these should be initialized = nullptr Done. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:98: NSPoint midpoint_in_screen_; On 2016/06/10 03:17:17, tapted wrote: > = {0, 0} Removed entirely, realised some things were broken because I was moving things around/resizing things and not updating the hit location. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:99: }; On 2016/06/10 03:17:18, tapted wrote: > nit: DISALLOW_COPY... You'll need a default constructor Done. https://codereview.chromium.org/2016243002/diff/100001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:339: - (NSString*)AXSubrole { On 2016/06/10 03:17:18, tapted wrote: > nit: move after AXRole Done. https://codereview.chromium.org/2016243002/diff/100001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:358: - (NSString*)AXRoleDescription { On 2016/06/10 03:17:18, tapted wrote: > nit: move after AXSubrole Done. https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:89: gfx::Rect widget_bounds() { return widget_->GetClientAreaBoundsInScreen(); } On 2016/06/10 03:17:18, tapted wrote: > nit: GetWidgetBounds Done. https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:103: // Test for NSAccessibilityChildrenAttribute, including with ignored children in On 2016/06/10 03:17:18, tapted wrote: > including with -> to ensure it excludes? Done. https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:131: // Views with widget parents will have a NSWindow parent. On 2016/06/10 03:17:18, tapted wrote: > nit widget -> Widget. more below Done. https://codereview.chromium.org/2016243002/diff/100001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:170: TEST_F(NativeWidgetMacAccessibilityTest, RoleAttribute) { On 2016/06/10 03:17:18, tapted wrote: > I think we can merge the rest of the tests into one: "Tests for accessibility > attributes on a views::Textfield". (in a follow-up, we would compare the results > against an NSTextField) Done.
lgtm with some nits, and some stuff moved to a follow-up. https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/60001/ui/views/widget/native_... ui/views/widget/native_widget_mac_accessibility_unittest.mm:46: // Allow the child to take its full size for accurate hit tests. On 2016/06/16 07:05:22, Patti Lor wrote: > On 2016/06/10 03:17:17, tapted wrote: > > This is making the parent take the size of the child > > That was the intention, do you think it makes more sense to do it the other way > around? Ah, I just meant the comment didn't seem to match, but you've updated it - thanks! https://codereview.chromium.org/2016243002/diff/140001/content/browser/access... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/2016243002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility.cc:956: return ui::AXViewState::IsFlagSet(GetState(), state_enum); Let's move the browser_accessibility* changes to a follow-up. They're kinda orthogonal to the CL description. And, e.g., I'm not sure how tight the correspondence between ui::AXViewState and the "state" variable is here -- maybe it's coincidence that they're encoded the same way. https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.cc (right): https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.cc:19: bool AXViewState::IsFlagSet(uint32_t state, ui::AXState state_flag) { nit: // static https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:30: // Like HasStateFlag(), but static. HasStateFlag's is a bit "light" so it doesn't make sense to reference it here. Perhaps // Helper to check whether |state_flag| is set in the given |state|. https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:12: #import "ui/accessibility/ax_node_data.h" nit: can you update this to #include while you're here https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:14: #import "ui/accessibility/platform/ax_platform_node_delegate.h" this too - these are not objc headers https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:269: NSAccessibilityRoleDescriptionAttribute nit: comma after https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:277: [axAttributes addObjectsFromArray:kAllRoleAttributes]; nit: blank line before https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:344: } nit: no curlies required https://codereview.chromium.org/2016243002/diff/140001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:45: void FitBoundsToNewChild(View* view) { nit: new methods before overrides (except SetUp/TearDown since they're like constructors) https://codereview.chromium.org/2016243002/diff/140001/ui/views/widget/native... File ui/views/widget/native_widget_mac_unittest.mm (left): https://codereview.chromium.org/2016243002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:577: TEST_F(NativeWidgetMacTest, AccessibilityIntegration) { NativeWidgetMacAccessibilityTest doesn't add a test for views::Label yet, so let's move this in a follow-up.
Description was changed from ========== Mac a11y: Add RoleDescription, Value, Subrole accessibility attributes & tests. Populate NSAccessibilityRoleDescriptionAttribute, NSAccessibilityValueAttribute, and NSAccessibilitySubroleAttribute fields for views::View elements on Mac, with tests. Also add unit tests for the attributes currently supported. BUG=610591 ========== to ========== Mac a11y: Add RoleDescription, Value, Subrole accessibility attributes & tests. Populate NSAccessibilityRoleDescriptionAttribute, NSAccessibilityValueAttribute, and NSAccessibilitySubroleAttribute fields for views::View elements on Mac, with tests. Also add unit tests for the attributes currently supported, including a test specifically for textfields. BUG=610591 ==========
patricialor@chromium.org changed reviewers: + dmazzoni@chromium.org, sky@chromium.org
sky@: PTAL for OWNERS review for everything under ui/views/. dmazzoni@: PTAL for OWNERS review for content/browser/accessibility/* and ui/accessibility/*. Thanks both! https://codereview.chromium.org/2016243002/diff/140001/content/browser/access... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/2016243002/diff/140001/content/browser/access... content/browser/accessibility/browser_accessibility.cc:956: return ui::AXViewState::IsFlagSet(GetState(), state_enum); On 2016/06/17 03:32:49, tapted wrote: > Let's move the browser_accessibility* changes to a follow-up. They're kinda > orthogonal to the CL description. And, e.g., I'm not sure how tight the > correspondence between ui::AXViewState and the "state" variable is here -- maybe > it's coincidence that they're encoded the same way. Done. https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.cc (right): https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.cc:19: bool AXViewState::IsFlagSet(uint32_t state, ui::AXState state_flag) { On 2016/06/17 03:32:49, tapted wrote: > nit: // static Done. https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/ax_vi... File ui/accessibility/ax_view_state.h (right): https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/ax_vi... ui/accessibility/ax_view_state.h:30: // Like HasStateFlag(), but static. On 2016/06/17 03:32:49, tapted wrote: > HasStateFlag's is a bit "light" so it doesn't make sense to reference it here. > Perhaps > > // Helper to check whether |state_flag| is set in the given |state|. Done. https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... File ui/accessibility/platform/ax_platform_node_mac.mm (right): https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:12: #import "ui/accessibility/ax_node_data.h" On 2016/06/17 03:32:49, tapted wrote: > nit: can you update this to #include while you're here Done. https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:14: #import "ui/accessibility/platform/ax_platform_node_delegate.h" On 2016/06/17 03:32:49, tapted wrote: > this too - these are not objc headers Done, thanks for noticing :) https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:269: NSAccessibilityRoleDescriptionAttribute On 2016/06/17 03:32:49, tapted wrote: > nit: comma after Done. https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:277: [axAttributes addObjectsFromArray:kAllRoleAttributes]; On 2016/06/17 03:32:49, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/2016243002/diff/140001/ui/accessibility/platf... ui/accessibility/platform/ax_platform_node_mac.mm:344: } On 2016/06/17 03:32:49, tapted wrote: > nit: no curlies required Done. https://codereview.chromium.org/2016243002/diff/140001/ui/views/widget/native... File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right): https://codereview.chromium.org/2016243002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_accessibility_unittest.mm:45: void FitBoundsToNewChild(View* view) { On 2016/06/17 03:32:49, tapted wrote: > nit: new methods before overrides (except SetUp/TearDown since they're like > constructors) Done. https://codereview.chromium.org/2016243002/diff/140001/ui/views/widget/native... File ui/views/widget/native_widget_mac_unittest.mm (left): https://codereview.chromium.org/2016243002/diff/140001/ui/views/widget/native... ui/views/widget/native_widget_mac_unittest.mm:577: TEST_F(NativeWidgetMacTest, AccessibilityIntegration) { On 2016/06/17 03:32:49, tapted wrote: > NativeWidgetMacAccessibilityTest doesn't add a test for views::Label yet, so > let's move this in a follow-up. Done.
LGTM
lgtm
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 Link to the patchset: https://codereview.chromium.org/2016243002/#ps160001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2016243002/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Mac a11y: Add RoleDescription, Value, Subrole accessibility attributes & tests. Populate NSAccessibilityRoleDescriptionAttribute, NSAccessibilityValueAttribute, and NSAccessibilitySubroleAttribute fields for views::View elements on Mac, with tests. Also add unit tests for the attributes currently supported, including a test specifically for textfields. BUG=610591 ========== to ========== Mac a11y: Add RoleDescription, Value, Subrole accessibility attributes & tests. Populate NSAccessibilityRoleDescriptionAttribute, NSAccessibilityValueAttribute, and NSAccessibilitySubroleAttribute fields for views::View elements on Mac, with tests. Also add unit tests for the attributes currently supported, including a test specifically for textfields. BUG=610591 Committed: https://crrev.com/244fc7d3829a5f62b28c5655badb2df290676e2d Cr-Commit-Position: refs/heads/master@{#400859} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/244fc7d3829a5f62b28c5655badb2df290676e2d Cr-Commit-Position: refs/heads/master@{#400859} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
