Fix reversed focus traversal order issue.

Fix reversed focus traversal order issue.

This change fixes several problems in handling focus traversal and resolves TabbedPane's traversal order issue.

Reversed focus traversal is very tricky, and the original traversal code overlooked several things.
(1) A focusable view may have a FocusTraverasble. 
(2) When we are going down, we have to check if the current view has a FocusTraversable.
(3) When we are going up from a FocusTraversable, the parent view may gain the next focus.
This change fixes these issues.

BUG=6871
TEST=See issue 6871.

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

[Yuta Kitamura, 2009-06-12 18:56:05]

Rietveld gives me a "500 Server Error", so let me try to send this again. I
apologize if it duplicates.

--

Hi, I created a patch that fixes TabbedPane's focus order issue. Could
you have a look at this?

Thanks,

http://codereview.chromium.org/125062/diff/1/8
File views/focus/focus_manager.cc (left):

http://codereview.chromium.org/125062/diff/1/8#oldcode586
Line 586: bool dont_loop) {
dont_loop parameter was not used at all. Since looping is handled inside
GetNextFocusableView(), we don't need it.

[jcampan, 2009-06-12 20:24:52]

Thanks for fixing this!

Some nits.

http://codereview.chromium.org/125062/diff/1/5
File views/widget/root_view.cc (right):

http://codereview.chromium.org/125062/diff/1/5#newcode585
Line 585: bool skip_starting_view = !check_starting_view;
It might be clearer to get rid of skip_starting_view and use directly
check_starting_view instead.

http://codereview.chromium.org/125062/diff/1/5#newcode640
Line 640: bool skip_starting_view,
This could be replaced with check_starting_view

http://codereview.chromium.org/125062/diff/1/5#newcode708
Line 708: bool skip_starting_view,
Same here, we could change skip_starting_view to check_starting_view

[Yuta Kitamura, 2009-06-12 23:03:53]

Nits applied. Thank you for your review! I'm happy to fix this.

http://codereview.chromium.org/125062/diff/1/5
File views/widget/root_view.cc (right):

http://codereview.chromium.org/125062/diff/1/5#newcode585
Line 585: bool skip_starting_view = !check_starting_view;
On 2009/06/12 20:24:52, jcampan wrote:
> It might be clearer to get rid of skip_starting_view and use directly
> check_starting_view instead.

Done.

http://codereview.chromium.org/125062/diff/1/5#newcode640
Line 640: bool skip_starting_view,
On 2009/06/12 20:24:52, jcampan wrote:
> This could be replaced with check_starting_view

Done.

http://codereview.chromium.org/125062/diff/1/5#newcode708
Line 708: bool skip_starting_view,
On 2009/06/12 20:24:52, jcampan wrote:
> Same here, we could change skip_starting_view to check_starting_view

Done.

[jcampan, 2009-06-12 23:07:30]

LGTM