Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a dialog-within-a-dia…

Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a dialog-within-a-dialog

Currently both views::RootView and a dialog's ContentsView() are
populating AX_ROLE_DIALOG. This sets up a weird dialog-within-a-dialog.
Also, since the ContentsView doesn't include the title and dialog
buttons, it creates a nested group that excludes the dialog buttons and
is hard to navigate with a11y tools since it hides the dialog contents
inside the group.

To fix, just rely on views::RootView to populate the window role, and
obey GetAccessibleWindowRole() when deciding whether to notify a11y
tools of the window appearance.

BUG=741285
Review-Url: https://codereview.chromium.org/2980713002
Cr-Commit-Position: refs/heads/master@{#486153}
Committed: https://chromium.googlesource.com/chromium/src/+/df3bc2070765d8e38fd47850ad0c112e128e5933

[tapted, 2017-07-12 05:20:37]

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

[commit-bot: I haz the power, 2017-07-12 05:20:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-07-12 05:29:36]

Description was changed from

==========
Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a
dialog-within-a-dialog

BUG=741285
==========

to

==========
Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a
dialog-within-a-dialog

Currently both views::RootView and a dialog's ContentsView() are
populating AX_ROLE_DIALOG. This sets up a weird dialog-within-a-dialog
that's confusing. Also, since the ContentsView doesn't include the title
and dialog buttons, it creates a group that excludes the dialog buttons
and is hard to navigate with a11y tools.

To fix, just rely on views::RootView to populate the window role, and
obey GetAccessibleWindowRole() when deciding whether to notify a11y
tools of the window appearance.

BUG=741285
==========

[tapted, 2017-07-12 05:29:36]

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

[tapted, 2017-07-12 05:34:26]

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

[commit-bot: I haz the power, 2017-07-12 05:34:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-07-12 05:35:20]

Description was changed from

==========
Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a
dialog-within-a-dialog

Currently both views::RootView and a dialog's ContentsView() are
populating AX_ROLE_DIALOG. This sets up a weird dialog-within-a-dialog
that's confusing. Also, since the ContentsView doesn't include the title
and dialog buttons, it creates a group that excludes the dialog buttons
and is hard to navigate with a11y tools.

To fix, just rely on views::RootView to populate the window role, and
obey GetAccessibleWindowRole() when deciding whether to notify a11y
tools of the window appearance.

BUG=741285
==========

to

==========
Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a
dialog-within-a-dialog

Currently both views::RootView and a dialog's ContentsView() are
populating AX_ROLE_DIALOG. This sets up a weird dialog-within-a-dialog
that's confusing. Also, since the ContentsView doesn't include the title
and dialog buttons, it creates a nested group that excludes the dialog
buttons and is hard to navigate with a11y tools since it hides the
dialog contents inside the group.

To fix, just rely on views::RootView to populate the window role, and
obey GetAccessibleWindowRole() when deciding whether to notify a11y
tools of the window appearance.

BUG=741285
==========

[tapted, 2017-07-12 05:42:12]

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

[commit-bot: I haz the power, 2017-07-12 05:42:13]

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-07-12 06:24:04]

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

[commit-bot: I haz the power, 2017-07-12 06:24:05]

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

[tapted, 2017-07-12 07:06:12]

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

[commit-bot: I haz the power, 2017-07-12 07:06:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-07-12 07:08:29]

Description was changed from

==========
Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a
dialog-within-a-dialog

Currently both views::RootView and a dialog's ContentsView() are
populating AX_ROLE_DIALOG. This sets up a weird dialog-within-a-dialog
that's confusing. Also, since the ContentsView doesn't include the title
and dialog buttons, it creates a nested group that excludes the dialog
buttons and is hard to navigate with a11y tools since it hides the
dialog contents inside the group.

To fix, just rely on views::RootView to populate the window role, and
obey GetAccessibleWindowRole() when deciding whether to notify a11y
tools of the window appearance.

BUG=741285
==========

to

==========
Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a
dialog-within-a-dialog

