Add initial tests for keyboard access (tabbing in some dialogs)....

Add initial tests for keyboard access (tabbing in some dialogs).
BUG=none
TEST=none

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

[David Tseng, 2010-07-23 22:35:55]

Ready for a first look.  I intend this to be added to the interactive UI tests
and expand it to cover all dialogs.

[tfarina, 2010-07-25 03:10:39]

Mostly style nits below!

http://codereview.chromium.org/3015026/diff/4002/11001
File chrome/browser/views/browser_keyboard_accessibility.cc (right):

http://codereview.chromium.org/3015026/diff/4002/11001#newcode6
chrome/browser/views/browser_keyboard_accessibility.cc:6: #include
"chrome/browser/automation/ui_controls.h"
please, fix the order of these includes, it should be alphabetical.

http://codereview.chromium.org/3015026/diff/4002/11001#newcode17
chrome/browser/views/browser_keyboard_accessibility.cc:17: class
BrowserKeyboardAccessibility : public InProcessBrowserTest,
the indentation of this class is all wrong.

- class should not be indented.
- public Notification... should be aligned with the above public if possible or
indented 4 spaces.

http://codereview.chromium.org/3015026/diff/4002/11001#newcode20
chrome/browser/views/browser_keyboard_accessibility.cc:20:
BrowserKeyboardAccessibility() : is_waiting_(false),
put this on the following line indented 4 spaces.

