Don't expose Android set selection accessibility action for empty text fields

Don't expose Android set selection accessibility action for empty text fields

We shouldn't be exposing the SET_SELECTION action on text fields that
only have placeholder text, that's how TalkBack distinguishes between
real text and placeholder text.

Also fixes bug where CUT, COPY, and PASTE actions were set in the
base class but not the Lollipop subclass.

BUG=645161
Committed: https://crrev.com/f43b118065d29486b240bc3546ac566c01959ebb
Cr-Commit-Position: refs/heads/master@{#424217}

[dmazzoni, 2016-09-08 18:03:33]

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

[commit-bot: I haz the power, 2016-09-08 18:04:00]

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

[dmazzoni, 2016-09-08 18:04:10]

Description was changed from

==========
Android placeholder text shouldn't be selectable

We shouldn't be exposing the SET_SELECTION action on text fields that
only have placeholder text, that's how TalkBack distinguishes between
real text and placeholder text.

BUG=645161
==========

to

==========
Android placeholder text shouldn't be selectable

We shouldn't be exposing the SET_SELECTION action on text fields that
only have placeholder text, that's how TalkBack distinguishes between
real text and placeholder text.

Also fixes bug where CUT, COPY, and PASTE actions were set in the
base class but not the Lollipop subclass.

BUG=645161
==========

[dmazzoni, 2016-09-08 18:04:11]

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

[commit-bot: I haz the power, 2016-09-08 18:48:50]

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

[commit-bot: I haz the power, 2016-09-08 18:48:50]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2016-09-08 20:49:47]

https://codereview.chromium.org/2319903003/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2319903003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:428:
!node->GetValue().empty());
If I understand the bug correctly, don't you want to check for the placeholder
attribute here?

https://codereview.chromium.org/2319903003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java
(right):

https://codereview.chromium.org/2319903003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:830:
if (hasNonEmptyValue) {
How about read-only text fields?

[dmazzoni, 2016-09-14 18:56:39]

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

[commit-bot: I haz the power, 2016-09-14 18:57:23]

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, 2016-09-14 20:07:45]

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

[commit-bot: I haz the power, 2016-09-14 20:07:46]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dmazzoni, 2016-09-14 22:39:48]

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

[commit-bot: I haz the power, 2016-09-14 22:40:16]

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, 2016-09-15 00:25:09]

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

[commit-bot: I haz the power, 2016-09-15 00:25:10]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dmazzoni, 2016-10-05 19:40:48]

Fixed and updated tests.

https://codereview.chromium.org/2319903003/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2319903003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:428:
!node->GetValue().empty());
On 2016/09/08 at 20:49:47, David Tseng wrote:
> If I understand the bug correctly, don't you want to check for the placeholder
attribute here?

No, because placeholder was just one possible example of an accessible label for
the text field. The real issue is that we want to know whether it has a value or
not.

There was a bug in GetValue, though - it was sometimes returning inner text. I
fixed that and added tests.

https://codereview.chromium.org/2319903003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java
(right):

https://codereview.chromium.org/2319903003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:830:
if (hasNonEmptyValue) {
On 2016/09/08 at 20:49:47, David Tseng wrote:
> How about read-only text fields?

Hmmm, it seems to me that you still ought to be able to select and copy text in
a read-only text box.

I'll address readonly in a follow-up if that's okay.

[dmazzoni, 2016-10-05 19:42:07]

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

[commit-bot: I haz the power, 2016-10-05 19:42:33]

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, 2016-10-05 20:59:33]

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

[commit-bot: I haz the power, 2016-10-05 20:59:34]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[David Tseng, 2016-10-06 16:27:58]

https://codereview.chromium.org/2319903003/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_manager_android.cc
(right):

https://codereview.chromium.org/2319903003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_manager_android.cc:428:
!node->GetValue().empty());
On 2016/10/05 19:40:48, dmazzoni_ooo_until_oct_10 wrote:
> On 2016/09/08 at 20:49:47, David Tseng wrote:
> > If I understand the bug correctly, don't you want to check for the
placeholder
> attribute here?
> 
> No, because placeholder was just one possible example of an accessible label
for
> the text field. The real issue is that we want to know whether it has a value
or
> not.
> 
> There was a bug in GetValue, though - it was sometimes returning inner text. I
> fixed that and added tests.
> 


Value comes from value description and string value in Blink. Value description
uses aria value text which I guess isn't selectable. String value uses the text
value of the text field which is selectable. If the node is a select, it uses
aria label, which isn't selectable.

