Small tweaks to improve toolbar keyboard accessibility: Put focus rects...

Small tweaks to improve toolbar keyboard accessibility:  Put focus rects
around more controls (including location bar, only when in full keyboard
access mode).  Fix accessible names.  Fix spacebar to activate menu buttons.
Remove methods for child focus, they're not needed anymore.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50462

[sky, 2010-06-21 20:55:32]

LGTM

http://codereview.chromium.org/2817022/diff/1/5
File chrome/browser/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/2817022/diff/1/5#newcode11
chrome/browser/views/location_bar/location_bar_view.cc:11: #include
"app/l10n_util.h"
after app/drag...

http://codereview.chromium.org/2817022/diff/1/5#newcode567
chrome/browser/views/location_bar/location_bar_view.cc:567:
canvas->DrawFocusRect(r.x() - 1, r.y(), r.width() + 2, r.height());
Make sure this works for rtl.

http://codereview.chromium.org/2817022/diff/1/6
File chrome/browser/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/2817022/diff/1/6#newcode158
chrome/browser/views/location_bar/location_bar_view.h:158: void
set_show_focus_rect(bool show) {
Since you've got a SchedulePaint in here, it's no longer cheap and should be
SetShowFocusRect.

[David Tseng, 2010-06-21 21:54:56]

LGTM.

Does it make sense to pass through up/down arrows to the location bar
in addition to left/right arrows?  Alternatively, should we pass
through everything besides tab/shift tab?

On 6/21/10, sky@chromium.org <sky@chromium.org> wrote:
> LGTM
>
>
> http://codereview.chromium.org/2817022/diff/1/5
> File chrome/browser/views/location_bar/location_bar_view.cc (right):
>
> http://codereview.chromium.org/2817022/diff/1/5#newcode11
> chrome/browser/views/location_bar/location_bar_view.cc:11: #include
> "app/l10n_util.h"
> after app/drag...
>
> http://codereview.chromium.org/2817022/diff/1/5#newcode567
> chrome/browser/views/location_bar/location_bar_view.cc:567:
> canvas->DrawFocusRect(r.x() - 1, r.y(), r.width() + 2, r.height());
> Make sure this works for rtl.
>
> http://codereview.chromium.org/2817022/diff/1/6
> File chrome/browser/views/location_bar/location_bar_view.h (right):
>
> http://codereview.chromium.org/2817022/diff/1/6#newcode158
> chrome/browser/views/location_bar/location_bar_view.h:158: void
> set_show_focus_rect(bool show) {
> Since you've got a SchedulePaint in here, it's no longer cheap and
> should be SetShowFocusRect.
>
> http://codereview.chromium.org/2817022/show
>

[dmazzoni, 2010-06-21 22:52:58]

Done, thanks.

On Mon, Jun 21, 2010 at 1:55 PM, <sky@chromium.org> wrote:

> LGTM
>
>
> http://codereview.chromium.org/2817022/diff/1/5
> File chrome/browser/views/location_bar/location_bar_view.cc (right):
>
> http://codereview.chromium.org/2817022/diff/1/5#newcode11
> chrome/browser/views/location_bar/location_bar_view.cc:11: #include
> "app/l10n_util.h"
> after app/drag...
>
> http://codereview.chromium.org/2817022/diff/1/5#newcode567
> chrome/browser/views/location_bar/location_bar_view.cc:567:
> canvas->DrawFocusRect(r.x() - 1, r.y(), r.width() + 2, r.height());
> Make sure this works for rtl.
>
> http://codereview.chromium.org/2817022/diff/1/6
> File chrome/browser/views/location_bar/location_bar_view.h (right):
>
> http://codereview.chromium.org/2817022/diff/1/6#newcode158
> chrome/browser/views/location_bar/location_bar_view.h:158: void
> set_show_focus_rect(bool show) {
> Since you've got a SchedulePaint in here, it's no longer cheap and
> should be SetShowFocusRect.
>
>
> http://codereview.chromium.org/2817022/show
>

[dmazzoni, 2010-06-21 22:55:09]

Good question - the way it works right now is that accelerators get first
dibs at keystrokes - they can grab keystrokes first and steal them from the
view. This patch makes sure that we don't steal left and right arrows from
the location entry view, even though we've registered those as accelerators.
All other keys already get passed directly through, including the up and
down arrows.

- Dominic

On Mon, Jun 21, 2010 at 2:54 PM, David Tseng <dtseng@chromium.org> wrote:

> LGTM.
>
> Does it make sense to pass through up/down arrows to the location bar
> in addition to left/right arrows?  Alternatively, should we pass
> through everything besides tab/shift tab?
>
> On 6/21/10, sky@chromium.org <sky@chromium.org> wrote:
> > LGTM
> >
> >
> > http://codereview.chromium.org/2817022/diff/1/5
> > File chrome/browser/views/location_bar/location_bar_view.cc (right):
> >
> > http://codereview.chromium.org/2817022/diff/1/5#newcode11
> > chrome/browser/views/location_bar/location_bar_view.cc:11: #include
> > "app/l10n_util.h"
> > after app/drag...
> >
> > http://codereview.chromium.org/2817022/diff/1/5#newcode567
> > chrome/browser/views/location_bar/location_bar_view.cc:567:
> > canvas->DrawFocusRect(r.x() - 1, r.y(), r.width() + 2, r.height());
> > Make sure this works for rtl.
> >
> > http://codereview.chromium.org/2817022/diff/1/6
> > File chrome/browser/views/location_bar/location_bar_view.h (right):
> >
> > http://codereview.chromium.org/2817022/diff/1/6#newcode158
> > chrome/browser/views/location_bar/location_bar_view.h:158: void
> > set_show_focus_rect(bool show) {
> > Since you've got a SchedulePaint in here, it's no longer cheap and
> > should be SetShowFocusRect.
> >
> > http://codereview.chromium.org/2817022/show
> >
>

[dmazzoni at google, 2010-06-21 22:55:35]

Good question - the way it works right now is that accelerators get first
dibs at keystrokes - they can grab keystrokes first and steal them from the
view. This patch makes sure that we don't steal left and right arrows from
the location entry view, even though we've registered those as accelerators.
All other keys already get passed directly through, including the up and
down arrows.

- Dominic

On Mon, Jun 21, 2010 at 2:54 PM, David Tseng <dtseng@chromium.org> wrote:

> LGTM.
>
> Does it make sense to pass through up/down arrows to the location bar
> in addition to left/right arrows?  Alternatively, should we pass
> through everything besides tab/shift tab?
>
> On 6/21/10, sky@chromium.org <sky@chromium.org> wrote:
> > LGTM
> >
> >
> > http://codereview.chromium.org/2817022/diff/1/5
> > File chrome/browser/views/location_bar/location_bar_view.cc (right):
> >
> > http://codereview.chromium.org/2817022/diff/1/5#newcode11
> > chrome/browser/views/location_bar/location_bar_view.cc:11: #include
> > "app/l10n_util.h"
> > after app/drag...
> >
> > http://codereview.chromium.org/2817022/diff/1/5#newcode567
> > chrome/browser/views/location_bar/location_bar_view.cc:567:
> > canvas->DrawFocusRect(r.x() - 1, r.y(), r.width() + 2, r.height());
> > Make sure this works for rtl.
> >
> > http://codereview.chromium.org/2817022/diff/1/6
> > File chrome/browser/views/location_bar/location_bar_view.h (right):
> >
> > http://codereview.chromium.org/2817022/diff/1/6#newcode158
> > chrome/browser/views/location_bar/location_bar_view.h:158: void
> > set_show_focus_rect(bool show) {
> > Since you've got a SchedulePaint in here, it's no longer cheap and
> > should be SetShowFocusRect.
> >
> > http://codereview.chromium.org/2817022/show
> >
>