Make the selection start and end attributes of an html input control...

Make the selection start and end attributes of an html input control
accessible, implement IAccessibleText methods to retrieve this info,
and post a notification when the cursor moves within a text field.

BUG=none
TEST=Added new test to renderer_accessibility_browsertest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=61216

[Chris Guillory, 2010-09-28 17:46:47]

LGTM. Some comments.

http://codereview.chromium.org/3389037/diff/1/2
File chrome/browser/browser_accessibility_win.cc (right):

http://codereview.chromium.org/3389037/diff/1/2#newcode704
chrome/browser/browser_accessibility_win.cc:704: StringToInt(sel_start_text,
&sel_start) &&
NitNit: Perhaps add GetAttributeAsInt, though the return value would be
different from GetAttributeAsBstr...

http://codereview.chromium.org/3389037/diff/1/3
File chrome/browser/browser_accessibility_win.h (right):

http://codereview.chromium.org/3389037/diff/1/3#newcode298
chrome/browser/browser_accessibility_win.h:298: long* offset) {
In the IAccessible methods we use the windows typedef LONG, I think lint doesn't
complain about those.

http://codereview.chromium.org/3389037/diff/1/4
File chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc
(right):

http://codereview.chromium.org/3389037/diff/1/4#newcode163
chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc:163: {
Remove these braces?

http://codereview.chromium.org/3389037/diff/1/4#newcode181
chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc:181: //
TODO(dmazzoni): uncomment when text input boxes send accessibility
Link a bug here?

I believe calling select() generates at least a focus notification. And since
all notifications update all node properties you may be able to test this
currently.

http://codereview.chromium.org/3389037/diff/1/5
File webkit/glue/webaccessibility.cc (right):

http://codereview.chromium.org/3389037/diff/1/5#newcode325
webkit/glue/webaccessibility.cc:325: WebKit::WebNode node = src.node();
Use reference here as well? I assume the reference usage for avoiding copy
constructor.

[dmazzoni, 2010-09-28 19:38:28]

On Tue, Sep 28, 2010 at 10:46 AM, <ctguil@chromium.org> wrote:

> LGTM. Some comments.
>
>
> http://codereview.chromium.org/3389037/diff/1/2
> File chrome/browser/browser_accessibility_win.cc (right):
>
> http://codereview.chromium.org/3389037/diff/1/2#newcode704
> chrome/browser/browser_accessibility_win.cc:704:
> StringToInt(sel_start_text, &sel_start) &&
> NitNit: Perhaps add GetAttributeAsInt, though the return value would be
> different from GetAttributeAsBstr...
>

Good idea, done.


> http://codereview.chromium.org/3389037/diff/1/3
> File chrome/browser/browser_accessibility_win.h (right):
>
> http://codereview.chromium.org/3389037/diff/1/3#newcode298
> chrome/browser/browser_accessibility_win.h:298: long* offset) {
> In the IAccessible methods we use the windows typedef LONG, I think lint
> doesn't complain about those.
>

Fixed.


> http://codereview.chromium.org/3389037/diff/1/4
> File
>
> chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc
> (right):
>
> http://codereview.chromium.org/3389037/diff/1/4#newcode163
>
> chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc:163:
> {
> Remove these braces?
>

Done, this is now split into two tests.


> http://codereview.chromium.org/3389037/diff/1/4#newcode181
>
> chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc:181:
> // TODO(dmazzoni): uncomment when text input boxes send accessibility
> Link a bug here?
>
> I believe calling select() generates at least a focus notification. And
> since all notifications update all node properties you may be able to
> test this currently.
>

We can't until accessibility notifications do something cross-platform. The
next refactoring should handle that.


> http://codereview.chromium.org/3389037/diff/1/5
> File webkit/glue/webaccessibility.cc (right):
>
> http://codereview.chromium.org/3389037/diff/1/5#newcode325
> webkit/glue/webaccessibility.cc:325: WebKit::WebNode node = src.node();
> Use reference here as well? I assume the reference usage for avoiding
> copy constructor.


Would be nice, but unfortunately some of the methods we want to call aren't
const. I added it where possible.

- Dominic

[Paweł Hajdan Jr., 2010-09-29 08:45:16]

Drive-by with test comments.

http://codereview.chromium.org/3389037/diff/1/4
File chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc
(right):

http://codereview.chromium.org/3389037/diff/1/4#newcode47
chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc:47: //
Execute some JavaScript.
Can we move that to ui_test_utils? I'd suggest ExecuteJavascriptInMainFrame.

http://codereview.chromium.org/3389037/diff/1/4#newcode182
chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc:182: //
notifications when the selection changes.
Never check in commented-out code. It's going to rot super-fast.

[dmazzoni, 2010-10-01 17:04:53]

On Wed, Sep 29, 2010 at 1:45 AM, <phajdan.jr@chromium.org> wrote:

> Drive-by with test comments.
> http://codereview.chromium.org/3389037/diff/1/4#newcode47
> chrome/browser/renderer_host/test/renderer_accessibility_browsertest.cc:47:
> // Execute some JavaScript.
> Can we move that to ui_test_utils? I'd suggest
> ExecuteJavascriptInMainFrame.
>

I ended up taking the JavaScript call out of this test, but I'll add this
function to ui_test_utils in another changelist when I need that code again.

- Dominic

[dmazzoni, 2010-10-01 17:06:18]

Ready for another review. The changelist now posts a notification when the
cursor or selection changes. I also had to modify UpdateTree so that it
updates the tree in-place if possible, and now NVDA supports text editing in
Chrome!

[Chris Guillory, 2010-10-01 18:06:43]

LGreatTM!!

http://codereview.chromium.org/3389037/diff/23001/24003
File chrome/browser/browser_accessibility_win.cc (right):

http://codereview.chromium.org/3389037/diff/23001/24003#newcode650
chrome/browser/browser_accessibility_win.cc:650: string16 text_str;
Can we use a reference here.

[dmazzoni, 2010-10-01 19:57:13]

On Fri, Oct 1, 2010 at 11:06 AM, <ctguil@chromium.org> wrote:

> http://codereview.chromium.org/3389037/diff/23001/24003#newcode650
> chrome/browser/browser_accessibility_win.cc:650: string16 text_str;
> Can we use a reference here.
>

Not easily, you can only use a reference if you initialize it in the same
line.

- Dominic