Fix TalkBack feedback for password fields in Android O

Fix TalkBack feedback for password fields in Android O

Prior to O, we were supposed to expose either the value of a password field,
or just dots, depending on a value of a system setting, and independent of
what's shown visually.

In O, we're now supposed to expose what's visually shown, whether that's
characters or dots.

HOWEVER, if a character is replaced with a dot we have to update our internal
state, but not fire an Android event with that character change, to keep it
silent.

This fix reverts the change to LayoutText landed by Paul Miller in r468393
(and merged back to M58) because it was suppressing changes to the internal
accessibility state when a character was replaced with a dot. Instead now we
keep the internal state updated but suppress that particular event from being
fired on Android.

As a minor improvement, this change implements setTextSelection
on AccessibilityNodeInfo, which enables TalkBack to announce when
the cursor reaches the beginning or end of a text field the same
way as it does with native fields.

BUG=721663, 716212
TBR=wangxianzhu
NOTRY=true

Review-Url: https://codereview.chromium.org/2881723002
Cr-Commit-Position: refs/heads/master@{#471383}
Committed: https://chromium.googlesource.com/chromium/src/+/4e36c746c6c51486f9e57e47a946ee35918fd9f0

[dmazzoni, 2017-05-12 06:41:07]

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

[commit-bot: I haz the power, 2017-05-12 06:41:29]

Dry run: CQ is trying da patch.

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

[dmazzoni, 2017-05-12 06:44:47]

Description was changed from

==========
Fix TalkBack feedback for password fields in Android O

Prior to O, we were supposed to expose either the value of a password field,
or just dots, depending on a value of a system setting, and independent of
what's shown visually.

In O, we're now supposed to expose what's visually shown, whether that's
characters or dots.

HOWEVER, if a character is replaced with a dot we have to update our internal
state, but not fire an Android event with that character change, to keep it
silent.

This fix reverts the change to LayoutText landed by Paul Miller in r468393
(and merged back to M58) because it was suppressing changes to the internal
accessibility state when a character was replaced with a dot. Instead now we
keep the internal state updated but suppress that particular event from being
fired on Android.

BUG=721663
==========

to

==========
Fix TalkBack feedback for password fields in Android O

Prior to O, we were supposed to expose either the value of a password field,
or just dots, depending on a value of a system setting, and independent of
what's shown visually.

In O, we're now supposed to expose what's visually shown, whether that's
characters or dots.

HOWEVER, if a character is replaced with a dot we have to update our internal
state, but not fire an Android event with that character change, to keep it
silent.

This fix reverts the change to LayoutText landed by Paul Miller in r468393
(and merged back to M58) because it was suppressing changes to the internal
accessibility state when a character was replaced with a dot. Instead now we
keep the internal state updated but suppress that particular event from being
fired on Android.

BUG=721663,716212
TBR=wangxianzhu
==========

[dmazzoni, 2017-05-12 06:44:47]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org, paulmiller@chromium.org, wangxianzhu@chromium.org

[dmazzoni, 2017-05-12 06:46:16]

https://codereview.chromium.org/2881723002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2881723002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutText.cpp:1721: if (!force &&
Equal(text_.Impl(), text.Get()))
Note that the changes in this file are just a revert of
https://codereview.chromium.org/2846133002/

[aboxhall, 2017-05-12 07:09:34]

lgtm

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