http://codereview.chromium.org/3015026/diff/4002/11001#newcode25
chrome/browser/views/browser_keyboard_accessibility.cc:25: }
this } can be on the same line of {

http://codereview.chromium.org/3015026/diff/4002/11001#newcode45
chrome/browser/views/browser_keyboard_accessibility.cc:45: void
TabCycler(gfx::NativeWindow hwnd, bool tabForward);
nit: forward_tab?

http://codereview.chromium.org/3015026/diff/4002/11001#newcode47
chrome/browser/views/browser_keyboard_accessibility.cc:47: views::View*
current_view() { return current_view_; }
can this and the current_event be marked as const?

... current_view() const { ... }

http://codereview.chromium.org/3015026/diff/4002/11001#newcode49
chrome/browser/views/browser_keyboard_accessibility.cc:49: void is_waiting(bool
value) { is_waiting_ = value; }
void set_waiting(bool is_waiting) { ... }

http://codereview.chromium.org/3015026/diff/4002/11001#newcode62
chrome/browser/views/browser_keyboard_accessibility.cc:62:
IN_PROC_BROWSER_TEST_F(BrowserKeyboardAccessibility, TabInAboutChromeDialog) {
do not indent the functions in the namespace.

[dmazzoni, 2010-07-26 16:52:27]

Make sure that "win" and "test" are both in the filename.

When you add it to the gyp file, make sure you exclude Linux/views.

http://codereview.chromium.org/3015026/diff/4002/11003
File views/view.h (right):

http://codereview.chromium.org/3015026/diff/4002/11003#newcode19
views/view.h:19: #include "chrome/common/notification_service.h"
Unfortunately views/ can't include chrome/ - views/ is supposed to work
independently of chrome.

I'm actually looking into using ViewDelegate as a way to get accessibility
notifications from views to chrome. I'll send you a patch that you can add to
this CL.

[David Tseng, 2010-07-26 20:29:54]

Thanks for looking through this.

http://codereview.chromium.org/3015026/diff/4002/11001
File chrome/browser/views/browser_keyboard_accessibility.cc (right):

http://codereview.chromium.org/3015026/diff/4002/11001#newcode6
chrome/browser/views/browser_keyboard_accessibility.cc:6: #include
"chrome/browser/automation/ui_controls.h"
please, fix the order of these includes, it should be alphabetical.
Done.

http://codereview.chromium.org/3015026/diff/4002/11001#newcode17
chrome/browser/views/browser_keyboard_accessibility.cc:17: class
BrowserKeyboardAccessibility : public InProcessBrowserTest,
the indentation of this class is all wrong.

- class should not be indented.
- public Notification... should be aligned with the above public if
possible or indented 4 spaces.
Fixed.

http://codereview.chromium.org/3015026/diff/4002/11001#newcode20
chrome/browser/views/browser_keyboard_accessibility.cc:20:
BrowserKeyboardAccessibility() : is_waiting_(false),
put this on the following line indented 4 spaces.
Fixed.

http://codereview.chromium.org/3015026/diff/4002/11001#newcode25
chrome/browser/views/browser_keyboard_accessibility.cc:25: }
this } can be on the same line of {

http://codereview.chromium.org/3015026/diff/4002/11001#newcode45
chrome/browser/views/browser_keyboard_accessibility.cc:45: void
TabCycler(gfx::NativeWindow hwnd, bool tabForward);
nit: forward_tab?
Fixed.

http://codereview.chromium.org/3015026/diff/4002/11001#newcode47
chrome/browser/views/browser_keyboard_accessibility.cc:47: views::View*
current_view() { return current_view_; }
can this and the current_event be marked as const?
Fixed.

... current_view() const { ... }

http://codereview.chromium.org/3015026/diff/4002/11001#newcode49
chrome/browser/views/browser_keyboard_accessibility.cc:49: void
is_waiting(bool value) { is_waiting_ = value; }
void set_waiting(bool is_waiting) { ... }
Changed.

http://codereview.chromium.org/3015026/diff/4002/11001#newcode62
chrome/browser/views/browser_keyboard_accessibility.cc:62:
IN_PROC_BROWSER_TEST_F(BrowserKeyboardAccessibility,
TabInAboutChromeDialog) {
do not indent the functions in the namespace.
Fixed.

On 7/24/10, tfarina@chromium.org <tfarina@chromium.org> wrote:
> Mostly style nits below!
>
>
> http://codereview.chromium.org/3015026/diff/4002/11001
> File chrome/browser/views/browser_keyboard_accessibility.cc (right):
>
> http://codereview.chromium.org/3015026/diff/4002/11001#newcode6
> chrome/browser/views/browser_keyboard_accessibility.cc:6: #include
> "chrome/browser/automation/ui_controls.h"
> please, fix the order of these includes, it should be alphabetical.
>
> http://codereview.chromium.org/3015026/diff/4002/11001#newcode17
> chrome/browser/views/browser_keyboard_accessibility.cc:17: class
> BrowserKeyboardAccessibility : public InProcessBrowserTest,
> the indentation of this class is all wrong.
>
> - class should not be indented.
> - public Notification... should be aligned with the above public if
> possible or indented 4 spaces.
>
> http://codereview.chromium.org/3015026/diff/4002/11001#newcode20
> chrome/browser/views/browser_keyboard_accessibility.cc:20:
> BrowserKeyboardAccessibility() : is_waiting_(false),
> put this on the following line indented 4 spaces.
>
> http://codereview.chromium.org/3015026/diff/4002/11001#newcode25
> chrome/browser/views/browser_keyboard_accessibility.cc:25: }
> this } can be on the same line of {
>
> http://codereview.chromium.org/3015026/diff/4002/11001#newcode45
> chrome/browser/views/browser_keyboard_accessibility.cc:45: void
> TabCycler(gfx::NativeWindow hwnd, bool tabForward);
> nit: forward_tab?
>
> http://codereview.chromium.org/3015026/diff/4002/11001#newcode47
> chrome/browser/views/browser_keyboard_accessibility.cc:47: views::View*
> current_view() { return current_view_; }
> can this and the current_event be marked as const?
>
> ... current_view() const { ... }
>
> http://codereview.chromium.org/3015026/diff/4002/11001#newcode49
> chrome/browser/views/browser_keyboard_accessibility.cc:49: void
> is_waiting(bool value) { is_waiting_ = value; }
> void set_waiting(bool is_waiting) { ... }
>
> http://codereview.chromium.org/3015026/diff/4002/11001#newcode62
> chrome/browser/views/browser_keyboard_accessibility.cc:62:
> IN_PROC_BROWSER_TEST_F(BrowserKeyboardAccessibility,
> TabInAboutChromeDialog) {
> do not indent the functions in the namespace.
>
> http://codereview.chromium.org/3015026/show
>

[David Tseng, 2010-07-26 20:47:13]

> Make sure that "win" and "test" are both in the filename.

Done.
>
> When you add it to the gyp file, make sure you exclude Linux/views.
How do I do this?
>
>
> http://codereview.chromium.org/3015026/diff/4002/11003
> File views/view.h (right):
>
> http://codereview.chromium.org/3015026/diff/4002/11003#newcode19
> views/view.h:19: #include "chrome/common/notification_service.h"
> Unfortunately views/ can't include chrome/ - views/ is supposed to work
> independently of chrome.
>
> I'm actually looking into using ViewDelegate as a way to get
> accessibility notifications from views to chrome. I'll send you a patch
> that you can add to this CL.
>
Done; thanks!
> http://codereview.chromium.org/3015026/show
>

[dmazzoni, 2010-07-26 20:49:34]

On Mon, Jul 26, 2010 at 1:47 PM, David Tseng <dtseng@chromium.org> wrote:

> > When you add it to the gyp file, make sure you exclude Linux/views.
> How do I do this?
>

There's a section in the .gypi file:

['OS=="linux" and toolkit_views==1', {
-      'sources!': [

The 'sources!' means sources to exclude. Add it there, in the same form as
npapi_interactive_test.cc

- Dominic

[dmazzoni, 2010-07-26 21:12:46]

Looking pretty good, just a couple more suggestions for readability.

http://codereview.chromium.org/3015026/diff/39001/40002
File chrome/browser/views/browser_keyboard_accessibility_test_win.cc (right):

http://codereview.chromium.org/3015026/diff/39001/40002#newcode78
chrome/browser/views/browser_keyboard_accessibility_test_win.cc:78:
TabCycler(current_view()->GetWidget()->GetNativeView(), true);
Make a helper method GetNativeView() so that you don't need to repeat
current_view()->GetWidget()->GetNativeView() all over the code.

http://codereview.chromium.org/3015026/diff/39001/40002#newcode85
chrome/browser/views/browser_keyboard_accessibility_test_win.cc:85:
TabCycler(current_view()->GetWidget()->GetNativeView(), true);
Why are you doing this multiple times? Add a comment to explain why, like for
robustness, and put it in a loop if possible.

[David Tseng, 2010-07-26 22:36:26]

http://codereview.chromium.org/3015026/diff/39001/40002
File chrome/browser/views/browser_keyboard_accessibility_test_win.cc
(right):

http://codereview.chromium.org/3015026/diff/39001/40002#newcode78
chrome/browser/views/browser_keyboard_accessibility_test_win.cc:78:
TabCycler(current_view()->GetWidget()->GetNativeView(), true);
Make a helper method GetNativeView() so that you don't need to repeat
current_view()->GetWidget()->GetNativeView() all over the code.
Done.

http://codereview.chromium.org/3015026/diff/39001/40002#newcode85
chrome/browser/views/browser_keyboard_accessibility_test_win.cc:85:
TabCycler(current_view()->GetWidget()->GetNativeView(), true);
Why are you doing this multiple times? Add a comment to explain why,
like for robustness, and put it in a loop if possible.
Cleaned up; thanks.

On 7/26/10, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
> Looking pretty good, just a couple more suggestions for readability.
>
>
>
> http://codereview.chromium.org/3015026/diff/39001/40002
> File chrome/browser/views/browser_keyboard_accessibility_test_win.cc
> (right):
>
> http://codereview.chromium.org/3015026/diff/39001/40002#newcode78
> chrome/browser/views/browser_keyboard_accessibility_test_win.cc:78:
> TabCycler(current_view()->GetWidget()->GetNativeView(), true);
> Make a helper method GetNativeView() so that you don't need to repeat
> current_view()->GetWidget()->GetNativeView() all over the code.
>
> http://codereview.chromium.org/3015026/diff/39001/40002#newcode85
> chrome/browser/views/browser_keyboard_accessibility_test_win.cc:85:
> TabCycler(current_view()->GetWidget()->GetNativeView(), true);
> Why are you doing this multiple times? Add a comment to explain why,
> like for robustness, and put it in a loop if possible.
>
> http://codereview.chromium.org/3015026/show
>

[David Tseng, 2010-07-27 22:43:37]

I've gone ahead and added the rest of the dialogs from Browser; two of
them, SyncMyBookmarks and TaskManager have focus issues (and
corresponding bugs) noted before each test and the tests themselves
are marked as FAILS_.

On 7/26/10, David Tseng <dtseng@chromium.org> wrote:
> http://codereview.chromium.org/3015026/diff/39001/40002
> File chrome/browser/views/browser_keyboard_accessibility_test_win.cc
> (right):
>
> http://codereview.chromium.org/3015026/diff/39001/40002#newcode78
> chrome/browser/views/browser_keyboard_accessibility_test_win.cc:78:
> TabCycler(current_view()->GetWidget()->GetNativeView(), true);
> Make a helper method GetNativeView() so that you don't need to repeat
> current_view()->GetWidget()->GetNativeView() all over the code.
> Done.
>
> http://codereview.chromium.org/3015026/diff/39001/40002#newcode85
> chrome/browser/views/browser_keyboard_accessibility_test_win.cc:85:
> TabCycler(current_view()->GetWidget()->GetNativeView(), true);
> Why are you doing this multiple times? Add a comment to explain why,
> like for robustness, and put it in a loop if possible.
> Cleaned up; thanks.
>
> On 7/26/10, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
>> Looking pretty good, just a couple more suggestions for readability.
>>
>>
>>
>> http://codereview.chromium.org/3015026/diff/39001/40002
>> File chrome/browser/views/browser_keyboard_accessibility_test_win.cc
>> (right):
>>
>> http://codereview.chromium.org/3015026/diff/39001/40002#newcode78
>> chrome/browser/views/browser_keyboard_accessibility_test_win.cc:78:
>> TabCycler(current_view()->GetWidget()->GetNativeView(), true);
>> Make a helper method GetNativeView() so that you don't need to repeat
>> current_view()->GetWidget()->GetNativeView() all over the code.
>>
>> http://codereview.chromium.org/3015026/diff/39001/40002#newcode85
>> chrome/browser/views/browser_keyboard_accessibility_test_win.cc:85:
>> TabCycler(current_view()->GetWidget()->GetNativeView(), true);
>> Why are you doing this multiple times? Add a comment to explain why,
>> like for robustness, and put it in a loop if possible.
>>
>> http://codereview.chromium.org/3015026/show
>>
>

[dmazzoni, 2010-07-27 23:05:52]

Any chance this test could be easily adapted to work with toolbars as well?

It's fine to do this in a separate CL if this one is otherwise ready to go.

- Dominic

On Tue, Jul 27, 2010 at 3:43 PM, David Tseng <dtseng@chromium.org> wrote:

> I've gone ahead and added the rest of the dialogs from Browser; two of
> them, SyncMyBookmarks and TaskManager have focus issues (and
> corresponding bugs) noted before each test and the tests themselves
> are marked as FAILS_.
>
> On 7/26/10, David Tseng <dtseng@chromium.org> wrote:
> > http://codereview.chromium.org/3015026/diff/39001/40002
> > File chrome/browser/views/browser_keyboard_accessibility_test_win.cc
> > (right):
> >
> > http://codereview.chromium.org/3015026/diff/39001/40002#newcode78
> > chrome/browser/views/browser_keyboard_accessibility_test_win.cc:78:
> > TabCycler(current_view()->GetWidget()->GetNativeView(), true);
> > Make a helper method GetNativeView() so that you don't need to repeat
> > current_view()->GetWidget()->GetNativeView() all over the code.
> > Done.
> >
> > http://codereview.chromium.org/3015026/diff/39001/40002#newcode85
> > chrome/browser/views/browser_keyboard_accessibility_test_win.cc:85:
> > TabCycler(current_view()->GetWidget()->GetNativeView(), true);
> > Why are you doing this multiple times? Add a comment to explain why,
> > like for robustness, and put it in a loop if possible.
> > Cleaned up; thanks.
> >
> > On 7/26/10, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
> >> Looking pretty good, just a couple more suggestions for readability.
> >>
> >>
> >>
> >> http://codereview.chromium.org/3015026/diff/39001/40002
> >> File chrome/browser/views/browser_keyboard_accessibility_test_win.cc
> >> (right):
> >>
> >> http://codereview.chromium.org/3015026/diff/39001/40002#newcode78
> >> chrome/browser/views/browser_keyboard_accessibility_test_win.cc:78:
> >> TabCycler(current_view()->GetWidget()->GetNativeView(), true);
> >> Make a helper method GetNativeView() so that you don't need to repeat
> >> current_view()->GetWidget()->GetNativeView() all over the code.
> >>
> >> http://codereview.chromium.org/3015026/diff/39001/40002#newcode85
> >> chrome/browser/views/browser_keyboard_accessibility_test_win.cc:85:
> >> TabCycler(current_view()->GetWidget()->GetNativeView(), true);
> >> Why are you doing this multiple times? Add a comment to explain why,
> >> like for robustness, and put it in a loop if possible.
> >>
> >> http://codereview.chromium.org/3015026/show
> >>
> >
>

[David Tseng, 2010-07-28 01:04:54]

> Any chance this test could be easily adapted to work with toolbars as well?
>
> It's fine to do this in a separate CL if this one is otherwise ready to go.
>
> - Dominic
I have this working (sort of); please take a look.  The test passes,
but I'm not sure if tabbing through the toolbar raises all of the
focus notifications I expect on the individual menu buttons.  The
tabbing doesn't cycle because I'm getting the same view sending a
focus notification on subsequent tabs.  I'm guessing this is because
we only ever call NotifyAccessibilityEvent (through
FocusManager::SetFocusedView) on the toolbar and not the individual
menu buttons?

David

[David Tseng, 2010-07-28 17:52:14]

Ok; this is ready for another look.  I've added support for not firing
native accessibility notifications based on a flag; we're now doing
this for the LocationBarView and NativeControl's.

Thanks,
David

On 7/27/10, David Tseng <dtseng@chromium.org> wrote:
>> Any chance this test could be easily adapted to work with toolbars as
>> well?
>>
>> It's fine to do this in a separate CL if this one is otherwise ready to
>> go.
>>
>> - Dominic
> I have this working (sort of); please take a look.  The test passes,
> but I'm not sure if tabbing through the toolbar raises all of the
> focus notifications I expect on the individual menu buttons.  The
> tabbing doesn't cycle because I'm getting the same view sending a
> focus notification on subsequent tabs.  I'm guessing this is because
> we only ever call NotifyAccessibilityEvent (through
> FocusManager::SetFocusedView) on the toolbar and not the individual
> menu buttons?
>
> David
>

[dmazzoni, 2010-07-28 19:31:15]

LGTM.

Great test!

http://codereview.chromium.org/3015026/diff/27008/57019
File views/view_win.cc (right):

http://codereview.chromium.org/3015026/diff/27008/57019#newcode39
views/view_win.cc:39: bool is_native) {
How about send_native_event instead of is_native?

[Chris Guillory, 2010-07-28 22:02:20]

LTGM

http://codereview.chromium.org/3015026/diff/27008/57012
File chrome/browser/views/browser_keyboard_accessibility_test_win.cc (right):

http://codereview.chromium.org/3015026/diff/27008/57012#newcode79
chrome/browser/views/browser_keyboard_accessibility_test_win.cc:79:
TabCycler(about_chrome_window->GetNativeWindow(), true);
Nit: This code is duplicated a lot. Perhaps collapse these TabCycler calls into
an overloading TabCycler function with no arguments.