In conclusion, I don't think value is a good indicator of what's selectable or
not. Also, are you expecting this gets called only on text fields? Is Talkback
thinking about supporting dom selections? Might be nice to be able to select a
link, for example, to paste into a Doc.

[dmazzoni, 2016-10-10 15:19:31]

On 2016/10/06 16:27:58, David Tseng wrote:
> Value comes from value description and string value in Blink. Value
description
> uses aria value text which I guess isn't selectable. String value uses the
text
> value of the text field which is selectable. If the node is a select, it uses
> aria label, which isn't selectable.

Note that this code I added is only used for editable text fields. For an input,
textarea, or contenteditable, we implement GetValue() correctly and
aria-valuetext doesn't apply, that's only for range controls.

> In conclusion, I don't think value is a good indicator of what's selectable or
> not. Also, are you expecting this gets called only on text fields? Is Talkback
> thinking about supporting dom selections? Might be nice to be able to select a
> link, for example, to paste into a Doc.

This is only used for editable text controls, to indicate whether or not we
should
expose the SET_SELECTION action on that control.

TalkBack is looking into supporting DOM selection in the future but right now
they
don't have the APIs for that. In particular, there's no API that would allow you
to
make the start and end of the selection in different nodes.

I think this change is correct for the narrow place where it's used. Would any
name changes help clarify it?

[David Tseng, 2016-10-10 16:06:54]

lgtm

Update the description to say Don't expose action set selection when a text
field has empty value.

[dmazzoni, 2016-10-10 16:33:19]

Description was changed from

==========
Android placeholder text shouldn't be selectable

We shouldn't be exposing the SET_SELECTION action on text fields that
only have placeholder text, that's how TalkBack distinguishes between
real text and placeholder text.

Also fixes bug where CUT, COPY, and PASTE actions were set in the
base class but not the Lollipop subclass.

BUG=645161
==========

to

==========
Don't expose Android set selection accessibility action for empty text fields

We shouldn't be exposing the SET_SELECTION action on text fields that
only have placeholder text, that's how TalkBack distinguishes between
real text and placeholder text.

Also fixes bug where CUT, COPY, and PASTE actions were set in the
base class but not the Lollipop subclass.

BUG=645161
==========

[dmazzoni, 2016-10-10 16:33:30]

Title updated to make it more clear, thanks

[dmazzoni, 2016-10-10 19:13:22]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-10-10 19:13:23]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2319903003/#ps100001
(title: "Update one test expectation")

[commit-bot: I haz the power, 2016-10-10 19:13:40]

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, 2016-10-10 20:14:12]

Description was changed from

==========
Don't expose Android set selection accessibility action for empty text fields

We shouldn't be exposing the SET_SELECTION action on text fields that
only have placeholder text, that's how TalkBack distinguishes between
real text and placeholder text.

Also fixes bug where CUT, COPY, and PASTE actions were set in the
base class but not the Lollipop subclass.

BUG=645161
==========

to

==========
Don't expose Android set selection accessibility action for empty text fields

We shouldn't be exposing the SET_SELECTION action on text fields that
only have placeholder text, that's how TalkBack distinguishes between
real text and placeholder text.

Also fixes bug where CUT, COPY, and PASTE actions were set in the
base class but not the Lollipop subclass.

BUG=645161
==========

[commit-bot: I haz the power, 2016-10-10 20:14:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-10 20:17:01]

Description was changed from

==========
Don't expose Android set selection accessibility action for empty text fields

We shouldn't be exposing the SET_SELECTION action on text fields that
only have placeholder text, that's how TalkBack distinguishes between
real text and placeholder text.

Also fixes bug where CUT, COPY, and PASTE actions were set in the
base class but not the Lollipop subclass.

BUG=645161
==========

to

==========
Don't expose Android set selection accessibility action for empty text fields

We shouldn't be exposing the SET_SELECTION action on text fields that
only have placeholder text, that's how TalkBack distinguishes between
real text and placeholder text.

Also fixes bug where CUT, COPY, and PASTE actions were set in the
base class but not the Lollipop subclass.

BUG=645161

Committed: https://crrev.com/f43b118065d29486b240bc3546ac566c01959ebb
Cr-Commit-Position: refs/heads/master@{#424217}
==========

[commit-bot: I haz the power, 2016-10-10 20:17:02]

Patchset 6 (id:??) landed as
https://crrev.com/f43b118065d29486b240bc3546ac566c01959ebb
Cr-Commit-Position: refs/heads/master@{#424217}