Initial support for accessibility actions on ARC.

Initial support for accessibility actions on ARC.

BUG=683396
TEST=with ChromeVox on, Search+space while within ARC; verify click propagates to ARC and the node gets clicked.

Review-Url: https://codereview.chromium.org/2700103004
Cr-Commit-Position: refs/heads/master@{#453028}
Committed: https://chromium.googlesource.com/chromium/src/+/32ea1736ed52553710e6002a63c9ab0ea75261aa

[David Tseng, 2017-02-17 19:02:07]

Description was changed from

==========
Initial support for accessibility actions on ARC.

BUG=
==========

to

==========
Initial support for accessibility actions on ARC.

BUG=683396
==========

[David Tseng, 2017-02-17 19:02:07]

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

[David Tseng, 2017-02-17 19:03:12]

Ready for an initial look. Actions didn't quite fit the RFH abstractions, so
left that alone for now.

[David Tseng, 2017-02-17 19:17:11]

Description was changed from

==========
Initial support for accessibility actions on ARC.

BUG=683396
==========

to

==========
Initial support for accessibility actions on ARC.

BUG=683396
TEST=with ChromeVox on, Search+space while within ARC; verify click propagates
to ARC and the node gets clicked.
==========

[David Tseng, 2017-02-19 14:41:10]

Patchset #3 (id:40001) has been deleted

[David Tseng, 2017-02-19 14:42:27]

Patchset #2 (id:20001) has been deleted

[dmazzoni, 2017-02-20 04:50:48]

A couple of suggestions for readability, but the idea of
AXHostDelegate sounds good

https://codereview.chromium.org/2700103004/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc
(right):

https://codereview.chromium.org/2700103004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:164:
arc::mojom::AccessibilityActionType::FOCUS;
Is there a "none"?

Might be clearer to initialize it to "none" and set it to "focus" in the
default case of the switch with a comment saying that we're temporarily
just mapping all others to focus for now.

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_hos...
File ui/accessibility/ax_host_delegate.h (right):

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_hos...
ui/accessibility/ax_host_delegate.h:12: // Classes that host an accessibility
tree in the browser that also wish to
"in the browser" could be interpreted to mean that this is something related to
web
browser windows. Perhaps "somewhere on the desktop" would be more clear.

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_id_registry.cc (right):

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_id_registry.cc:63: id_to_host_delegate_[0] = delegate;
Was the user supposed to already call GetOrCreateAXTreeID with that delegate?
If so then that delegate would end up in the map twice - so it sounds like not.

Maybe the docs for SetDesktopHostDelegate should be more clear to say that
you should call it *instead* of GetOrCreateAXTreeID. Alternatively maybe
GetHostDelegate could take a second argument is_desktop.

Finally, it might be good to DCHECK that it wasn't previously set, as that'd be
an error for this to be called twice

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_id_registry.h (right):

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_id_registry.h:26: //     - from a backing object
from a backing AXHostDelegate object

[David Tseng, 2017-02-21 19:09:05]

https://codereview.chromium.org/2700103004/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc
(right):

https://codereview.chromium.org/2700103004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc:164:
arc::mojom::AccessibilityActionType::FOCUS;
On 2017/02/20 04:50:46, dmazzoni wrote:
> Is there a "none"?
> 
> Might be clearer to initialize it to "none" and set it to "focus" in the
> default case of the switch with a comment saying that we're temporarily
> just mapping all others to focus for now.

Nope, no none. The intention wasn't to map everything to focus, but give the
local an initial value. Doing nothing (returning) for the default case is right.

And, it looks like it's fine (compliation wise) to leave it uninitialized, so
went ahead and did that.

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_hos...
File ui/accessibility/ax_host_delegate.h (right):

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_hos...
ui/accessibility/ax_host_delegate.h:12: // Classes that host an accessibility
tree in the browser that also wish to
On 2017/02/20 04:50:46, dmazzoni wrote:
> "in the browser" could be interpreted to mean that this is something related
to
> web
> browser windows. Perhaps "somewhere on the desktop" would be more clear.

That's not quite what it meant to me. Browser process (added).

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_id_registry.cc (right):

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_id_registry.cc:63: id_to_host_delegate_[0] = delegate;
On 2017/02/20 04:50:46, dmazzoni wrote:
> Was the user supposed to already call GetOrCreateAXTreeID with that delegate?
> If so then that delegate would end up in the map twice - so it sounds like
not.
> 
> Maybe the docs for SetDesktopHostDelegate should be more clear to say that
> you should call it *instead* of GetOrCreateAXTreeID. Alternatively maybe
> GetHostDelegate could take a second argument is_desktop.
> 
> Finally, it might be good to DCHECK that it wasn't previously set, as that'd
be
> an error for this to be called twice

Got rid of desktop specifics and added a new constructor for AXHostDelegate
taking an existing or known id. See the docs for that for more info/if it's
clear.

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_tre...
File ui/accessibility/ax_tree_id_registry.h (right):

https://codereview.chromium.org/2700103004/diff/60001/ui/accessibility/ax_tre...
ui/accessibility/ax_tree_id_registry.h:26: //     - from a backing object
On 2017/02/20 04:50:46, dmazzoni wrote:
> from a backing AXHostDelegate object

Done.

[David Tseng, 2017-02-21 19:35:35]

Patchset #4 (id:100001) has been deleted

[David Tseng, 2017-02-21 19:35:46]

Patchset #3 (id:80001) has been deleted

[dmazzoni, 2017-02-22 05:17:38]

Thanks. All of that looks great. Is this ready or still
in progress?

[David Tseng, 2017-02-22 05:53:14]

It's ready but mind the base change.
On Tue, Feb 21, 2017 at 9:17 PM <dmazzoni@chromium.org> wrote:

> Thanks. All of that looks great. Is this ready or still
> in progress?
>
>
> https://codereview.chromium.org/2700103004/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2017-02-22 05:54:41]

dtseng@chromium.org changed reviewers:
+ yawano@chromium.org

[David Tseng, 2017-02-22 05:54:42]

+ yawano.

[dmazzoni, 2017-02-22 07:44:28]

lgtm

https://codereview.chromium.org/2700103004/diff/120001/chrome/browser/ui/aura...
File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right):

