MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

This requires the label to respond to AXValue, not just AXTitle.
views::Label currently reports nil for its AXValue. Cocoa labels are
just readonly NSTextFields, so they have AXValue.

Fix by sharing more responders used for text fields currently. This
brought in to question the behaviour for secure text fields, which
appears to have changed in macOS. Get the shared code up to speed with
current behavior.

BUG=657945
Review-Url: https://codereview.chromium.org/2934993002
Cr-Commit-Position: refs/heads/master@{#479582}
Committed: https://chromium.googlesource.com/chromium/src/+/a523016fb1f4607afe920e467b73d2ff6473d5f1

[tapted, 2017-06-13 04:30:58]

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

[commit-bot: I haz the power, 2017-06-13 04:31:11]

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-06-13 05:30:36]

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

[commit-bot: I haz the power, 2017-06-13 05:30:37]

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_...)

[tapted, 2017-06-13 05:35:57]

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

[commit-bot: I haz the power, 2017-06-13 05:36:09]

Dry run: CQ is trying da patch.

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

[tapted, 2017-06-13 05:38:25]

Description was changed from

==========
MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

This requires the label to respond to AXValue, no just AXTitle.

BUG=657945
==========

to

==========
MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

This requires the label to respond to AXValue, not just AXTitle.

BUG=657945
==========

[tapted, 2017-06-13 05:51:12]

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

[commit-bot: I haz the power, 2017-06-13 05:51:17]

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-06-13 06:10:22]

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

[commit-bot: I haz the power, 2017-06-13 06:10:23]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-06-13 08:04:33]

Description was changed from

==========
MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

This requires the label to respond to AXValue, not just AXTitle.

BUG=657945
==========

to

==========
MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

This requires the label to respond to AXValue, not just AXTitle.
views::Label currently reports nil for its AXValue. Cocoa labels are
just readonly NSTextFields, so they have AXValue.

BUG=657945
==========

[tapted, 2017-06-13 08:04:34]

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

[tapted, 2017-06-13 08:05:13]

Hi Dominic, please take a look

[dmazzoni, 2017-06-13 20:35:55]

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

https://codereview.chromium.org/2934993002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:293: if
(node_->GetData().HasState(ui::AX_STATE_PROTECTED))
I thought we were supposed to expose something for
protected fields. I'll have to look more closely at
what Safari does today.

On other platforms we just expose the bullets directly, i.e. if
the user types "hunter2" we expose "*******" as the value
but we support all cursor, selection, and value apis on that
field as if it was unprotected and really contained stars.

I'm wondering if this is still correct or it's historical or wrong.

https://codereview.chromium.org/2934993002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:296: return [value
isKindOfClass:[NSString class]] ? value : [self AXTitle];
This seems like a backwards way of doing it to me.

I'd have getStringValue return AX_ATTR_VALUE, and then
reimplement AXValue in terms of getStringValue rather
than the other way around.

Internally, "value" means the string value of a form control,
and it never means anything else. On Mac, AXValue is overloaded
to expose a lot of other things, but we should keep that logic
inside AXValue and not in helper functions that access internals,
if that makes sense.

[chromium-reviews, 2017-06-13 21:25:51]

On 6/13/2017 4:35 PM, dmazzoni@chromium.org wrote:
>
>
https://codereview.chromium.org/2934993002/diff/40001/ui/accessibility/platfo...
> File ui/accessibility/platform/ax_platform_node_mac.mm (right):
>
>
https://codereview.chromium.org/2934993002/diff/40001/ui/accessibility/platfo...
> ui/accessibility/platform/ax_platform_node_mac.mm:293: if
> (node_->GetData().HasState(ui::AX_STATE_PROTECTED))
> I thought we were supposed to expose something for
> protected fields. I'll have to look more closely at
> what Safari does today.
>
> On other platforms we just expose the bullets directly, i.e. if
> the user types "hunter2" we expose "*******" as the value
> but we support all cursor, selection, and value apis on that
> field as if it was unprotected and really contained stars.

Safari exposes the value that appears in the password field. If this is 
a set of bullets, then it will expose a set of bullets, if a set of 
black circles, then it will expose that.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tapted, 2017-06-14 06:59:40]

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

[commit-bot: I haz the power, 2017-06-14 06:59:50]

Dry run: CQ is trying da patch.

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

[tapted, 2017-06-14 07:18:58]

