Add Android AX functions to set the value and selection of a text field.

Add Android AX functions to set the value and selection of a text field.

BUG=414571
Committed: https://crrev.com/ab9f8645f966c4d01a8009ac6a67a7c7f082e273
Cr-Commit-Position: refs/heads/master@{#302756}

[dmazzoni, 2014-10-24 22:41:46]

dmazzoni@chromium.org changed reviewers:
+ nasko@chromium.org, plundblad@chromium.org

[dmazzoni, 2014-10-24 22:41:47]

nasko: OWNERS review of content/browser/frame_host and content/common

plundblad: everything else

[Peter Lundblad, 2014-10-27 10:21:15]

Hi,

I don't see the newly supported actions being added to the list of supported
actions on the nodes. We should do that for text fields.

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

https://codereview.chromium.org/681503002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:235:
}
It looks like this moves the cursor to the beginning of the text when there
are no arguments.  According to the sdk documentation, that should 'clear
the selection'. Does this behaviour match actual framework behaviour?

https://codereview.chromium.org/681503002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:240:
Log.e("AX", "Scroll forward");
Accidental?

[dmazzoni, 2014-10-27 16:17:02]

On 2014/10/27 10:21:15, Peter Lundblad wrote:
> Hi,
> 
> I don't see the newly supported actions being added to the list of supported
> actions on the nodes. We should do that for text fields.

Done, thanks.

https://codereview.chromium.org/681503002/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:235:
> }
> It looks like this moves the cursor to the beginning of the text when there
> are no arguments.  According to the sdk documentation, that should 'clear
> the selection'. Does this behaviour match actual framework behaviour?

Reading through the Android code, it sets the selection start and end to -1 if
you clear the selection, but if the field is focused you'll get a cursor at the
beginning - so I think this is right.

I also noticed that when calling SetText, the framework sets the selection to
the end of the text so I did that too.

>
https://codereview.chromium.org/681503002/diff/1/content/public/android/java/...
>
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:240:
> Log.e("AX", "Scroll forward");
> Accidental?

Fixed.

[nasko, 2014-10-27 17:18:25]

https://codereview.chromium.org/681503002/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/681503002/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.cc:288: const
BrowserAccessibility& node, const base::string16& value) {
nit: In general this CL doesn't use the proper style, but considering it is
consistent with surrounding code, I'm ok with it. It might be useful to fix it
to comply with it at some point.

https://codereview.chromium.org/681503002/diff/20001/content/renderer/accessi...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/681503002/diff/20001/content/renderer/accessi...
content/renderer/accessibility/renderer_accessibility_complete.cc:386: int
acc_obj_id, base::string16 value) {
Each parameter must be on its own line.

[Peter Lundblad, 2014-10-29 13:14:33]

dmazzoni@chromium.org writes:
> On 2014/10/27 10:21:15, Peter Lundblad wrote:
> > Hi,
> 
> > I don't see the newly supported actions being added to the list of  
> > supported
> > actions on the nodes. We should do that for text fields.
> 
> Done, thanks.
> 
>
https://codereview.chromium.org/681503002/diff/1/content/public/android/java/...
> 
>
content/public/android/java/src/org/chromium/content/browser/accessibility/BrowserAccessibilityManager.java:235:
> > }
> > It looks like this moves the cursor to the beginning of the text when  
> > there
> > are no arguments.  According to the sdk documentation, that should 'clear
> > the selection'. Does this behaviour match actual framework behaviour?
> 
> Reading through the Android code, it sets the selection start and end to -1  
> if
> you clear the selection, but if the field is focused you'll get a cursor at  
> the
> beginning - so I think this is right.

Alright, can you add a comment describing that because it looks odd without
knowing that.

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

[Peter Lundblad, 2014-10-29 13:21:40]

lgtm

[dmazzoni, 2014-11-04 23:22:53]

https://codereview.chromium.org/681503002/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/681503002/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.cc:288: const
BrowserAccessibility& node, const base::string16& value) {
On 2014/10/27 17:18:25, nasko wrote:
> nit: In general this CL doesn't use the proper style, but considering it is
> consistent with surrounding code, I'm ok with it. It might be useful to fix it
> to comply with it at some point.

OK. Fixed for the parts of the code that this change touches. I'll make a
clang-format pass after this lands.

https://codereview.chromium.org/681503002/diff/20001/content/renderer/accessi...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/681503002/diff/20001/content/renderer/accessi...
content/renderer/accessibility/renderer_accessibility_complete.cc:386: int
acc_obj_id, base::string16 value) {
On 2014/10/27 17:18:25, nasko wrote:
> Each parameter must be on its own line.

Done.

[nasko, 2014-11-04 23:28:01]

LGTM

[dmazzoni, 2014-11-04 23:59:32]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-11-05 00:00:21]

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

[commit-bot: I haz the power, 2014-11-05 01:20:05]

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

[commit-bot: I haz the power, 2014-11-05 01:20:06]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2014-11-05 04:49:20]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-11-05 04:49:42]

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

[commit-bot: I haz the power, 2014-11-05 05:16:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-05 05:18:35]

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