MacViews/a11y: Allow accessibility clients to set new selections in Textfields.

MacViews/a11y: Allow accessibility clients to set new selections in Textfields.

Currently, the `SelectedTextRange` a11y attribute on Views is reported as
unmodifiable, which differs from native text field elements on Mac.

This change will support allowing accessibility clients to change the text
selection (including moving the cursor position) for views::Textfields.

BUG=610591, 645596
TEST=With #secondary-ui-md turned on in chrome:flags, open the XCode
Accessibility Inspector and bring up the HTTP Authentication dialog. Type some
text into the username dialog, select some text, and inspect it with Alt+Space
on macOS 10.12 or Cmd+F7 otherwise. In the Accessibility Inspector, scroll down
to SelectedTextRange and check it's editable. Setting the location and length of
the control then changes the text selection made earlier.

Review-Url: https://codereview.chromium.org/2684203002
Cr-Commit-Position: refs/heads/master@{#451148}
Committed: https://chromium.googlesource.com/chromium/src/+/0a8d26e24fbf3e0a262f2e0fe72100c979a77428

[Patti Lor, 2017-02-09 07:30:27]

Description was changed from

==========
MacViews/a11y: Allow accessibility clients to set new selections in Textfields.

Currently, the `SelectedTextRange` a11y attribute on Views is reported as
unmodifiable, which differs from native text field elements on Mac.

This change will support allowing accessibility clients to change the text
selection (including moving the cursor position) for views::Textfields.

BUG=610591
TEST=With #secondary-ui-md turned on in chrome:flags, open the XCode
Accessibility Inspector and bring up the HTTP Authentication dialog. Type some
text into the username dialog, select some text, and inspect it with Alt+Space
on macOS 10.12 or Cmd+F7 otherwise. In the Accessibility Inspector, scroll down
to SelectedTextRange and check it's editable. Setting the location and length of
the control then changes the text selection made earlier.
==========

to

==========
MacViews/a11y: Allow accessibility clients to set new selections in Textfields.

Currently, the `SelectedTextRange` a11y attribute on Views is reported as
unmodifiable, which differs from native text field elements on Mac.

This change will support allowing accessibility clients to change the text
selection (including moving the cursor position) for views::Textfields.

BUG=610591, 645596
TEST=With #secondary-ui-md turned on in chrome:flags, open the XCode
Accessibility Inspector and bring up the HTTP Authentication dialog. Type some
text into the username dialog, select some text, and inspect it with Alt+Space
on macOS 10.12 or Cmd+F7 otherwise. In the Accessibility Inspector, scroll down
to SelectedTextRange and check it's editable. Setting the location and length of
the control then changes the text selection made earlier.
==========

[Patti Lor, 2017-02-09 07:37:37]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 07:38:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 07:59:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 07:59:46]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-09 22:51:39]

patricialor@chromium.org changed reviewers:
+ tapted@chromium.org

[Patti Lor, 2017-02-09 22:51:40]

Hi Trent, PTAL :)

[tapted, 2017-02-09 23:09:38]

lgtm with the following

https://codereview.chromium.org/2684203002/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2684203002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:436: NSRange range = [value
rangeValue];
I think we should only do this if data.action == ui::AX_ACTION_SET_SELECTION

NSValue might be used for other attributes, but it might not be an NSRange
that's stored there (but we can assume that if set data.action to
ui::AX_ACTION_SET_SELECTION above then this is what's in there).

https://codereview.chromium.org/2684203002/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right):

https://codereview.chromium.org/2684203002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:415: // Check it's
not possible to change the selection range when it's read-only.
Check that it *is* possible? Which I think is right - you can still change the
selection, and copy the text, just not write to the selection.

[nektarios, 2017-02-09 23:17:57]

1. 1. + data.focus_offset
I am about to send out a patch that takes the anchor_id and focus_id into
onsideration. Could you please set those too?

2. + data.focus_offset = range.location + range.length;
Use |NSMaxRange(_:)|

3. Personally, I would advise using ObjC literals. You may read
https://clang.llvm.org/docs/ObjectiveCLiterals.html

