Auto-focus the first button in the App Info dialog

Auto-focus the first button in the App Info dialog

To make the dialog more accessible, auto-focus the first button in the
dialog when it opens so focus is already on an element inside the
dialog. This behaviour is more consistent with existing dialogs in
Chrome, and helps users using screen-readers and similar with navigating
the dialog after it opens.

BUG=425311
Committed: https://crrev.com/1870b34b26ad58d36845e8932959f06350e1543c
Cr-Commit-Position: refs/heads/master@{#302415}

[sashab, 2014-10-30 04:31:07]

sashab@chromium.org changed reviewers:
+ benwells@chromium.org

[benwells, 2014-10-30 21:42:13]

https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
(right):

https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:129:
UpdateButtonFocus();
This seems like a strange way to do this. Did you copy this from another dialog?

https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.h
(right):

https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.h:52: void
UpdateButtonFocus();
Nit: please name this function FocusFirstButton or something a bit more helpful.

[benwells, 2014-10-30 21:49:41]

On 2014/10/30 21:42:13, benwells wrote:
>
https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
> File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
> (right):
> 
>
https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
> chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:129:
> UpdateButtonFocus();
> This seems like a strange way to do this. Did you copy this from another
dialog?
> 
>
https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
> File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.h
> (right):
> 
>
https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
> chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.h:52: void
> UpdateButtonFocus();
> Nit: please name this function FocusFirstButton or something a bit more
helpful.

Also maybe the close button should be focused by default. wdyt?

[sashab, 2014-10-31 02:03:04]

https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
(right):

https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:129:
UpdateButtonFocus();
On 2014/10/30 21:42:12, benwells wrote:
> This seems like a strange way to do this. Did you copy this from another
dialog?

Yup; I thought maybe the focus could be re-set every time the dialog was made
visible/hidden. Changed back to the standard way of doing this.

https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.h
(right):

https://codereview.chromium.org/689833004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.h:52: void
UpdateButtonFocus();
On 2014/10/30 21:42:12, benwells wrote:
> Nit: please name this function FocusFirstButton or something a bit more
helpful.

Changed this to just generically focus the first element in the view.

[sashab, 2014-11-02 23:41:36]

Added TODOs; I think this is the best way we're gonna get it to work for now.
Happy to look at a better fix later.

[benwells, 2014-11-03 00:11:10]

Let's just remove the TODOs as I don't really know the best approach.

https://codereview.chromium.org/689833004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/app_list_dialog_container.cc (right):

https://codereview.chromium.org/689833004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog_container.cc:185: views::View*
GetInitiallyFocusedView() override { return GetContentsView(); }
Why did this change in this patch set?

[sashab, 2014-11-03 00:14:46]

Removed TODOs.

https://codereview.chromium.org/689833004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/app_list_dialog_container.cc (right):

https://codereview.chromium.org/689833004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog_container.cc:185: views::View*
GetInitiallyFocusedView() override { return GetContentsView(); }
On 2014/11/03 00:11:10, benwells wrote:
> Why did this change in this patch set?

I just noticed this now, so thought I'd fix it. We definitely don't want to use
DialogDelegate's GetInitiallyFocusedView method, so even though the focus will
be moved later, the most correct thing to focus first (to me) seems to be the
dialog itself (as opposed to NULL).

Alternatively we just remove GetInitiallyFocusedView() from both methods since
technically they're doing nothing (the focus is being changed anyway). But I
think this makes more sense (move focus to the dialog, then to the button).

[benwells, 2014-11-03 02:22:35]

lgtm, but please remove both of those GetInitiallyFocusedView methods if they
have no effect. I think it is best to delete code that has no effect, otherwise
it is just confusing for people to understand why it is there, and it adds bits
to the binary.

[sashab, 2014-11-03 03:23:24]

Ok, I've decided that the right way to fix the GetInitiallyFocusedView()
overrides is to fix DialogDelegate::GetInitiallyFocusedView to not segfault if
the view is not a DialogClientView.

So change this line:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/window/di...
to an early return if |dcv| is null.

Will do this in a follow-up patch; I think we're good with this one for now.

https://codereview.chromium.org/689833004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/app_list_dialog_container.cc (right):

https://codereview.chromium.org/689833004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog_container.cc:97: views::View*
GetInitiallyFocusedView() override { return GetContentsView(); }
This one is *needed* since DialogDelegate::GetInitiallyFocusedView() tries to
cast the client view to a DialogClientView, which its not, so it segfaults. Made
it return NULL in the next patch (technically more correct) since this is
ignored anyway.

https://codereview.chromium.org/689833004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog_container.cc:176: // Overridden
from views::WidgetDelegate:
GetInitiallyFocusedView() is not needed here since the client view *is* a
DialogClientView, so it safely returns NULL.

[benwells, 2014-11-03 04:29:46]

lgtm. yeah fixing that assumption in dialog_delegate.cc sounds wise.

[sashab, 2014-11-03 05:35:46]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-11-03 05:36:54]

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

[commit-bot: I haz the power, 2014-11-03 06:17:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-03 06:18:41]

Patchset 6 (id:??) landed as
https://crrev.com/1870b34b26ad58d36845e8932959f06350e1543c
Cr-Commit-Position: refs/heads/master@{#302415}