Description was changed from

==========
MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

This requires the label to respond to AXValue, not just AXTitle.
views::Label currently reports nil for its AXValue. Cocoa labels are
just readonly NSTextFields, so they have AXValue.

BUG=657945
==========

to

==========
MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

This requires the label to respond to AXValue, not just AXTitle.
views::Label currently reports nil for its AXValue. Cocoa labels are
just readonly NSTextFields, so they have AXValue.

Fix by sharing more responders used for text fields currently. This
brought in to question the behaviour for secure text fields, which
appears to have changed in macOS. Get the shared code up to speed with
current behavior.

BUG=657945
==========

[tapted, 2017-06-14 07:21:59]

PTAL

On 2017/06/13 21:25:51, chromium-reviews wrote:
> Safari exposes the value that appears in the password field. If this is 
> a set of bullets, then it will expose a set of bullets, if a set of 
> black circles, then it will expose that.

I've implemented this by trusting the value put on AXNodeData to be
appropriately masked.

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

https://codereview.chromium.org/2934993002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:293: if
(node_->GetData().HasState(ui::AX_STATE_PROTECTED))
On 2017/06/13 20:35:54, dmazzoni wrote:
> I thought we were supposed to expose something for
> protected fields. I'll have to look more closely at
> what Safari does today.
> 
> On other platforms we just expose the bullets directly, i.e. if
> the user types "hunter2" we expose "*******" as the value
> but we support all cursor, selection, and value apis on that
> field as if it was unprotected and really contained stars.
> 
> I'm wondering if this is still correct or it's historical or wrong.

My best guess is that macOS had a regression that got fixed, but was still
present when the code first went in, and the goal was to mimic what AppKit does.
I'm pretty sure I've looked at what AppKit was doing in the past, and it's quite
different to the current behaviour. There are some openradar notes about broken
secure text fields on Mac too - e.g. http://openradar.appspot.com/12206825 and
http://openradar.appspot.com/24607522

Anyway, I got all the secure text input stuff up to speed with current behaviour
and added test coverage.

https://codereview.chromium.org/2934993002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_mac.mm:296: return [value
isKindOfClass:[NSString class]] ? value : [self AXTitle];
On 2017/06/13 20:35:54, dmazzoni wrote:
> This seems like a backwards way of doing it to me.
> 
> I'd have getStringValue return AX_ATTR_VALUE, and then
> reimplement AXValue in terms of getStringValue rather
> than the other way around.
> 
> Internally, "value" means the string value of a form control,
> and it never means anything else. On Mac, AXValue is overloaded
> to expose a lot of other things, but we should keep that logic
> inside AXValue and not in helper functions that access internals,
> if that makes sense.

Part of the concern was that some of the methods were assuming that AXValue
would return a string. E.g. AXNumberOfCharacters returning [[self AXValue]
length] feels dangerous to me -- it's fine if the AXValue is nil, but if it's an
NSValue, asking for length will throw an exception.

I've renamed this and moved things around -- see what you think.

[commit-bot: I haz the power, 2017-06-14 07:46:06]

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

[commit-bot: I haz the power, 2017-06-14 07:46:07]

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_...)

[tapted, 2017-06-14 11:44:32]

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

[commit-bot: I haz the power, 2017-06-14 11:44:48]

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-06-14 12:55:03]

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

[commit-bot: I haz the power, 2017-06-14 12:55:04]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-06-14 16:35:52]

On 2017/06/14 07:21:59, tapted wrote:
> > I'd have getStringValue return AX_ATTR_VALUE, and then
> > reimplement AXValue in terms of getStringValue rather
> > than the other way around.
> > 
> > Internally, "value" means the string value of a form control,
> > and it never means anything else. On Mac, AXValue is overloaded
> > to expose a lot of other things, but we should keep that logic
> > inside AXValue and not in helper functions that access internals,
> > if that makes sense.
> 
> Part of the concern was that some of the methods were assuming that AXValue
> would return a string. E.g. AXNumberOfCharacters returning [[self AXValue]
> length] feels dangerous to me -- it's fine if the AXValue is nil, but if it's
an
> NSValue, asking for length will throw an exception.
> 
> I've renamed this and moved things around -- see what you think.

Ah, OK.