For example:
NSRange range = NSMakeRange(1, 3);
can be replaced by
NSRange range = {1, 3};

NSValue value = [NSValue valueWithRange:range];
can be replaced with
NSValue value = @(range);

with


NSValue bla = [NSValue valueWithBla:];



[NSValue valueWithBool:YES]
can be replaced by
@YES

[Patti Lor, 2017-02-13 04:58:37]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 04:58:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 05:34:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 05:34:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Patti Lor, 2017-02-13 23:31:03]

Hi both, thanks for the reviews!

@nektarios - I'm not sure what using setting the anchor_id and focus_id would
involve - as far as I can tell, Views elements don't set node ids, and I don't
think there is any equivalent id that NSAccessibility provides, so I'm not sure
if this would be useful. Could you link the patch where you're doing this?

https://codereview.chromium.org/2684203002/diff/20001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_mac.mm (right):

https://codereview.chromium.org/2684203002/diff/20001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:436: NSRange range = [value
rangeValue];
On 2017/02/09 23:09:37, tapted wrote:
> I think we should only do this if data.action == ui::AX_ACTION_SET_SELECTION
> 
> NSValue might be used for other attributes, but it might not be an NSRange
> that's stored there (but we can assume that if set data.action to
> ui::AX_ACTION_SET_SELECTION above then this is what's in there).

Done, thanks :)

https://codereview.chromium.org/2684203002/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right):

https://codereview.chromium.org/2684203002/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:415: // Check it's
not possible to change the selection range when it's read-only.
On 2017/02/09 23:09:37, tapted wrote:
> Check that it *is* possible? Which I think is right - you can still change the
> selection, and copy the text, just not write to the selection.

Done, thanks. I had to change Textfield::SetSelectionRange() to do this though -
it looks like msw@ was the one who wrote it originally (back in 2014). But I
think the change is correct, because if you try and do it with the mouse (on a
read-only textfield) it lets you select. I'll get him to do owner's review when
this is ready.

[tapted, 2017-02-14 05:45:32]

https://codereview.chromium.org/2684203002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2684203002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1418: if (!enabled() ||
!range.IsValid())
as discussed - I'm not sure about changing this without changing a few other
sites in textfield.cc. And after checking password fields in Cocoa, the right
thing to do might be to make the selected range attribute unmodifiable via a11y
to be consistent (and having the test verify this). But that's fine as a
follow-up with a note in the test that the behaviour is inconsistent with Cocoa
(i.e. NSAccessibilitySelectedTextRangeAttribute shouldn't be settable after
doing SetReadOnly(true).

[Patti Lor, 2017-02-15 05:40:07]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 05:40:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 06:21:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 06:21:28]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-15 06:34:47]

Thanks Trent, PTAL

https://codereview.chromium.org/2684203002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2684203002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1418: if (!enabled() ||
!range.IsValid())
On 2017/02/14 05:45:32, tapted wrote:
> as discussed - I'm not sure about changing this without changing a few other
> sites in textfield.cc. And after checking password fields in Cocoa, the right
> thing to do might be to make the selected range attribute unmodifiable via
a11y
> to be consistent (and having the test verify this). But that's fine as a
> follow-up with a note in the test that the behaviour is inconsistent with
Cocoa
> (i.e. NSAccessibilitySelectedTextRangeAttribute shouldn't be settable after
> doing SetReadOnly(true).

Yep, so as discovered & discussed offline, selection-only NSTextfields allow
setting SelectedTextRange via a11y, but not NSSecureTextfields. I've reverted
this file, and added a note in the test to say that we deviate from Cocoa
behaviour here & filed https://crbug.com/692362.

[Patti Lor, 2017-02-15 06:34:48]

Thanks Trent, PTAL

https://codereview.chromium.org/2684203002/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2684203002/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1418: if (!enabled() ||
!range.IsValid())
On 2017/02/14 05:45:32, tapted wrote:
> as discussed - I'm not sure about changing this without changing a few other
> sites in textfield.cc. And after checking password fields in Cocoa, the right
> thing to do might be to make the selected range attribute unmodifiable via
a11y
> to be consistent (and having the test verify this). But that's fine as a
> follow-up with a note in the test that the behaviour is inconsistent with
Cocoa
> (i.e. NSAccessibilitySelectedTextRangeAttribute shouldn't be settable after
> doing SetReadOnly(true).

Yep, so as discovered & discussed offline, selection-only NSTextfields allow
setting SelectedTextRange via a11y, but not NSSecureTextfields. I've reverted
this file, and added a note in the test to say that we deviate from Cocoa
behaviour here & filed https://crbug.com/692362.

[Patti Lor, 2017-02-15 06:34:48]

Thanks Trent, PTAL

[tapted, 2017-02-15 22:31:52]

lgtm - thanks!

https://codereview.chromium.org/2684203002/diff/60001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right):

https://codereview.chromium.org/2684203002/diff/60001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:417: // a11y in
non-editable, selectable NSTextfields. https://crbug.com/692362
.. selectable NSTextfields (unless they are password fields).

[dmazzoni, 2017-02-15 22:36:10]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2017-02-15 22:36:11]

