Move implementation of accessibility actions to views::View

Move implementation of accessibility actions to views::View

We had two parallel, nearly identical implementations of accessible
actions for Views, one in NativeViewAccessibilityBase (used on Windows
and macOS), and one in AXViewObjWrapper (used on Chrome OS). Remove
the duplication and push all of those handlers to View.

While there were some tiny logic differences between the two that are
now merged, in effect this should be a pure refactoring change with no
user-visible impact.

BUG=699617
Review-Url: https://codereview.chromium.org/2795843002
Cr-Commit-Position: refs/heads/master@{#461624}
Committed: https://chromium.googlesource.com/chromium/src/+/59016e60a48d96c7b7d9bf71cf61307ea1df0903

[dmazzoni, 2017-04-03 20:28:30]

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

[commit-bot: I haz the power, 2017-04-03 20:29:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-04-03 20:29:32]

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

[dmazzoni, 2017-04-03 20:29:33]

@tapted: please review this, should make things cleaner.

dougt, patricialor: FYI

[commit-bot: I haz the power, 2017-04-03 20:42:23]

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

[commit-bot: I haz the power, 2017-04-03 20:42:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2017-04-03 20:55:04]

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

[commit-bot: I haz the power, 2017-04-03 20:55:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-04-03 21:07:50]

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

[commit-bot: I haz the power, 2017-04-03 21:07:58]

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-04-03 21:42:08]

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

[commit-bot: I haz the power, 2017-04-03 21:42:09]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tapted, 2017-04-03 23:28:23]

looks neat - just a few minor things

https://codereview.chromium.org/2795843002/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (left):

https://codereview.chromium.org/2795843002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:90: virtual void
DoDefaultAction() = 0;
test_ax_node_wrapper.h still has one of these by the look of the trybots

https://codereview.chromium.org/2795843002/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (right):

https://codereview.chromium.org/2795843002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:84: // Calls one of the
methods below to perform an accessibility action,
nit: comment out of date (no more "methods below" :)

https://codereview.chromium.org/2795843002/diff/40001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_unittest.cc (right):

https://codereview.chromium.org/2795843002/diff/40001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_unittest.cc:137:
EXPECT_FALSE(SetFocused(button_accessibility(), true));
looks like this is failing - `case ui::AX_ACTION_FOCUS` always returns `true` in
view.cc - I'm not sure whether returning false under some condition (or just
`return HasFocus();`) is the right thing there. (alternatively perhaps the
SetFocused() helper could return something like `view_accessibility->GetFocus()
== view_accessibility->GetNativeObject()`)

https://codereview.chromium.org/2795843002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2795843002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1410: gfx::Point center = rect.CenterPoint();
nit: const gfx::Point center = GetLocalBounds().CenterPoint(); ?

https://codereview.chromium.org/2795843002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1427: ShowContextMenu(bounds().CenterPoint(),
ui::MENU_SOURCE_KEYBOARD);
I think ShowContextMenu expects screen coordinates (see
View::ProcessMousePressed)

Something like `bounds().CenterPoint() -> GetBoundsInScreen().CenterPoint()`
looks simplest

[dmazzoni, 2017-04-03 23:46:21]

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

[commit-bot: I haz the power, 2017-04-03 23:46:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-04-03 23:54:33]

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

[commit-bot: I haz the power, 2017-04-03 23:54:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-04-03 23:55:16]

https://codereview.chromium.org/2795843002/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (left):

https://codereview.chromium.org/2795843002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:90: virtual void
DoDefaultAction() = 0;
On 2017/04/03 23:28:23, tapted wrote:
> test_ax_node_wrapper.h still has one of these by the look of the trybots

Done.

https://codereview.chromium.org/2795843002/diff/40001/ui/accessibility/platfo...
File ui/accessibility/platform/ax_platform_node_delegate.h (right):