https://codereview.chromium.org/2700103004/diff/120001/chrome/browser/ui/aura...
chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:150: :
AXHostDelegate(0), enabled_(false), processing_events_(false) {}
We really ought to have a constant somewhere in the code for this.
Rather than refactoring it in this change could you at least just add a
comment inside the initializer, something like this?

: AXHostDelegate(0 /* desktop tree id */), enabled_(false), ...

[yawano, 2017-02-22 08:39:05]

c/b/chromeos/arc/accessibility lgtm.

https://codereview.chromium.org/2700103004/diff/120001/chrome/browser/ui/aura...
File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right):

https://codereview.chromium.org/2700103004/diff/120001/chrome/browser/ui/aura...
chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:150: :
AXHostDelegate(0), enabled_(false), processing_events_(false) {}
api::automation::kDesktopTreeId? -
https://cs.chromium.org/chromium/src/chrome/common/extensions/api/automation_...

[David Tseng, 2017-02-22 16:12:41]

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

[commit-bot: I haz the power, 2017-02-22 16:13:21]

Dry run: 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-02-22 17:11:40]

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

[commit-bot: I haz the power, 2017-02-22 17:11:41]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-02-23 18:44:26]

https://codereview.chromium.org/2700103004/diff/120001/chrome/browser/ui/aura...
File chrome/browser/ui/aura/accessibility/automation_manager_aura.cc (right):

https://codereview.chromium.org/2700103004/diff/120001/chrome/browser/ui/aura...
chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:150: :
AXHostDelegate(0), enabled_(false), processing_events_(false) {}
On 2017/02/22 08:39:05, yawano wrote:
> api::automation::kDesktopTreeId? -
>
https://cs.chromium.org/chromium/src/chrome/common/extensions/api/automation_...

Done.

https://codereview.chromium.org/2700103004/diff/120001/chrome/browser/ui/aura...
chrome/browser/ui/aura/accessibility/automation_manager_aura.cc:150: :
AXHostDelegate(0), enabled_(false), processing_events_(false) {}
On 2017/02/22 07:44:28, dmazzoni wrote:
> We really ought to have a constant somewhere in the code for this.
> Rather than refactoring it in this change could you at least just add a
> comment inside the initializer, something like this?
> 
> : AXHostDelegate(0 /* desktop tree id */), enabled_(false), ...
> 

Yeah, there's a constant (already being used) just forgot to use it :).

I actually meant to get rid of the id and make RFHI inherit fro m AXHost but
nexdt change.

[David Tseng, 2017-02-23 19:04:06]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-02-23 19:04:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, yawano@chromium.org
Link to the patchset: https://codereview.chromium.org/2700103004/#ps140001
(title: "Use desktop tree id.")

[commit-bot: I haz the power, 2017-02-23 19:05:16]

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-02-23 19:15:10]

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

[commit-bot: I haz the power, 2017-02-23 19:15:11]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[David Tseng, 2017-02-24 22:03:24]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-02-24 22:03:41]

The CQ bit was unchecked by dtseng@chromium.org

[David Tseng, 2017-02-24 22:06:58]

dtseng@chromium.org changed reviewers:
+ rockot@chromium.org

[David Tseng, 2017-02-24 22:06:59]

+ rockot for c/b/e/BUILD.gn

[Ken Rockot(use gerrit already), 2017-02-24 22:07:30]

rs lgtm

[David Tseng, 2017-02-24 23:00:18]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2017-02-24 23:00:52]

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-02-25 00:52:31]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487977218036100,
"parent_rev": "5dcf3589e69f5511ded5a0180da2ddb00db23234", "commit_rev":
"32ea1736ed52553710e6002a63c9ab0ea75261aa"}

[commit-bot: I haz the power, 2017-02-25 00:53:11]

Description was changed from

==========
Initial support for accessibility actions on ARC.

BUG=683396
TEST=with ChromeVox on, Search+space while within ARC; verify click propagates
to ARC and the node gets clicked.
==========

to

==========
Initial support for accessibility actions on ARC.

BUG=683396
TEST=with ChromeVox on, Search+space while within ARC; verify click propagates
to ARC and the node gets clicked.

Review-Url: https://codereview.chromium.org/2700103004
Cr-Commit-Position: refs/heads/master@{#453028}
Committed:
https://chromium.googlesource.com/chromium/src/+/32ea1736ed52553710e6002a63c9...
==========

[commit-bot: I haz the power, 2017-02-25 00:53:13]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/32ea1736ed52553710e6002a63c9...