lgtm

[Patti Lor, 2017-02-15 23:22:05]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 23:23:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nektarios, 2017-02-15 23:23:34]

Sorry got caught up in other work. I'll add the anchor / focus node IDs myself.

[nektarios, 2017-02-15 23:23:48]

nektar@chromium.org changed reviewers:
+ nektar@chromium.org

[nektarios, 2017-02-15 23:23:49]

lgtm

[Patti Lor, 2017-02-16 00:34:11]

@nektarios: No worries, thanks for that.
Thanks all for the reviews! Updated the comment as tapted@ suggested.

https://codereview.chromium.org/2684203002/diff/60001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_accessibility_unittest.mm (right):

https://codereview.chromium.org/2684203002/diff/60001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_accessibility_unittest.mm:417: // a11y in
non-editable, selectable NSTextfields. https://crbug.com/692362
On 2017/02/15 22:31:51, tapted wrote:
> .. selectable NSTextfields (unless they are password fields).

Done.

[commit-bot: I haz the power, 2017-02-16 01:27:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 01:27:13]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Patti Lor, 2017-02-16 01:52:44]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2017-02-16 01:52:44]

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/2684203002/#ps80001
(title: "Update comment.")

[commit-bot: I haz the power, 2017-02-16 01:54:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 03:42:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 03:42:29]

Failed to apply patch for
ui/views/widget/native_widget_mac_accessibility_unittest.mm:
While running git apply --index -p1;
  error: patch failed:
ui/views/widget/native_widget_mac_accessibility_unittest.mm:411
  error: ui/views/widget/native_widget_mac_accessibility_unittest.mm: patch does
not apply