https://codereview.chromium.org/2795843002/diff/40001/ui/accessibility/platfo...
ui/accessibility/platform/ax_platform_node_delegate.h:84: // Calls one of the
methods below to perform an accessibility action,
On 2017/04/03 23:28:22, tapted wrote:
> nit: comment out of date (no more "methods below" :)

Done.

https://codereview.chromium.org/2795843002/diff/40001/ui/views/accessibility/...
File ui/views/accessibility/native_view_accessibility_unittest.cc (right):

https://codereview.chromium.org/2795843002/diff/40001/ui/views/accessibility/...
ui/views/accessibility/native_view_accessibility_unittest.cc:137:
EXPECT_FALSE(SetFocused(button_accessibility(), true));
On 2017/04/03 23:28:23, tapted wrote:
> looks like this is failing - `case ui::AX_ACTION_FOCUS` always returns `true`
in
> view.cc - I'm not sure whether returning false under some condition (or just
> `return HasFocus();`) is the right thing there. (alternatively perhaps the
> SetFocused() helper could return something like
`view_accessibility->GetFocus()
> == view_accessibility->GetNativeObject()`)

I wasn't quite reproducing the previous behavior, which was to return
false if it's not focusable. Done.

https://codereview.chromium.org/2795843002/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2795843002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1410: gfx::Point center = rect.CenterPoint();
On 2017/04/03 23:28:23, tapted wrote:
> nit: const gfx::Point center = GetLocalBounds().CenterPoint(); ?

Done.

https://codereview.chromium.org/2795843002/diff/40001/ui/views/view.cc#newcod...
ui/views/view.cc:1427: ShowContextMenu(bounds().CenterPoint(),
ui::MENU_SOURCE_KEYBOARD);
On 2017/04/03 23:28:23, tapted wrote:
> I think ShowContextMenu expects screen coordinates (see
> View::ProcessMousePressed)
> 
> Something like `bounds().CenterPoint() -> GetBoundsInScreen().CenterPoint()`
> looks simplest

Good catch! That was a bug from before, I guess nobody noticed.

[tapted, 2017-04-04 00:17:10]

lgtm

[commit-bot: I haz the power, 2017-04-04 00:31:12]

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

[commit-bot: I haz the power, 2017-04-04 00:31:13]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dmazzoni, 2017-04-04 02:56:55]

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

[commit-bot: I haz the power, 2017-04-04 02:57:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-04-04 03:30:41]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2017-04-04 03:32:16]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-04-04 03:32:17]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2795843002/#ps100001
(title: "Fix win compile")

[commit-bot: I haz the power, 2017-04-04 03:32:46]

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-04-04 04:00:42]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491276736722010,
"parent_rev": "75705ebbf91265a8db5218d1acff3f1026777e9e", "commit_rev":
"59016e60a48d96c7b7d9bf71cf61307ea1df0903"}

[commit-bot: I haz the power, 2017-04-04 04:01:49]

Description was changed from

==========
Move implementation of accessibility actions to views::View

We had two parallel, nearly identical implementations of accessible
actions for Views, one in NativeViewAccessibilityBase (used on Windows
and macOS), and one in AXViewObjWrapper (used on Chrome OS). Remove
the duplication and push all of those handlers to View.

While there were some tiny logic differences between the two that are
now merged, in effect this should be a pure refactoring change with no
user-visible impact.

BUG=699617
==========

to

==========
Move implementation of accessibility actions to views::View

We had two parallel, nearly identical implementations of accessible
actions for Views, one in NativeViewAccessibilityBase (used on Windows
and macOS), and one in AXViewObjWrapper (used on Chrome OS). Remove
the duplication and push all of those handlers to View.

While there were some tiny logic differences between the two that are
now merged, in effect this should be a pure refactoring change with no
user-visible impact.

BUG=699617

Review-Url: https://codereview.chromium.org/2795843002
Cr-Commit-Position: refs/heads/master@{#461624}
Committed:
https://chromium.googlesource.com/chromium/src/+/59016e60a48d96c7b7d9bf71cf61...
==========

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

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/59016e60a48d96c7b7d9bf71cf61...