Give up focus if the focused view becomes unfocusable

Give up focus if the focused view becomes unfocusable

A view becomes unfocusable if:
 - it becomes disabled; or
 - it becomes hidden; or
 - it is set to be both not focusable and not accessibility focusable.
In these cases focus should be moved to the next available view, or be
cleared if no view is available for focus.

Also, changed View::IsFocusable() to be non-virtual.

BUG=323956
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288781

[mohsen, 2013-12-06 23:09:03]

The real change of this CL is in files ui/views/view.{h,cc}. The rest is just
dealing with some renamings.

Please take a look...

[mohsen, 2013-12-06 23:09:57]

On 2013/12/06 23:09:03, mohsen wrote:
> The real change of this CL is in files ui/views/view.{h,cc}. The rest is just
> dealing with some renamings.
> 
> Please take a look...

Also, if the change gets approved, I will add some unit tests, too.

[sky, 2013-12-07 00:29:12]

https://codereview.chromium.org/108063004/diff/20001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/108063004/diff/20001/ui/views/view.cc#newcode457
ui/views/view.cc:457: if (!visible) {
You have this code in a bunch of places. I think what you should do is create a
method something like, MoveFocusIfNecessary() that roughly does:

if (IsAccessibilityFocusable() || !Contains(focus_view))
  return;

focus_manager->AdvanceFocus(true);
if (Contains(focus_view))
  focus_manager->ClearFocus();

And write tests for this.

[mohsen, 2013-12-11 20:10:12]

I have updated the code and added tests.

Important files to check are:
- ui/views/view.{h,cc};
- ui/views/focus/focus_manager.{h,cc};
- ui/views/view_unittest.cc.
The rest just deal with renamings.

Please take a look...

https://codereview.chromium.org/108063004/diff/20001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/108063004/diff/20001/ui/views/view.cc#newcode457
ui/views/view.cc:457: if (!visible) {
On 2013/12/07 00:29:13, sky wrote:
> You have this code in a bunch of places. I think what you should do is create
a
> method something like, MoveFocusIfNecessary() that roughly does:
> 
> if (IsAccessibilityFocusable() || !Contains(focus_view))
>   return;
> 
> focus_manager->AdvanceFocus(true);
> if (Contains(focus_view))
>   focus_manager->ClearFocus();
> 
> And write tests for this.

I have extracted the common code and moved it to the FocusManager. This way, it
becomes much simpler and there is no need to see if this view (in some cases) is
equal to the focused view or (in other cases) contains the focused view. 

However, I still clear the focus instead of moving it to another view, as it
seems more natural to me. For example, if the user is typing in a textfield and,
for whatever reason, the textfield gets disabled, I think it is better to clear
the focus instead of moving the focus to another view, because the user may not
notice the change and continue typing and possibly override the other view's
data! But, if you think it is better to move focus to another view, it can be
done easily.

[sky, 2013-12-11 21:29:27]

https://codereview.chromium.org/108063004/diff/40001/ui/views/focus/focus_man...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/108063004/diff/40001/ui/views/focus/focus_man...
ui/views/focus/focus_manager.cc:356: void
FocusManager::ClearFocusIfUnfocusable() {
Why is this clearing and not advancing? Also, I think this implementation has to
be more careful. If the Widget the FocusManager is associated with is not active
I believe this will attempt to activate it.

https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc#newcode...
ui/views/view.cc:1211: focusable_ = focusable;
early out if didn't change.

https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc#newcode...
ui/views/view.cc:1224: accessibility_focusable_ = accessibility_focusable;
early out if didn't change.

[mohsen, 2013-12-12 18:26:53]

https://codereview.chromium.org/108063004/diff/40001/ui/views/focus/focus_man...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/108063004/diff/40001/ui/views/focus/focus_man...
ui/views/focus/focus_manager.cc:356: void
FocusManager::ClearFocusIfUnfocusable() {
On 2013/12/11 21:29:29, sky wrote:
> Why is this clearing and not advancing? Also, I think this implementation has
to
> be more careful. If the Widget the FocusManager is associated with is not
active
> I believe this will attempt to activate it.

1. About clearing vs advancing, as I stated in my previous message:

"I still clear the focus instead of moving it to another view, as it seems more
natural to me. For example, if the user is typing in a textfield and, for
whatever reason, the textfield gets disabled, I think it is better to clear the
focus instead of moving the focus to another view, because the user may not
notice the change and continue typing and possibly override the other view's
data! But, if you think it is better to move focus to another view, it can be
done easily."

That's my only reason to prefer clearing, and I don't have a strong opinion
about it.

2. In the second sentence of your comment, do you mean that if the widget is not
active, clearing the focus might activate it? If this is the case then we will
have the problem anyway. Or I'm completely missing your point here?

https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc#newcode...
ui/views/view.cc:1211: focusable_ = focusable;
On 2013/12/11 21:29:29, sky wrote:
> early out if didn't change.

Done.

https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc#newcode...
ui/views/view.cc:1224: accessibility_focusable_ = accessibility_focusable;
On 2013/12/11 21:29:29, sky wrote:
> early out if didn't change.

Done.

[sky, 2013-12-12 20:21:59]

On Thu, Dec 12, 2013 at 10:26 AM,  <mohsen@chromium.org> wrote:
>
>
https://codereview.chromium.org/108063004/diff/40001/ui/views/focus/focus_man...
> File ui/views/focus/focus_manager.cc (right):
>
>
https://codereview.chromium.org/108063004/diff/40001/ui/views/focus/focus_man...
> ui/views/focus/focus_manager.cc:356: void
> FocusManager::ClearFocusIfUnfocusable() {
> On 2013/12/11 21:29:29, sky wrote:
>>
>> Why is this clearing and not advancing? Also, I think this
>
> implementation has to
>>
>> be more careful. If the Widget the FocusManager is associated with is
>
> not active
>>
>> I believe this will attempt to activate it.
>
>
> 1. About clearing vs advancing, as I stated in my previous message:
>
>
> "I still clear the focus instead of moving it to another view, as it
> seems more natural to me. For example, if the user is typing in a
> textfield and, for whatever reason, the textfield gets disabled, I think
> it is better to clear the focus instead of moving the focus to another
> view, because the user may not notice the change and continue typing and
> possibly override the other view's data! But, if you think it is better
> to move focus to another view, it can be
> done easily."
>
> That's my only reason to prefer clearing, and I don't have a strong
> opinion about it.

Consider if when closing a window the OS didn't focus the next window,
wouldn't that be supremely annoying? So, yes, advance focus.

>
> 2. In the second sentence of your comment, do you mean that if the
> widget is not active, clearing the focus might activate it? If this is
> the case then we will have the problem anyway. Or I'm completely missing
> your point here?

I believe the way you have the code now we will try to activate the
window, even if the window is not active. That should not happen.

  -Scott

>
>
> https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc
> File ui/views/view.cc (right):
>
>
https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc#newcode...
> ui/views/view.cc:1211: focusable_ = focusable;
> On 2013/12/11 21:29:29, sky wrote:
>>
>> early out if didn't change.
>
>
> Done.
>
>
>
https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc#newcode...
> ui/views/view.cc:1224: accessibility_focusable_ =
> accessibility_focusable;
> On 2013/12/11 21:29:29, sky wrote:
>>
>> early out if didn't change.
>
>
> Done.
>
> https://codereview.chromium.org/108063004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mohsen, 2013-12-13 07:08:33]

On 2013/12/12 20:21:59, sky wrote:
> On Thu, Dec 12, 2013 at 10:26 AM,  <mailto:mohsen@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/108063004/diff/40001/ui/views/focus/focus_man...
> > File ui/views/focus/focus_manager.cc (right):
> >
> >
>
https://codereview.chromium.org/108063004/diff/40001/ui/views/focus/focus_man...
> > ui/views/focus/focus_manager.cc:356: void
> > FocusManager::ClearFocusIfUnfocusable() {
> > On 2013/12/11 21:29:29, sky wrote:
> >>
> >> Why is this clearing and not advancing? Also, I think this
> >
> > implementation has to
> >>
> >> be more careful. If the Widget the FocusManager is associated with is
> >
> > not active
> >>
> >> I believe this will attempt to activate it.
> >
> >
> > 1. About clearing vs advancing, as I stated in my previous message:
> >
> >
> > "I still clear the focus instead of moving it to another view, as it
> > seems more natural to me. For example, if the user is typing in a
> > textfield and, for whatever reason, the textfield gets disabled, I think
> > it is better to clear the focus instead of moving the focus to another
> > view, because the user may not notice the change and continue typing and
> > possibly override the other view's data! But, if you think it is better
> > to move focus to another view, it can be
> > done easily."
> >
> > That's my only reason to prefer clearing, and I don't have a strong
> > opinion about it.
> 
> Consider if when closing a window the OS didn't focus the next window,
> wouldn't that be supremely annoying? So, yes, advance focus.

Done.

> 
> >
> > 2. In the second sentence of your comment, do you mean that if the
> > widget is not active, clearing the focus might activate it? If this is
> > the case then we will have the problem anyway. Or I'm completely missing
> > your point here?
> 
> I believe the way you have the code now we will try to activate the
> window, even if the window is not active. That should not happen.

I still can't see how the code might activate the window. So, I have added
ViewTest.DisableViewDoesNotActivateWidget test. Does this test address your
concern?

> 
>   -Scott
> 
> >
> >
> > https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc
> > File ui/views/view.cc (right):
> >
> >
>
https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc#newcode...
> > ui/views/view.cc:1211: focusable_ = focusable;
> > On 2013/12/11 21:29:29, sky wrote:
> >>
> >> early out if didn't change.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/108063004/diff/40001/ui/views/view.cc#newcode...
> > ui/views/view.cc:1224: accessibility_focusable_ =
> > accessibility_focusable;
> > On 2013/12/11 21:29:29, sky wrote:
> >>
> >> early out if didn't change.
> >
> >
> > Done.
> >
> > https://codereview.chromium.org/108063004/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-12-13 17:16:23]

Can you separate this out to the name change (no functionality) first to make
this easier to review?

[sky, 2013-12-13 17:41:56]

I like your test, but it doesn't really cover the case I'm saying. I realize
it's a bit hard to test the case I'm mentioning. The problem with the code as
you have it is that advance focus does a request focus, request focus attempts
to activate the parent widget. This means if the widget is not active, and
FocusManager::focus_view_ becomes invalid then your code does a RequestFocus,
which attempts to activate the widget. I think the only way to see this in a
test is something like:
widget1 with view1 focused
widget2 made active
switch to another app
request focus on view1 (other app still active)
activate widget2
view2 becomes not focusable
this will force widget1 to become active.

To write a test for this you likely need to use ShowAndFocusNativeWindow for the
desktop.

https://codereview.chromium.org/108063004/diff/90001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/108063004/diff/90001/ui/views/view_unittest.c...
ui/views/view_unittest.cc:2943: TEST_F(ViewTest,
DisableViewDoesNotActivateWidget) {
Focus related tests need to be in interactive_ui_tests.

[mohsen, 2013-12-13 19:27:23]

On 2013/12/13 17:41:56, sky wrote:
> I like your test, but it doesn't really cover the case I'm saying. I realize
> it's a bit hard to test the case I'm mentioning. The problem with the code as
> you have it is that advance focus does a request focus, request focus attempts
> to activate the parent widget. This means if the widget is not active, and
> FocusManager::focus_view_ becomes invalid then your code does a RequestFocus,
> which attempts to activate the widget. I think the only way to see this in a
> test is something like:
> widget1 with view1 focused
> widget2 made active
> switch to another app
> request focus on view1 (other app still active)
> activate widget2
> view2 becomes not focusable
> this will force widget1 to become active.
> 
> To write a test for this you likely need to use ShowAndFocusNativeWindow for
the
> desktop.
> 
> https://codereview.chromium.org/108063004/diff/90001/ui/views/view_unittest.cc
> File ui/views/view_unittest.cc (right):
> 
>
https://codereview.chromium.org/108063004/diff/90001/ui/views/view_unittest.c...
> ui/views/view_unittest.cc:2943: TEST_F(ViewTest,
> DisableViewDoesNotActivateWidget) {
> Focus related tests need to be in interactive_ui_tests.

A question, while I'm splitting the CL and converting tests to interactive ui
tests:

You are saying that the problem is with the code "as I have it now", which means
that it can be resolved by writing the code in some other way. But, I think,
however we write it, we will eventually call AdvanceFocus which will have the
problem you described here, as I understand it. Right?

[mohsen, 2013-12-13 20:19:14]

On 2013/12/13 19:27:23, mohsen wrote:
> On 2013/12/13 17:41:56, sky wrote:
> > I like your test, but it doesn't really cover the case I'm saying. I realize
> > it's a bit hard to test the case I'm mentioning. The problem with the code
as
> > you have it is that advance focus does a request focus, request focus
attempts
> > to activate the parent widget. This means if the widget is not active, and
> > FocusManager::focus_view_ becomes invalid then your code does a
RequestFocus,
> > which attempts to activate the widget. I think the only way to see this in a
> > test is something like:
> > widget1 with view1 focused
> > widget2 made active
> > switch to another app
> > request focus on view1 (other app still active)
> > activate widget2
> > view2 becomes not focusable
> > this will force widget1 to become active.
> > 
> > To write a test for this you likely need to use ShowAndFocusNativeWindow for
> the
> > desktop.
> > 
> >
https://codereview.chromium.org/108063004/diff/90001/ui/views/view_unittest.cc
> > File ui/views/view_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/108063004/diff/90001/ui/views/view_unittest.c...
> > ui/views/view_unittest.cc:2943: TEST_F(ViewTest,
> > DisableViewDoesNotActivateWidget) {
> > Focus related tests need to be in interactive_ui_tests.
> 
> A question, while I'm splitting the CL and converting tests to interactive ui
> tests:
> 
> You are saying that the problem is with the code "as I have it now", which
means
> that it can be resolved by writing the code in some other way. But, I think,
> however we write it, we will eventually call AdvanceFocus which will have the
> problem you described here, as I understand it. Right?

Renaming CL is up for review: https://codereview.chromium.org/103493005/

[sky, 2013-12-13 21:04:08]

Two options:
. AdvanceFocus needs a way to advance and store, without actually
attempting to change.
. If not active, just clear out the focused and stored view.

2 is easiest, and since this is an edge case I'm fine with it.

  -Scott

On Fri, Dec 13, 2013 at 11:27 AM,  <mohsen@chromium.org> wrote:
> On 2013/12/13 17:41:56, sky wrote:
>>
>> I like your test, but it doesn't really cover the case I'm saying. I
>> realize
>> it's a bit hard to test the case I'm mentioning. The problem with the code
>> as
>> you have it is that advance focus does a request focus, request focus
>> attempts
>> to activate the parent widget. This means if the widget is not active, and
>> FocusManager::focus_view_ becomes invalid then your code does a
>> RequestFocus,
>> which attempts to activate the widget. I think the only way to see this in
>> a
>> test is something like:
>> widget1 with view1 focused
>> widget2 made active
>> switch to another app
>> request focus on view1 (other app still active)
>> activate widget2
>> view2 becomes not focusable
>> this will force widget1 to become active.
>
>
>> To write a test for this you likely need to use ShowAndFocusNativeWindow
>> for
>
> the
>>
>> desktop.
>
>
>>
>>
https://codereview.chromium.org/108063004/diff/90001/ui/views/view_unittest.cc
>> File ui/views/view_unittest.cc (right):
>
>
>
>
https://codereview.chromium.org/108063004/diff/90001/ui/views/view_unittest.c...
>>
>> ui/views/view_unittest.cc:2943: TEST_F(ViewTest,
>> DisableViewDoesNotActivateWidget) {
>> Focus related tests need to be in interactive_ui_tests.
>
>
> A question, while I'm splitting the CL and converting tests to interactive
> ui
> tests:
>
> You are saying that the problem is with the code "as I have it now", which
> means
> that it can be resolved by writing the code in some other way. But, I think,
> however we write it, we will eventually call AdvanceFocus which will have
> the
> problem you described here, as I understand it. Right?
>
> https://codereview.chromium.org/108063004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mohsen, 2014-08-07 20:33:17]

Resurrecting an old CL!

I have made some modifications. Can you take a look, please...

[sky, 2014-08-07 21:40:35]

As part of this could you make IsFocusable not virtual? I think there is only
one override which should be easy to convert to a setter.

https://codereview.chromium.org/108063004/diff/830001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/108063004/diff/830001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.h:189: void ReviseFocusedView();
I would rather you name this something like AdvanceFocusIfNecessary.

https://codereview.chromium.org/108063004/diff/830001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/108063004/diff/830001/ui/views/view.cc#newcod...
ui/views/view.cc:2379: FocusManager* focus_manager = GetFocusManager();
I would rather avoid the constant tree walking if possible. Can you check
IsAccessibilityFocusable before trying to get the fm too?

https://codereview.chromium.org/108063004/diff/830001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/108063004/diff/830001/ui/views/view_unittest....
ui/views/view_unittest.cc:2654: widget.Activate();
I suspect doing this is going to require the test be in interactive ui tests.
Maybe you could create a test with a fake NativeWidgetDelegate? I believe there
are tests that create a fake NativeWidgetDelegate already.

[mohsen, 2014-08-08 01:32:07]

Please take a look, especially at changes to make IsFocusable() non-virtual,
which I'm not 100% sure if it is correct!

https://codereview.chromium.org/108063004/diff/830001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/108063004/diff/830001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.h:189: void ReviseFocusedView();
On 2014/08/07 21:40:34, sky wrote:
> I would rather you name this something like AdvanceFocusIfNecessary.

Done.

https://codereview.chromium.org/108063004/diff/830001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/108063004/diff/830001/ui/views/view.cc#newcod...
ui/views/view.cc:2379: FocusManager* focus_manager = GetFocusManager();
On 2014/08/07 21:40:34, sky wrote:
> I would rather avoid the constant tree walking if possible. Can you check
> IsAccessibilityFocusable before trying to get the fm too?

Yep, done.

https://codereview.chromium.org/108063004/diff/830001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/108063004/diff/830001/ui/views/view_unittest....
ui/views/view_unittest.cc:2654: widget.Activate();
On 2014/08/07 21:40:34, sky wrote:
> I suspect doing this is going to require the test be in interactive ui tests.
> Maybe you could create a test with a fake NativeWidgetDelegate? I believe
there
> are tests that create a fake NativeWidgetDelegate already.

This test is not meant to test activation. It is only testing if focus is
advanced properly inside a widget. So, it is not really needed to be an
interactive ui test. The only activation requirement for this test is that
widget is always active. For that, I've subclasses a new widget class that
always returns true in its IsActive() function. Please check if it's fine.

[sky, 2014-08-08 14:52:30]

Your changes to WebView look right to me.

https://codereview.chromium.org/108063004/diff/830001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/108063004/diff/830001/ui/views/view_unittest....
ui/views/view_unittest.cc:2654: widget.Activate();
On 2014/08/08 01:32:07, mohsen wrote:
> On 2014/08/07 21:40:34, sky wrote:
> > I suspect doing this is going to require the test be in interactive ui
tests.
> > Maybe you could create a test with a fake NativeWidgetDelegate? I believe
> there
> > are tests that create a fake NativeWidgetDelegate already.
> 
> This test is not meant to test activation. It is only testing if focus is
> advanced properly inside a widget. So, it is not really needed to be an
> interactive ui test. The only activation requirement for this test is that
> widget is always active. For that, I've subclasses a new widget class that
> always returns true in its IsActive() function. Please check if it's fine.

You're new code triggers calling off to NativeWidgetDelegate::IsActive() below.
I'm worried about the effects of this with other tests running at the same time.
I think it best to mock out NativeWidgetDelegate.

https://codereview.chromium.org/108063004/diff/870001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/108063004/diff/870001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:361: if (widget_->IsActive()) {
nit: I would early out if !active. Less indentation that way.

[mohsen, 2014-08-08 17:45:07]

https://codereview.chromium.org/108063004/diff/830001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/108063004/diff/830001/ui/views/view_unittest....
ui/views/view_unittest.cc:2654: widget.Activate();
On 2014/08/08 14:52:30, sky wrote:
> On 2014/08/08 01:32:07, mohsen wrote:
> > On 2014/08/07 21:40:34, sky wrote:
> > > I suspect doing this is going to require the test be in interactive ui
> tests.
> > > Maybe you could create a test with a fake NativeWidgetDelegate? I believe
> > there
> > > are tests that create a fake NativeWidgetDelegate already.
> > 
> > This test is not meant to test activation. It is only testing if focus is
> > advanced properly inside a widget. So, it is not really needed to be an
> > interactive ui test. The only activation requirement for this test is that
> > widget is always active. For that, I've subclasses a new widget class that
> > always returns true in its IsActive() function. Please check if it's fine.
> 
> You're new code triggers calling off to NativeWidgetDelegate::IsActive()
below.
> I'm worried about the effects of this with other tests running at the same
time.
> I think it best to mock out NativeWidgetDelegate.

I guess by NativeWidgetDelegate you mean NativeWidgetPrivate. Right?

If yes, its mock implementation needs a lot of pure virtual function overrides
and yet I'm not sure if that would work as some of them might need proper
implementation (not just an empty implementation). The problem as I understand
is that we need Widget::IsActive() return true for the purpose of this test.
Isn't my above ActiveWidget enough for that matter? (Or a similar subclass of
NativeWidgetAura that overrides IsActive()?)

By the way, I believe this test should work properly even without providing any
mock IsActive() implementation. Normally, in views tests, NativeWidgetAura would
be used as the native widget implementation for widgets (rather than
DesktopNativeWidgetAura, even on desktop aura) and as far as I understand
system-level activation and parallel tests will not interfere with
NativeWidgetAura's notion of activation. Is that right?

https://codereview.chromium.org/108063004/diff/870001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/108063004/diff/870001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:361: if (widget_->IsActive()) {
On 2014/08/08 14:52:30, sky wrote:
> nit: I would early out if !active. Less indentation that way.

Done.

[sky, 2014-08-08 19:44:00]

For some reason I thought we have a test with a NativeWidgetPrivate subclass.
I'm wrong, so, ok, we'll see how this goes. LGTM

[sky, 2014-08-08 19:45:08]

Actually, don't land yet. The problem is you expect IsActive to return true
forever. That won't be the case if the bot runs something else that grabs active
status. Just imagine you're test running in parallel.

[mohsen, 2014-08-08 20:06:50]

On 2014/08/08 19:45:08, sky wrote:
> Actually, don't land yet. The problem is you expect IsActive to return true
> forever. That won't be the case if the bot runs something else that grabs
active
> status. Just imagine you're test running in parallel.

In my current implementation, IsActive() actually returns true forever
(ActiveWidget class overrides IsActive() function to return true).

[sky, 2014-08-08 21:11:24]

Perfect, LGTM then

[mohsen, 2014-08-09 03:53:39]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2014-08-09 03:55:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mohsen@chromium.org/108063004/890001

[commit-bot: I haz the power, 2014-08-09 09:12:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-09 09:48:08]

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

[commit-bot: I haz the power, 2014-08-09 09:48:10]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mohsen, 2014-08-10 01:29:46]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2014-08-10 01:30:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mohsen@chromium.org/108063004/890001

[commit-bot: I haz the power, 2014-08-10 03:20:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-10 04:00:11]

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

[commit-bot: I haz the power, 2014-08-10 04:00:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mohsen, 2014-08-11 06:12:01]

I have added another early return for when the view that might need to lose
focus is not focused at all! This is ideally not necessary, but some browser
tests were failing because hiding a view was trying to give up focus on some
other focused view. Please, take another look...

[sky, 2014-08-11 16:10:51]

https://codereview.chromium.org/108063004/diff/950001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/108063004/diff/950001/ui/views/view.cc#newcod...
ui/views/view.cc:2383: if (IsAccessibilityFocusable() || !HasFocus())
HasFocus has to walk the tree to find the FM, which I was hoping to avoid. So, I
don't think there is a point in the early out.

[mohsen, 2014-08-11 16:30:00]

https://codereview.chromium.org/108063004/diff/950001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/108063004/diff/950001/ui/views/view.cc#newcod...
ui/views/view.cc:2383: if (IsAccessibilityFocusable() || !HasFocus())
On 2014/08/11 16:10:51, sky wrote:
> HasFocus has to walk the tree to find the FM, which I was hoping to avoid. So,
I
> don't think there is a point in the early out.

The goal for this early out is not improving efficiency. The point is that we
should only advance focus if this view that's becoming unfocusable is _the_
focused view. If some other view is focused, we should not try to advance focus.

[sky, 2014-08-11 18:59:21]

Agreed. I thought you had that check in the FM, but it doesn't make sense there.

[sky, 2014-08-11 18:59:31]

LGTM

[mohsen, 2014-08-11 19:07:39]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2014-08-11 19:16:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mohsen@chromium.org/108063004/950001

[commit-bot: I haz the power, 2014-08-11 20:11:40]

Change committed as 288781