Patch:       ui/views/widget/native_widget_mac_accessibility_unittest.mm
Index: ui/views/widget/native_widget_mac_accessibility_unittest.mm
diff --git a/ui/views/widget/native_widget_mac_accessibility_unittest.mm
b/ui/views/widget/native_widget_mac_accessibility_unittest.mm
index
3d641d190f9eab41fa0d4a97d726766736902263..5bc9870d3086c5d63e64f76eb970a079bbb99827
100644
--- a/ui/views/widget/native_widget_mac_accessibility_unittest.mm
+++ b/ui/views/widget/native_widget_mac_accessibility_unittest.mm
@@ -411,6 +411,31 @@ TEST_F(NativeWidgetMacAccessibilityTest,
TextfieldWritableAttributes) {
   // Make sure the cursor is at the end of the replacement.
   EXPECT_EQ(gfx::Range(front.length() + replacement.length()),
             textfield->GetSelectedRange());
+
+  // Check it's not possible to change the selection range when read-only. Note
+  // that this behavior is inconsistent with Cocoa - selections can be set via
+  // a11y in selectable NSTextfields (unless they are password fields).
+  // https://crbug.com/692362
+  textfield->SetReadOnly(true);
+  EXPECT_FALSE([ax_node accessibilityIsAttributeSettable:
+                           NSAccessibilitySelectedTextRangeAttribute]);
+  textfield->SetReadOnly(false);
+  EXPECT_TRUE([ax_node accessibilityIsAttributeSettable:
+                           NSAccessibilitySelectedTextRangeAttribute]);
+
+  // Change the selection to a valid range within the text.
+  [ax_node accessibilitySetValue:[NSValue valueWithRange:NSMakeRange(2, 5)]
+                    forAttribute:NSAccessibilitySelectedTextRangeAttribute];
+  EXPECT_EQ(gfx::Range(2, 7), textfield->GetSelectedRange());
+  // If the length is longer than the value length, default to the max
possible.
+  [ax_node accessibilitySetValue:[NSValue valueWithRange:NSMakeRange(0, 1000)]
+                    forAttribute:NSAccessibilitySelectedTextRangeAttribute];
+  EXPECT_EQ(gfx::Range(0, textfield->text().length()),
+            textfield->GetSelectedRange());
+  // Check just moving the cursor works, too.
+  [ax_node accessibilitySetValue:[NSValue valueWithRange:NSMakeRange(5, 0)]
+                    forAttribute:NSAccessibilitySelectedTextRangeAttribute];
+  EXPECT_EQ(gfx::Range(5, 5), textfield->GetSelectedRange());
 }
 
 }  // namespace views

[Patti Lor, 2017-02-16 05:04:42]

The CQ bit was checked by patricialor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 05:05:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 07:09:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 07:09:01]

Dry run: This issue passed the CQ dry run.

[Patti Lor, 2017-02-16 23:05:02]

The CQ bit was checked by patricialor@chromium.org

[Patti Lor, 2017-02-16 23:05:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
nektar@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2684203002/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-02-16 23:06:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 00:11:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487286302363840,
"parent_rev": "c4dbfa9fc24da8f0be72945b3e4d5e6ad694c9bb", "commit_rev":
"0a8d26e24fbf3e0a262f2e0fe72100c979a77428"}

[commit-bot: I haz the power, 2017-02-17 00:12:24]

Description was changed from

==========
MacViews/a11y: Allow accessibility clients to set new selections in Textfields.

Currently, the `SelectedTextRange` a11y attribute on Views is reported as
unmodifiable, which differs from native text field elements on Mac.

This change will support allowing accessibility clients to change the text
selection (including moving the cursor position) for views::Textfields.

BUG=610591, 645596
TEST=With #secondary-ui-md turned on in chrome:flags, open the XCode
Accessibility Inspector and bring up the HTTP Authentication dialog. Type some
text into the username dialog, select some text, and inspect it with Alt+Space
on macOS 10.12 or Cmd+F7 otherwise. In the Accessibility Inspector, scroll down
to SelectedTextRange and check it's editable. Setting the location and length of
the control then changes the text selection made earlier.
==========

to

==========
MacViews/a11y: Allow accessibility clients to set new selections in Textfields.

Currently, the `SelectedTextRange` a11y attribute on Views is reported as
unmodifiable, which differs from native text field elements on Mac.

This change will support allowing accessibility clients to change the text
selection (including moving the cursor position) for views::Textfields.

BUG=610591, 645596
TEST=With #secondary-ui-md turned on in chrome:flags, open the XCode
Accessibility Inspector and bring up the HTTP Authentication dialog. Type some
text into the username dialog, select some text, and inspect it with Alt+Space
on macOS 10.12 or Cmd+F7 otherwise. In the Accessibility Inspector, scroll down
to SelectedTextRange and check it's editable. Setting the location and length of
the control then changes the text selection made earlier.

Review-Url: https://codereview.chromium.org/2684203002
Cr-Commit-Position: refs/heads/master@{#451148}
Committed:
https://chromium.googlesource.com/chromium/src/+/0a8d26e24fbf3e0a262f2e0fe721...
==========

[commit-bot: I haz the power, 2017-02-17 00:12:26]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0a8d26e24fbf3e0a262f2e0fe721...