I played around with Accessibility Inspector some more. I see what you
mean about native labels, for example in System Preferences. They're
just read-only NSTextFields, so they support all of the text field attributes
like AXNumberOfCharacters and AXVisibleCharacterRange, even though
the role is AXStaticText.

In Safari, an AXStaticText in web content doesn't expose any of those
attributes.
However, it does support all of the text marker attributes which I think is
probably
more fundamental to advanced reading functions.

My only possible concern here is that if we end up merging this code with
the web accessibility code (e.g. browser_accessibility_cocoa), we'll be exposing
a different set of attributes on the web than Safari does. Even if things work
fine that can make it slower to debug other unrelated issues.

Still, if the current change is sufficient to allow a label to be read
word-by-word
then we should do it. We'll just have to decide later if we want to handle the
web differently or not.

[dmazzoni, 2017-06-14 16:37:33]

lgtm

[tapted, 2017-06-15 01:58:09]

On 2017/06/14 16:35:52, dmazzoni wrote:
> On 2017/06/14 07:21:59, tapted wrote:
> > > I'd have getStringValue return AX_ATTR_VALUE, and then
> > > reimplement AXValue in terms of getStringValue rather
> > > than the other way around.
> > > 
> > > Internally, "value" means the string value of a form control,
> > > and it never means anything else. On Mac, AXValue is overloaded
> > > to expose a lot of other things, but we should keep that logic
> > > inside AXValue and not in helper functions that access internals,
> > > if that makes sense.
> > 
> > Part of the concern was that some of the methods were assuming that AXValue
> > would return a string. E.g. AXNumberOfCharacters returning [[self AXValue]
> > length] feels dangerous to me -- it's fine if the AXValue is nil, but if
it's
> an
> > NSValue, asking for length will throw an exception.
> > 
> > I've renamed this and moved things around -- see what you think.
> 
> Ah, OK.
> 
> I played around with Accessibility Inspector some more. I see what you
> mean about native labels, for example in System Preferences. They're
> just read-only NSTextFields, so they support all of the text field attributes
> like AXNumberOfCharacters and AXVisibleCharacterRange, even though
> the role is AXStaticText.
> 
> In Safari, an AXStaticText in web content doesn't expose any of those
> attributes.
> However, it does support all of the text marker attributes which I think is
> probably
> more fundamental to advanced reading functions.
> 
> My only possible concern here is that if we end up merging this code with
> the web accessibility code (e.g. browser_accessibility_cocoa), we'll be
exposing
> a different set of attributes on the web than Safari does. Even if things work
> fine that can make it slower to debug other unrelated issues.
> 
> Still, if the current change is sufficient to allow a label to be read
> word-by-word
> then we should do it. We'll just have to decide later if we want to handle the
> web differently or not.

Yup - responding to AXValue gets the word-by-word navigation happening. I'll
keep an eye on the web stuff while refactoring.

[tapted, 2017-06-15 01:58:20]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-06-15 01:58:33]

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-06-15 02:03:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497491900182810,
"parent_rev": "2f7ee461cdd933ac5ed72f737b9ea65c5b19a28c", "commit_rev":
"a523016fb1f4607afe920e467b73d2ff6473d5f1"}

[commit-bot: I haz the power, 2017-06-15 02:03:34]

Description was changed from

==========
MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

This requires the label to respond to AXValue, not just AXTitle.
views::Label currently reports nil for its AXValue. Cocoa labels are
just readonly NSTextFields, so they have AXValue.

Fix by sharing more responders used for text fields currently. This
brought in to question the behaviour for secure text fields, which
appears to have changed in macOS. Get the shared code up to speed with
current behavior.

BUG=657945
==========

to

==========
MacViews a11y: Allow VoiceOver to read out views::Label word-by-word.

This requires the label to respond to AXValue, not just AXTitle.
views::Label currently reports nil for its AXValue. Cocoa labels are
just readonly NSTextFields, so they have AXValue.

Fix by sharing more responders used for text fields currently. This
brought in to question the behaviour for secure text fields, which
appears to have changed in macOS. Get the shared code up to speed with
current behavior.

BUG=657945

Review-Url: https://codereview.chromium.org/2934993002
Cr-Commit-Position: refs/heads/master@{#479582}
Committed:
https://chromium.googlesource.com/chromium/src/+/a523016fb1f4607afe920e467b73...
==========

[commit-bot: I haz the power, 2017-06-15 02:03:36]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a523016fb1f4607afe920e467b73...