Mac a11y: Add RoleDescription and Value attributes to accessibility information.

ui/views/widget/native_widget_mac_accessibility_unittest.mm

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+
+#import <Cocoa/Cocoa.h>
+
+#include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "testing/gtest_mac.h"

[tapted, 2016/06/06 04:09:52]

nit: import

[Patti Lor, 2016/06/10 01:36:10]

Done.

+#include "ui/accessibility/ax_view_state.h"
+#import "ui/gfx/mac/coordinate_conversion.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/test/widget_test.h"
+#include "ui/views/widget/widget.h"
+
+// TODO(patricialor): Test against Cocoa attributes as well to ensure
+// consistency between Cocoa and toolkit-views.
+
+namespace views {
+
+namespace test {

[tapted, 2016/06/06 04:09:52]

remove `test` -- we can have an anonymous namespace, which will give the classes
internal linkage

[Patti Lor, 2016/06/10 01:36:10]

Done.

+
+class AXIgnoredTestView : public View {

[tapted, 2016/06/06 04:09:52]

nit: remove the `AX` prefix - IgnoredTestView is good (the AX prefix implies
other stuff)

[Patti Lor, 2016/06/10 01:36:10]

Done.

+  void GetAccessibleState(ui::AXViewState* state) override {
+    state->role = ui::AX_ROLE_IGNORED;

[tapted, 2016/06/06 04:09:52]

#include "ui/accessibility/ax_enums.h" for IWYU

[Patti Lor, 2016/06/10 01:36:10]

Done.

+  }
+};

[tapted, 2016/06/06 04:09:52]

nit: private:\n  DISALLOW_COPY_AND_ASSIGN - you'll need a constructor

[Patti Lor, 2016/06/10 01:36:10]

Done.

+
+class AXTestView : public Label {

[tapted, 2016/06/06 04:09:52]

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?

[Patti Lor, 2016/06/10 01:36:10]

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.

+ public:
+  explicit AXTestView(base::string16 text) : Label(text) {}
+
+  // Label:
+  void GetAccessibleState(ui::AXViewState* state) override {
+    Label::GetAccessibleState(state);
+    state->value = state->name;
+  }
+};
+
+class NativeWidgetMacAccessibilityTest : public WidgetTest {
+ public:
+  void SetUp() override {
+    WidgetTest::SetUp();
+    widget_ = CreateTopLevelPlatformWidget();
+    widget_bounds_ = gfx::Rect(50, 50, 100, 100);
+    widget_->SetBounds(widget_bounds_);
+    NSRect ns_widget_bounds = gfx::ScreenRectToNSRect(widget_bounds_);

[tapted, 2016/06/06 04:09:52]

nit: perhaps just

midpoint_in_screen_ = gfx::ScreenPointToNSPoint(
    widget_->GetWindowBoundsInScreen().CenterPoint()));

[Patti Lor, 2016/06/10 01:36:10]

Done.

+    widget_midpoint_ =
+        NSMakePoint(NSMidX(ns_widget_bounds), NSMidY(ns_widget_bounds));
+  }
+
+  void TearDown() override {
+    widget_->CloseNow();
+    WidgetTest::TearDown();
+  }
+
+  id AttributeValueAtMidpoint(NSString* attribute) {
+    // Accessibility hit tests come in Cocoa screen coordinates.
+    id hit = [widget_->GetNativeWindow() accessibilityHitTest:widget_midpoint_];
+    id value = [hit accessibilityAttributeValue:attribute];
+    return value;
+  }
+
+  Label* AddChildLabel(gfx::Size size) {

[tapted, 2016/06/06 04:09:52]

const gfx::Size& size

[Patti Lor, 2016/06/10 01:36:10]

Done.

+    Label* label = new Label(kTestString);
+    label->SetSize(size);
+    widget()->GetContentsView()->AddChildView(label);
+    return label;
+  }
+
+  Widget* widget() { return widget_; }
+  gfx::Rect widget_bounds() { return widget_bounds_; }

[tapted, 2016/06/06 04:09:52]

we should remove |widget_bounds_| -- we can call a method on widget_ here
instead.

I think most useful would be ContentBounds() ->
widget_->GetClientAreaBoundsInScreen

[Patti Lor, 2016/06/10 01:36:10]

Done.

+
+  const base::string16 kTestString = base::ASCIIToUTF16("Green");
+
+ private:
+  gfx::Rect widget_bounds_;
+  Widget* widget_;
+
+  NSPoint widget_midpoint_;
+};
+

[tapted, 2016/06/06 04:09:52]

anonymous namespace should be closed here

[Patti Lor, 2016/06/10 01:36:10]

Done.

+TEST_F(NativeWidgetMacAccessibilityTest, ChildrenAttribute) {

[tapted, 2016/06/06 04:09:52]

All tests should have a short description describing their purpose

[Patti Lor, 2016/06/10 01:36:10]

Done.

+  // Check childless views don't have accessibility children.
+  EXPECT_EQ(0u,
+            [AttributeValueAtMidpoint(NSAccessibilityChildrenAttribute) count]);
+
+  const unsigned int num_children = 3;

[tapted, 2016/06/06 04:09:51]

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

[Patti Lor, 2016/06/10 01:36:10]

Ooh, thanks for the links - thought the k thing was for member variables only.

+  for (unsigned int i = 0; i < num_children; ++i) {
+    // Make sure the labels won't interfere with the hit test.
+    AddChildLabel(gfx::Size(1, 1));

[tapted, 2016/06/06 04:09:52]

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.

[Patti Lor, 2016/06/10 01:36:11]

Yep, it works, have changed to empty sizes.

+  }
+  widget()->Show();

[tapted, 2016/06/06 04:09:52]

can this be done in SetUp()?

[Patti Lor, 2016/06/10 01:36:11]

Done.

+
+  EXPECT_EQ(num_children,
+            [AttributeValueAtMidpoint(NSAccessibilityChildrenAttribute) count]);
+
+  // Check ignored children don't show up in the accessibility tree.
+  widget()->GetContentsView()->AddChildView(new AXIgnoredTestView);
+  EXPECT_EQ(num_children,
+            [AttributeValueAtMidpoint(NSAccessibilityChildrenAttribute) count]);
+}
+
+TEST_F(NativeWidgetMacAccessibilityTest, ParentAttribute) {
+  Label* label = AddChildLabel(widget_bounds().size());

[tapted, 2016/06/06 04:09:51]

the widget bounds should include the titlebar - perhaps
widget()->GetContentsView()->bounds() is better

[Patti Lor, 2016/06/10 01:36:10]

Done.

+  widget()->Show();
+
+  // TODO(patricialor): Figure out why NativeViewAccessibility::GetParent
+  // sometimes returns a gfx::NativeView and other times a AXPlatformNodeCocoa.

[tapted, 2016/06/06 04:09:52]

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

[Patti Lor, 2016/06/10 01:36:11]

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 :)

[tapted, 2016/06/10 03:17:17]

Acknowledged.

+  EXPECT_NSEQ(widget()->GetNativeWindow(),

[tapted, 2016/06/06 04:09:52]

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|

[Patti Lor, 2016/06/10 01:36:11]

Changed to compare the expected role of the parent instead.

+              AttributeValueAtMidpoint(NSAccessibilityParentAttribute));
+
+  // Test an ignored role parent is skipped in favor of the grandparent widget.

[tapted, 2016/06/06 04:09:52]

remove " widget". (i.e .grandparent doesn't have to be a widget.

[Patti Lor, 2016/06/10 01:36:11]

Done.

+  widget()->GetContentsView()->RemoveChildView(label);
+  View* ignored_view = new AXIgnoredTestView;
+  ignored_view->SetBoundsRect(widget_bounds());
+  ignored_view->AddChildView(label);
+  widget()->GetContentsView()->AddChildView(ignored_view);
+  EXPECT_NSEQ(widget()->GetNativeWindow(),
+              AttributeValueAtMidpoint(NSAccessibilityParentAttribute));
+}
+
+TEST_F(NativeWidgetMacAccessibilityTest, PositionAttribute) {
+  widget()->Show();
+
+  NSValue* widget_origin = [NSValue
+      valueWithPoint:gfx::ScreenPointToNSPoint(widget_bounds().bottom_left())];
+  EXPECT_NSEQ(widget_origin,
+              AttributeValueAtMidpoint(NSAccessibilityPositionAttribute));
+
+  // Check the attribute is updated when the widget is moved.
+  gfx::Rect new_bounds(25, 30, 60, 30);
+  widget()->SetBounds(new_bounds);
+  widget_origin = [NSValue
+      valueWithPoint:gfx::ScreenPointToNSPoint(new_bounds.bottom_left())];
+  EXPECT_NSEQ(widget_origin,
+              AttributeValueAtMidpoint(NSAccessibilityPositionAttribute));
+}
+
+TEST_F(NativeWidgetMacAccessibilityTest, RoleAttribute) {
+  AddChildLabel(widget_bounds().size());
+  widget()->Show();
+
+  EXPECT_NSEQ(NSAccessibilityStaticTextRole,
+              AttributeValueAtMidpoint(NSAccessibilityRoleAttribute));
+}
+
+TEST_F(NativeWidgetMacAccessibilityTest, SizeAttribute) {
+  AddChildLabel(widget_bounds().size());
+  widget()->Show();
+
+  EXPECT_EQ(widget_bounds().size(),
+            gfx::ScreenSizeFromNSSize([AttributeValueAtMidpoint(
+                NSAccessibilitySizeAttribute) sizeValue]));
+
+  // Check the attribute is updated when the widget is resized.
+  gfx::Size new_size(40, 30);
+  widget()->SetSize(new_size);
+  EXPECT_EQ(new_size, gfx::ScreenSizeFromNSSize([AttributeValueAtMidpoint(
+                          NSAccessibilitySizeAttribute) sizeValue]));
+}
+
+TEST_F(NativeWidgetMacAccessibilityTest, TitleAttribute) {
+  AddChildLabel(widget_bounds().size());
+  widget()->Show();
+
+  EXPECT_NSEQ(base::SysUTF16ToNSString(kTestString),
+              AttributeValueAtMidpoint(NSAccessibilityTitleAttribute));
+}
+
+TEST_F(NativeWidgetMacAccessibilityTest, RoleDescriptionAttribute) {
+  AddChildLabel(widget_bounds().size());
+  widget()->Show();
+
+  // TODO(patricialor): Fix - AX_ATTR_DESCRIPTION doesn't seem to be set

[tapted, 2016/06/06 04:09:52]

So this is bad, we should provide AX_ATTR_DESCRIPTION for everything. But my
earlier suggestion should fix this.

[Patti Lor, 2016/06/10 01:36:11]

Done, thanks for that!

+  // anywhere (at least outside blink)?
+  EXPECT_NSEQ(
+      base::SysUTF16ToNSString(base::ASCIIToUTF16("label")),
+      AttributeValueAtMidpoint(NSAccessibilityRoleDescriptionAttribute));
+}
+
+TEST_F(NativeWidgetMacAccessibilityTest, ValueAttribute) {
+  View* view = new AXTestView(kTestString);
+  view->SetSize(widget_bounds().size());
+  widget()->GetContentsView()->AddChildView(view);
+  widget()->Show();
+
+  EXPECT_NSEQ(base::SysUTF16ToNSString(kTestString),
+              AttributeValueAtMidpoint(NSAccessibilityValueAttribute));
+}
+
+}  // namespace test
+
+}  // namespace views