Add an accessibility setting to expose password values.

Add an accessibility setting to expose password values.

Android has a feature where if you have headphones plugged in, the screen
reader speaks the password text as you type it. Without headphones, it says
"dot dot dot". To implement this we need to get the password out of Blink.

BUG=399043
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184994

[dmazzoni, 2014-11-06 23:29:11]

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

[Mike West, 2014-11-07 09:51:28]

This is a good feature. We should add it.

However, it conflicts a bit with a goal the password manager team is floating
which would allow us to keep the password itself out of the renderer process
(http://mikewest.github.io/credentialmanagement/writeonly/ is one iteration).

Is there a way we could limit this to exposing password information only when
there's been a user gesture on the field? It's fine to expose _typed_ passwords,
but exposing filled passwords might make our work a little harder in the future.

https://codereview.chromium.org/704363004/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/704363004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:227: return true;
Nit: This seems slightly more readable if you drop the `if` at the top, and
write this as `return isPasswordField();`

https://codereview.chromium.org/704363004/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXRenderObject.cpp (right):

https://codereview.chromium.org/704363004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXRenderObject.cpp:847: String AXRenderObject::text()
const
Is this ever exposed to JavaScript?

https://codereview.chromium.org/704363004/diff/1/Source/core/frame/Settings.in
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/704363004/diff/1/Source/core/frame/Settings.i...
Source/core/frame/Settings.in:303: # If true, the value in password fields is
exposed as if it wasn't protected.
Nit: "exposed to assistive technologies"? "exposed for accessibility support"?

[dmazzoni, 2014-11-07 16:23:24]

On 2014/11/07 09:51:28, Mike West wrote:
> This is a good feature. We should add it.
> 
> However, it conflicts a bit with a goal the password manager team is floating
> which would allow us to keep the password itself out of the renderer process
> (http://mikewest.github.io/credentialmanagement/writeonly/ is one iteration).

Sounds like a neat feature! The spec draft you linked to only talks about hiding
the
password from DOM APIs. Can you say more about keeping it out of the render
process entirely? Is the idea that the render process would know perhaps how
many characters are in the field so that it can render dots/stars, but the
actual
password is stored in the browser only?

> Is there a way we could limit this to exposing password information only when
> there's been a user gesture on the field? It's fine to expose _typed_
passwords,
> but exposing filled passwords might make our work a little harder in the
future.

OK, so how about I make an HTMLInputElement keep track of whether or not
it's been modified as a result of a user gesture, and we clear that flag if it
loses focus?

Then we'd only expose the actual password value via accessibility APIs if 

Alternatively, it seems like if your support for the writeonly attribute will
mean
that the password value is never in the renderer process at all, then this seems
like it should be moot - we'll have to retrieve the password value some other
way anyway when that point happens.

Either way, this is convincing me that I should write a Chromium-side test
to make sure this API works, so I'll know if it breaks in the future.

>
https://codereview.chromium.org/704363004/diff/1/Source/core/accessibility/AX...
> Source/core/accessibility/AXRenderObject.cpp:847: String
AXRenderObject::text()
> const
> Is this ever exposed to JavaScript?

Currently none of the accessibility APIs are exposed to JavaScript at all
(unless you could the
automation extension API currently under development - but that's from a
separate app or
extension that the user would grant host permission to). There are proposals
floating around
to reflect some accessibility information into the DOM starting with the
computed role; so
far there are no plans to expose form field values - I don't see any reason we'd
need to
since those are already readily available from the DOM.

[Mike West, 2014-11-07 16:31:27]

Ok. LGTM. Don't worry about making room for the future change at the moment; as
long as you know how you might go about doing it, I'm happy.

The plan would indeed be to send a similarly-sized nonce down to the renderer
process, and then do an on-the-fly replacement in the network stack. All
theoretical at this point...

[dmazzoni, 2014-11-07 16:49:49]

OK, sounds good. Do you have a bug filed for the writeonly work? We can
just add a blocking bug
to make sure that we make sure "writeonly" is accessible.

On Fri, Nov 7, 2014 at 8:31 AM, <mkwst@chromium.org> wrote:

> Ok. LGTM. Don't worry about making room for the future change at the
> moment; as
> long as you know how you might go about doing it, I'm happy.
>
> The plan would indeed be to send a similarly-sized nonce down to the
> renderer
> process, and then do an on-the-fly replacement in the network stack. All
> theoretical at this point...
>
> https://codereview.chromium.org/704363004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-11-07 22:33:42]

https://codereview.chromium.org/704363004/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/704363004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:227: return true;
On 2014/11/07 09:51:28, Mike West wrote:
> Nit: This seems slightly more readable if you drop the `if` at the top, and
> write this as `return isPasswordField();`

Done.

https://codereview.chromium.org/704363004/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXRenderObject.cpp (right):

https://codereview.chromium.org/704363004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXRenderObject.cpp:847: String AXRenderObject::text()
const
On 2014/11/07 09:51:28, Mike West wrote:
> Is this ever exposed to JavaScript?

Nope.

https://codereview.chromium.org/704363004/diff/1/Source/core/frame/Settings.in
File Source/core/frame/Settings.in (right):

https://codereview.chromium.org/704363004/diff/1/Source/core/frame/Settings.i...
Source/core/frame/Settings.in:303: # If true, the value in password fields is
exposed as if it wasn't protected.
On 2014/11/07 09:51:28, Mike West wrote:
> Nit: "exposed to assistive technologies"? "exposed for accessibility support"?

Done.

[dmazzoni, 2014-11-07 22:33:46]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-11-07 22:34:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/704363004/20001

[commit-bot: I haz the power, 2014-11-07 23:53:57]

Committed patchset #2 (id:20001) as 184994