Currently both views::RootView and a dialog's ContentsView() are
populating AX_ROLE_DIALOG. This sets up a weird dialog-within-a-dialog.
Also, since the ContentsView doesn't include the title and dialog
buttons, it creates a nested group that excludes the dialog buttons and
is hard to navigate with a11y tools since it hides the dialog contents
inside the group.

To fix, just rely on views::RootView to populate the window role, and
obey GetAccessibleWindowRole() when deciding whether to notify a11y
tools of the window appearance.

BUG=741285
==========

[commit-bot: I haz the power, 2017-07-12 08:16:38]

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

[commit-bot: I haz the power, 2017-07-12 08:16:39]

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

[tapted, 2017-07-12 11:09:15]

Hi Mike, please take a look

[msw, 2017-07-12 17:14:34]

lgtm with a q

https://codereview.chromium.org/2980713002/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate.cc (left):

https://codereview.chromium.org/2980713002/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_delegate.cc:261: node_data->SetName(GetWindowTitle());
Where does the window title get pushed into the ax data now?

[tapted, 2017-07-12 23:29:57]

https://codereview.chromium.org/2980713002/diff/60001/ui/views/window/dialog_...
File ui/views/window/dialog_delegate.cc (left):

https://codereview.chromium.org/2980713002/diff/60001/ui/views/window/dialog_...
ui/views/window/dialog_delegate.cc:261: node_data->SetName(GetWindowTitle());
On 2017/07/12 17:14:34, msw wrote:
> Where does the window title get pushed into the ax data now?

This still happens in RootView:

void RootView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
  node_data->SetName(widget_->widget_delegate()->GetAccessibleWindowTitle());
  node_data->role = widget_->widget_delegate()->GetAccessibleWindowRole();
}


where GetAccessibleWindowTitle has a default implementation:

base::string16 WidgetDelegate::GetAccessibleWindowTitle() const {
  return GetWindowTitle();
}


(likely it was always a bug that this line used GetWindowTitle() rather than
GetAccessibleWindowTitle() ..)

[tapted, 2017-07-12 23:31:56]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2017-07-12 23:32:23]

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-07-12 23:37:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499902316100070,
"parent_rev": "b66e999e669170eb39a790a6ede48cef36a13d82", "commit_rev":
"df3bc2070765d8e38fd47850ad0c112e128e5933"}

[commit-bot: I haz the power, 2017-07-12 23:37:52]

Description was changed from

==========
Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a
dialog-within-a-dialog

Currently both views::RootView and a dialog's ContentsView() are
populating AX_ROLE_DIALOG. This sets up a weird dialog-within-a-dialog.
Also, since the ContentsView doesn't include the title and dialog
buttons, it creates a nested group that excludes the dialog buttons and
is hard to navigate with a11y tools since it hides the dialog contents
inside the group.

To fix, just rely on views::RootView to populate the window role, and
obey GetAccessibleWindowRole() when deciding whether to notify a11y
tools of the window appearance.

BUG=741285
==========

to

==========
Views a11y: Obey DialogDelegate::GetAccessibleWindowRole() rather than making a
dialog-within-a-dialog

Currently both views::RootView and a dialog's ContentsView() are
populating AX_ROLE_DIALOG. This sets up a weird dialog-within-a-dialog.
Also, since the ContentsView doesn't include the title and dialog
buttons, it creates a nested group that excludes the dialog buttons and
is hard to navigate with a11y tools since it hides the dialog contents
inside the group.

To fix, just rely on views::RootView to populate the window role, and
obey GetAccessibleWindowRole() when deciding whether to notify a11y
tools of the window appearance.

BUG=741285

Review-Url: https://codereview.chromium.org/2980713002
Cr-Commit-Position: refs/heads/master@{#486153}
Committed:
https://chromium.googlesource.com/chromium/src/+/df3bc2070765d8e38fd47850ad0c...
==========

[commit-bot: I haz the power, 2017-07-12 23:37:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/df3bc2070765d8e38fd47850ad0c...