A11y: Don't accounce password keystrokes twice

A11y: Don't accounce password keystrokes twice

When typing passwords on Android, TalkBack first announces the letter
from the keyboard. When the letter is inserted into the password field,
there is an AX_EVENT_TEXT_CHANGED, and TalkBack annaunces the inserted
letter as "dot". Then after a delay, SecureTextTimer replaces the letter
with a dot, causing a second AX_EVENT_TEXT_CHANGED, and TalkBack
announces "dot" again. This suppresses the second AX_EVENT_TEXT_CHANGED.

BUG=716212
Review-Url: https://codereview.chromium.org/2846133002
Cr-Commit-Position: refs/heads/master@{#468393}
Committed: https://chromium.googlesource.com/chromium/src/+/b042a7e565df1572caa21084289f6e37db216684

[paulmiller, 2017-04-27 22:19:43]

Description was changed from

==========
A11y: Don't accounce password keystrokes twice

When typing passwords on Android, TalkBack first announces the letter
from the keyboard. When the letter is inserted into the password field,
there is an AX_EVENT_TEXT_CHANGED, and TalkBack annaunces the inserted
letter as "dot". Then after a delay, SecureTextTimer replaces the letter
with a dot, causing a second AX_EVENT_TEXT_CHANGED, and TalkBack
announces "dot" again. This suppresses the second AX_EVENT_TEXT_CHANGED.

BUG=716212
==========

to

==========
A11y: Don't accounce password keystrokes twice

When typing passwords on Android, TalkBack first announces the letter
from the keyboard. When the letter is inserted into the password field,
there is an AX_EVENT_TEXT_CHANGED, and TalkBack annaunces the inserted
letter as "dot". Then after a delay, SecureTextTimer replaces the letter
with a dot, causing a second AX_EVENT_TEXT_CHANGED, and TalkBack
announces "dot" again. This suppresses the second AX_EVENT_TEXT_CHANGED.

BUG=716212
==========

[paulmiller, 2017-04-27 22:19:43]

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

[paulmiller, 2017-04-27 22:19:59]

On 2017/04/27 22:19:43, paulmiller wrote:
> mailto:paulmiller@chromium.org changed reviewers:
> + mailto:dmazzoni@chromium.org, mailto:wangxianzhu@chromium.org

PTAL

[dmazzoni, 2017-04-27 23:24:45]

This fix is what I had in mind but way too high up in the stack.

I'd make the change in:

BrowserAccessibilityManagerAndroid::NotifyAccessibilityEvent

and then use the fact that BrowserAccessibilityAndroid keeps track of old_value_
and new_value_ to specifically filter out events that don't
change the length, on password fields.

[paulmiller, 2017-04-27 23:29:33]

On 2017/04/27 23:24:45, dmazzoni wrote:
> This fix is what I had in mind but way too high up in the stack.
> 
> I'd make the change in:
> 
> BrowserAccessibilityManagerAndroid::NotifyAccessibilityEvent
> 
> and then use the fact that BrowserAccessibilityAndroid keeps track of
old_value_
> and new_value_ to specifically filter out events that don't
> change the length, on password fields.

Yeah, I tried that. See my comment #30 on the internal bug; there's no event
showing the replacement; they only show the addition of the dot, so the length
does change. What's wrong with putting the fix here? As far as I can tell, this
is the only point where we know that the text change was triggered by hiding the
password. Further down the line, I can't find any way to differentiate between
the 2 text change events.

[dmazzoni, 2017-04-28 00:23:46]

On 2017/04/27 23:29:33, paulmiller wrote:
> On 2017/04/27 23:24:45, dmazzoni wrote:
> > This fix is what I had in mind but way too high up in the stack.
> > 
> > I'd make the change in:
> > 
> > BrowserAccessibilityManagerAndroid::NotifyAccessibilityEvent
> > 
> > and then use the fact that BrowserAccessibilityAndroid keeps track of
> old_value_
> > and new_value_ to specifically filter out events that don't
> > change the length, on password fields.
> 
> Yeah, I tried that. See my comment #30 on the internal bug; there's no event
> showing the replacement; they only show the addition of the dot, so the length
> does change. What's wrong with putting the fix here? As far as I can tell,
this
> is the only point where we know that the text change was triggered by hiding
the
> password. Further down the line, I can't find any way to differentiate between
> the 2 text change events.

What I don't like about this change is that it's actually resulting in our
internal accessibility tree being wrong. The accessibility tree is stateful,
and by suppressing that event from LayoutText, it means we aren't in
sync with the correct value of the field. Even if it achieves the right result
it's not the way I want to go about doing it.

I'm wondering if there's a bug in GetTextChangeAddedCount or some of the
processing that leads up to that.

[paulmiller, 2017-04-29 00:07:15]

On 2017/04/28 00:23:46, dmazzoni wrote:
> On 2017/04/27 23:29:33, paulmiller wrote:
> > On 2017/04/27 23:24:45, dmazzoni wrote:
> > > This fix is what I had in mind but way too high up in the stack.
> > > 
> > > I'd make the change in:
> > > 
> > > BrowserAccessibilityManagerAndroid::NotifyAccessibilityEvent
> > > 
> > > and then use the fact that BrowserAccessibilityAndroid keeps track of
> > old_value_
> > > and new_value_ to specifically filter out events that don't
> > > change the length, on password fields.
> > 
> > Yeah, I tried that. See my comment #30 on the internal bug; there's no event
> > showing the replacement; they only show the addition of the dot, so the
length
> > does change. What's wrong with putting the fix here? As far as I can tell,
> this
> > is the only point where we know that the text change was triggered by hiding
> the
> > password. Further down the line, I can't find any way to differentiate
between
> > the 2 text change events.
> 
> What I don't like about this change is that it's actually resulting in our
> internal accessibility tree being wrong. The accessibility tree is stateful,
> and by suppressing that event from LayoutText, it means we aren't in
> sync with the correct value of the field. Even if it achieves the right result
> it's not the way I want to go about doing it.
> 
> I'm wondering if there's a bug in GetTextChangeAddedCount or some of the
> processing that leads up to that.

How about this? (patch set 2) If you look at SecureTextTimer::Fired(), it's
actually setting the text to itself (so it's not changing the value). I don't
yet understand how that works, but it means we can just suppress AX events in
the cases where the AX tree won't by out of sync.

[dmazzoni, 2017-05-01 15:43:25]

lgtm

OK, that does seem weird that it's setting the text to
itself, but if so then sending an extra accessibility event
does seem unnecessary.

[Xianzhu, 2017-05-01 17:55:58]

rs lgtm.

[paulmiller, 2017-05-01 17:57:24]

The CQ bit was checked by paulmiller@chromium.org

[paulmiller, 2017-05-01 17:57:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2846133002/#ps40001
(title: "add comment")

[commit-bot: I haz the power, 2017-05-01 17:57:53]

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-01 20:01:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493661444165220,
"parent_rev": "2b18a942f54f19aff1b52b7aeb3cf897a422b70b", "commit_rev":
"b042a7e565df1572caa21084289f6e37db216684"}

[commit-bot: I haz the power, 2017-05-01 20:01:51]

Description was changed from

==========
A11y: Don't accounce password keystrokes twice

When typing passwords on Android, TalkBack first announces the letter
from the keyboard. When the letter is inserted into the password field,
there is an AX_EVENT_TEXT_CHANGED, and TalkBack annaunces the inserted
letter as "dot". Then after a delay, SecureTextTimer replaces the letter
with a dot, causing a second AX_EVENT_TEXT_CHANGED, and TalkBack
announces "dot" again. This suppresses the second AX_EVENT_TEXT_CHANGED.

BUG=716212
==========

to

==========
A11y: Don't accounce password keystrokes twice

When typing passwords on Android, TalkBack first announces the letter
from the keyboard. When the letter is inserted into the password field,
there is an AX_EVENT_TEXT_CHANGED, and TalkBack annaunces the inserted
letter as "dot". Then after a delay, SecureTextTimer replaces the letter
with a dot, causing a second AX_EVENT_TEXT_CHANGED, and TalkBack
announces "dot" again. This suppresses the second AX_EVENT_TEXT_CHANGED.

BUG=716212

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

[commit-bot: I haz the power, 2017-05-01 20:01:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b042a7e565df1572caa21084289f...