https://codereview.chromium.org/2881723002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:1201:
return
(Settings.System.getInt(mContentViewCore.getContext().getContentResolver(),
Suggestion: pull this out into a local var to make this formatting less weird

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

https://codereview.chromium.org/2881723002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/KitKatBrowserAccessibilityManager.java:42:
node.setTextSelection(selectionStartIndex, selectionEndIndex);
Is this part of the fix as well? Could it be mentioned in the CL description?

[dmazzoni, 2017-05-12 07:33:33]

Description was changed from

==========
Fix TalkBack feedback for password fields in Android O

Prior to O, we were supposed to expose either the value of a password field,
or just dots, depending on a value of a system setting, and independent of
what's shown visually.

In O, we're now supposed to expose what's visually shown, whether that's
characters or dots.

HOWEVER, if a character is replaced with a dot we have to update our internal
state, but not fire an Android event with that character change, to keep it
silent.

This fix reverts the change to LayoutText landed by Paul Miller in r468393
(and merged back to M58) because it was suppressing changes to the internal
accessibility state when a character was replaced with a dot. Instead now we
keep the internal state updated but suppress that particular event from being
fired on Android.

BUG=721663,716212
TBR=wangxianzhu
==========

to

==========
Fix TalkBack feedback for password fields in Android O

Prior to O, we were supposed to expose either the value of a password field,
or just dots, depending on a value of a system setting, and independent of
what's shown visually.

In O, we're now supposed to expose what's visually shown, whether that's
characters or dots.

HOWEVER, if a character is replaced with a dot we have to update our internal
state, but not fire an Android event with that character change, to keep it
silent.

This fix reverts the change to LayoutText landed by Paul Miller in r468393
(and merged back to M58) because it was suppressing changes to the internal
accessibility state when a character was replaced with a dot. Instead now we
keep the internal state updated but suppress that particular event from being
fired on Android.

As a minor improvement, this change implements setTextSelection
on AccessibilityNodeInfo, which enables TalkBack to announce when
the cursor reaches the beginning or end of a text field the same
way as it does with native fields.

BUG=721663,716212
TBR=wangxianzhu
==========

[dmazzoni, 2017-05-12 07:43:09]

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

https://codereview.chromium.org/2881723002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:1201:
return
(Settings.System.getInt(mContentViewCore.getContext().getContentResolver(),
On 2017/05/12 07:09:34, aboxhall wrote:
> Suggestion: pull this out into a local var to make this formatting less weird

Done, but git cl format still does it a bit weird.

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

https://codereview.chromium.org/2881723002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/accessibility/KitKatBrowserAccessibilityManager.java:42:
node.setTextSelection(selectionStartIndex, selectionEndIndex);
On 2017/05/12 07:09:34, aboxhall wrote:
> Is this part of the fix as well? Could it be mentioned in the CL description?

Done.

[dmazzoni, 2017-05-12 07:44:57]

Landing change now because this is potentially urgent
for Android, and it'd be great to have a build with this
patch ASAP, but review is still requested, we should be
confident about this change if we want to merge it.

[dmazzoni, 2017-05-12 07:45:13]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-05-12 07:45:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/2881723002/#ps20001
(title: "Address feedback")

[commit-bot: I haz the power, 2017-05-12 07:45:42]

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-05-12 08:43:55]

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

[commit-bot: I haz the power, 2017-05-12 08:43:57]

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

[Xianzhu, 2017-05-12 15:34:49]

core/ lgtm.

[dmazzoni, 2017-05-12 18:01:22]

Description was changed from

==========
Fix TalkBack feedback for password fields in Android O

Prior to O, we were supposed to expose either the value of a password field,
or just dots, depending on a value of a system setting, and independent of
what's shown visually.

In O, we're now supposed to expose what's visually shown, whether that's
characters or dots.

HOWEVER, if a character is replaced with a dot we have to update our internal
state, but not fire an Android event with that character change, to keep it
silent.

This fix reverts the change to LayoutText landed by Paul Miller in r468393
(and merged back to M58) because it was suppressing changes to the internal
accessibility state when a character was replaced with a dot. Instead now we
keep the internal state updated but suppress that particular event from being
fired on Android.

As a minor improvement, this change implements setTextSelection
on AccessibilityNodeInfo, which enables TalkBack to announce when
the cursor reaches the beginning or end of a text field the same
way as it does with native fields.

BUG=721663,716212
TBR=wangxianzhu
==========

to

==========
Fix TalkBack feedback for password fields in Android O

Prior to O, we were supposed to expose either the value of a password field,
or just dots, depending on a value of a system setting, and independent of
what's shown visually.

In O, we're now supposed to expose what's visually shown, whether that's
characters or dots.

HOWEVER, if a character is replaced with a dot we have to update our internal
state, but not fire an Android event with that character change, to keep it
silent.

This fix reverts the change to LayoutText landed by Paul Miller in r468393
(and merged back to M58) because it was suppressing changes to the internal
accessibility state when a character was replaced with a dot. Instead now we
keep the internal state updated but suppress that particular event from being
fired on Android.

As a minor improvement, this change implements setTextSelection
on AccessibilityNodeInfo, which enables TalkBack to announce when
the cursor reaches the beginning or end of a text field the same
way as it does with native fields.

BUG=721663,716212
TBR=wangxianzhu
NOTRY=true
==========

[dmazzoni, 2017-05-12 18:01:34]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2017-05-12 18:03:46]

CQ is trying da patch.

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

[paulmiller, 2017-05-12 18:15:07]

revert lgtm

I don't fully understand what's going on here, though. Your description implies
the text in the layout tree does actually change when the character is visually
hidden. That was not my conclusion when I was debugging this, and my initial fix
depends on the fact that the new and old texts are equal. I assumed there was
some other mechanism which was changing what was displayed, separate from what
text was actually in the layout tree, but I never found what that mechanism was.
Your description also implies my initial fix was getting the AX tree out of
sync, but I thought I'd avoided that by only suppressing the no-op events. Was I
wrong about that?

[commit-bot: I haz the power, 2017-05-12 18:43:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494612094741590,
"parent_rev": "b06d81fdf9fc3a85fe95aa3ef9cd1d7de09758c8", "commit_rev":
"4e36c746c6c51486f9e57e47a946ee35918fd9f0"}

[commit-bot: I haz the power, 2017-05-12 18:43:47]

Description was changed from

==========
Fix TalkBack feedback for password fields in Android O

Prior to O, we were supposed to expose either the value of a password field,
or just dots, depending on a value of a system setting, and independent of
what's shown visually.

In O, we're now supposed to expose what's visually shown, whether that's
characters or dots.

HOWEVER, if a character is replaced with a dot we have to update our internal
state, but not fire an Android event with that character change, to keep it
silent.

This fix reverts the change to LayoutText landed by Paul Miller in r468393
(and merged back to M58) because it was suppressing changes to the internal
accessibility state when a character was replaced with a dot. Instead now we
keep the internal state updated but suppress that particular event from being
fired on Android.

As a minor improvement, this change implements setTextSelection
on AccessibilityNodeInfo, which enables TalkBack to announce when
the cursor reaches the beginning or end of a text field the same
way as it does with native fields.

BUG=721663,716212
TBR=wangxianzhu
NOTRY=true
==========

to

==========
Fix TalkBack feedback for password fields in Android O

Prior to O, we were supposed to expose either the value of a password field,
or just dots, depending on a value of a system setting, and independent of
what's shown visually.

In O, we're now supposed to expose what's visually shown, whether that's
characters or dots.

HOWEVER, if a character is replaced with a dot we have to update our internal
state, but not fire an Android event with that character change, to keep it
silent.

This fix reverts the change to LayoutText landed by Paul Miller in r468393
(and merged back to M58) because it was suppressing changes to the internal
accessibility state when a character was replaced with a dot. Instead now we
keep the internal state updated but suppress that particular event from being
fired on Android.

As a minor improvement, this change implements setTextSelection
on AccessibilityNodeInfo, which enables TalkBack to announce when
the cursor reaches the beginning or end of a text field the same
way as it does with native fields.

BUG=721663,716212
TBR=wangxianzhu
NOTRY=true

Review-Url: https://codereview.chromium.org/2881723002
Cr-Commit-Position: refs/heads/master@{#471383}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e36c746c6c51486f9e57e47a946...
==========

[commit-bot: I haz the power, 2017-05-12 18:43:49]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4e36c746c6c51486f9e57e47a946...

[dmazzoni, 2017-05-12 18:47:16]

On 2017/05/12 18:15:07, paulmiller wrote:
> revert lgtm
> 
> I don't fully understand what's going on here, though. Your description
implies
> the text in the layout tree does actually change when the character is
visually
> hidden. That was not my conclusion when I was debugging this, and my initial
fix
> depends on the fact that the new and old texts are equal. I assumed there was
> some other mechanism which was changing what was displayed, separate from what
> text was actually in the layout tree, but I never found what that mechanism
was.
> Your description also implies my initial fix was getting the AX tree out of
> sync, but I thought I'd avoided that by only suppressing the no-op events. Was
I
> wrong about that?

Great question. What's confusing here is that the accessibility tree contains
information from both the DOM tree (which has the typed password text,
unobscured), and the layout tree (which has what's shown visually). The
accessibility tree ends up looking something like this:

AX role=textfield value="password" - for DOM node <input type=password>
    AX role=div - for root of shadow dom tree
        AX role=staticText name="•••••••d" - for LayoutText object

Previously, we only exposed the full password (the top node in the tree) to
Android, or else we manually obscured it. Your fix worked fine, it got rid of
a spurious notification on the LayoutText object.

Now in O we want to expose the layout tree, so we expose whatever's
rendered. Your change suppressed the notification fired when the layout
text changed from "•••••••d" to ""••••••••", which we now need.

What my patch does, to match TextView, is to expose the contents of
the LayoutText, but only fire a notification when the DOM element changes.