NativeWidgetAura::ClearNativeFocus fix

Set aura window focus in NativeWidgetAura::ClearNativeFocus only if the focus is owned by child window.

 This was causing a focus to be stolen when non active window calls ClearNativeFocus.

BUG=114477
TEST=OmniboxViewTest.PopupAccelerators passes with this change


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

[Ben Goodger (Google), 2011-12-14 18:17:36]

Why not just check if active first?

Who calls ClearNativeFocus()?

-Ben

On Wed, Dec 14, 2011 at 9:54 AM, <oshima@chromium.org> wrote:

> Reviewers: Ben Goodger (Google),
>
> Description:
> Set aura window focus in NativeWidgetAura::**ClearNativeFocus only if the
> focus is
> owned by child window.
>
>  This was causing a focus to be stolen when non active window calls
> ClearNativeFocus.
>
> BUG=none
> TEST=OmniboxViewTest.**PopupAccelerators passes with this change
>
>
> Please review this at
http://codereview.chromium.**org/8931022/<http://codereview.chromium.org/8931...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M ui/views/widget/native_widget_**aura.cc
>
>
> Index: ui/views/widget/native_widget_**aura.cc
> diff --git a/ui/views/widget/native_**widget_aura.cc
> b/ui/views/widget/native_**widget_aura.cc
> index 931eca58a7996542bf207adca9badd**c4161af030..**
> e380806d83ea4c1b1087ffb7b2ddca**ab9db5e4b4 100644
> --- a/ui/views/widget/native_**widget_aura.cc
> +++ b/ui/views/widget/native_**widget_aura.cc
> @@ -528,8 +528,7 @@ void NativeWidgetAura::SetCursor(**gfx::NativeCursor
> cursor) {
>  }
>
>  void NativeWidgetAura::**ClearNativeFocus() {
> -  if (window_ && window_->GetFocusManager())
> -    window_->GetFocusManager()->**SetFocusedWindow(window_);
> +  // There is no native controls that can have a focus.
>  }
>
>  void NativeWidgetAura::**FocusNativeView(gfx::**NativeView native_view) {
>
>
>

[oshima, 2011-12-14 21:07:35]

On 2011/12/14 18:17:36, Ben Goodger (Google) wrote:
> Why not just check if active first?

a toplevel with transient parent may not be active but can have focus
(like constrained window, or modal window)

> 
> Who calls ClearNativeFocus()?

This is called from FocusManager::ClearFocus, which is called in several places,
but one that was
causing the issue is FocusManager::StoreFocusedView, which is called when a
window lost activation.

It is my understanding that ClearNativeFocus is to *reset* the native focus
within the window,
but not to move the focus owned by other toplevel window. At least, that's how
it used to work
on gtk. Maybe this should be renamed to ResetNativeFocus?

Let me know what you think and/or have better solution. I'm happy to update the
change.

- oshima

> -Ben
> 
> On Wed, Dec 14, 2011 at 9:54 AM, <mailto:oshima@chromium.org> wrote:
> 
> > Reviewers: Ben Goodger (Google),
> >
> > Description:
> > Set aura window focus in NativeWidgetAura::**ClearNativeFocus only if the
> > focus is
> > owned by child window.
> >
> >  This was causing a focus to be stolen when non active window calls
> > ClearNativeFocus.
> >
> > BUG=none
> > TEST=OmniboxViewTest.**PopupAccelerators passes with this change
> >
> >
> > Please review this at
>
http://codereview.chromium.**org/8931022/%3Chttp://codereview.chromium.org/89...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> >
> > Affected files:
> >  M ui/views/widget/native_widget_**aura.cc
> >
> >
> > Index: ui/views/widget/native_widget_**aura.cc
> > diff --git a/ui/views/widget/native_**widget_aura.cc
> > b/ui/views/widget/native_**widget_aura.cc
> > index 931eca58a7996542bf207adca9badd**c4161af030..**
> > e380806d83ea4c1b1087ffb7b2ddca**ab9db5e4b4 100644
> > --- a/ui/views/widget/native_**widget_aura.cc
> > +++ b/ui/views/widget/native_**widget_aura.cc
> > @@ -528,8 +528,7 @@ void NativeWidgetAura::SetCursor(**gfx::NativeCursor
> > cursor) {
> >  }
> >
> >  void NativeWidgetAura::**ClearNativeFocus() {
> > -  if (window_ && window_->GetFocusManager())
> > -    window_->GetFocusManager()->**SetFocusedWindow(window_);
> > +  // There is no native controls that can have a focus.
> >  }
> >
> >  void NativeWidgetAura::**FocusNativeView(gfx::**NativeView native_view) {
> >
> >
> >

[Ben Goodger (Google), 2011-12-14 21:09:57]

My question is then "why (i.e. under what circumstances) would you want to
clear or reset native focus within a window?"

-Ben

On Wed, Dec 14, 2011 at 1:07 PM, <oshima@chromium.org> wrote:

> On 2011/12/14 18:17:36, Ben Goodger (Google) wrote:
>
>> Why not just check if active first?
>>
>
> a toplevel with transient parent may not be active but can have focus
> (like constrained window, or modal window)
>
>
>  Who calls ClearNativeFocus()?
>>
>
> This is called from FocusManager::ClearFocus, which is called in several
> places,
> but one that was
> causing the issue is FocusManager::**StoreFocusedView, which is called
> when a
> window lost activation.
>
> It is my understanding that ClearNativeFocus is to *reset* the native focus
> within the window,
> but not to move the focus owned by other toplevel window. At least, that's
> how
> it used to work
> on gtk. Maybe this should be renamed to ResetNativeFocus?
>
> Let me know what you think and/or have better solution. I'm happy to
> update the
> change.
>
> - oshima
>
>  -Ben
>>
>
>  On Wed, Dec 14, 2011 at 9:54 AM, <mailto:oshima@chromium.org> wrote:
>>
>
>  > Reviewers: Ben Goodger (Google),
>> >
>> > Description:
>> > Set aura window focus in NativeWidgetAura::****ClearNativeFocus only
>> if the
>>
>> > focus is
>> > owned by child window.
>> >
>> >  This was causing a focus to be stolen when non active window calls
>> > ClearNativeFocus.
>> >
>> > BUG=none
>> > TEST=OmniboxViewTest.****PopupAccelerators passes with this change
>> >
>> >
>> > Please review this at
>>
>
> http://codereview.chromium.****org/8931022/%3Chttp://coderevi**
> ew.chromium.org/8931022/ <http://codereview.chromium.org/8931022/>>
>
>> >
>> > SVN Base:
>>
>
>
svn://svn.chromium.org/chrome/****trunk/src<http://svn.chromium.org/chrome/**trunk/src>
>
<http://svn.**chromium.org/chrome/trunk/src<http://svn.chromium.org/chrome/tru...
> >
>
>> >
>> > Affected files:
>> >  M ui/views/widget/native_widget_****aura.cc
>> >
>> >
>> > Index: ui/views/widget/native_widget_****aura.cc
>> > diff --git a/ui/views/widget/native_****widget_aura.cc
>> > b/ui/views/widget/native_****widget_aura.cc
>> > index 931eca58a7996542bf207adca9badd****c4161af030..**
>> > e380806d83ea4c1b1087ffb7b2ddca****ab9db5e4b4 100644
>> > --- a/ui/views/widget/native_****widget_aura.cc
>> > +++ b/ui/views/widget/native_****widget_aura.cc
>> > @@ -528,8 +528,7 @@ void NativeWidgetAura::SetCursor(****
>> gfx::NativeCursor
>> > cursor) {
>> >  }
>> >
>> >  void NativeWidgetAura::****ClearNativeFocus() {
>>
>> > -  if (window_ && window_->GetFocusManager())
>> > -    window_->GetFocusManager()->****SetFocusedWindow(window_);
>>
>> > +  // There is no native controls that can have a focus.
>> >  }
>> >
>> >  void NativeWidgetAura::****FocusNativeView(gfx::****NativeView
>> native_view) {
>> >
>> >
>> >
>>
>
>
>
>
http://codereview.chromium.**org/8931022/<http://codereview.chromium.org/8931...
>

[oshima, 2011-12-14 21:27:34]

On Wed, Dec 14, 2011 at 1:09 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> My question is then "why (i.e. under what circumstances) would you want to
> clear or reset native focus within a window?"


Ah, ok. here is the scenario that require resetting focus.

1) Set focus to RWHVA. (or a native control on gtk/win)
2) move the focus back to a view outside of RWHV (or native control on
gtk/win) using keyboard (tab).
3) View::OnFocus calls ClearNativeFocus to make sure all events goes to
toplevel window.

without resetting, events still goes to RWHVA (or native control).

- oshima


>
> -Ben
>
>
> On Wed, Dec 14, 2011 at 1:07 PM, <oshima@chromium.org> wrote:
>
>> On 2011/12/14 18:17:36, Ben Goodger (Google) wrote:
>>
>>> Why not just check if active first?
>>>
>>
>> a toplevel with transient parent may not be active but can have focus
>> (like constrained window, or modal window)
>>
>>
>>  Who calls ClearNativeFocus()?
>>>
>>
>> This is called from FocusManager::ClearFocus, which is called in several
>> places,
>> but one that was
>> causing the issue is FocusManager::**StoreFocusedView, which is called
>> when a
>> window lost activation.
>>
>> It is my understanding that ClearNativeFocus is to *reset* the native
>> focus
>> within the window,
>> but not to move the focus owned by other toplevel window. At least,
>> that's how
>> it used to work
>> on gtk. Maybe this should be renamed to ResetNativeFocus?
>>
>> Let me know what you think and/or have better solution. I'm happy to
>> update the
>> change.
>>
>> - oshima
>>
>>  -Ben
>>>
>>
>>  On Wed, Dec 14, 2011 at 9:54 AM, <mailto:oshima@chromium.org> wrote:
>>>
>>
>>  > Reviewers: Ben Goodger (Google),
>>> >
>>> > Description:
>>> > Set aura window focus in NativeWidgetAura::****ClearNativeFocus only
>>> if the
>>>
>>> > focus is
>>> > owned by child window.
>>> >
>>> >  This was causing a focus to be stolen when non active window calls
>>> > ClearNativeFocus.
>>> >
>>> > BUG=none
>>> > TEST=OmniboxViewTest.****PopupAccelerators passes with this change
>>> >
>>> >
>>> > Please review this at
>>>
>>
>> http://codereview.chromium.****org/8931022/%3Chttp://coderevi**
>> ew.chromium.org/8931022/ <http://codereview.chromium.org/8931022/>>
>>
>>> >
>>> > SVN Base:
>>>
>>
>>
svn://svn.chromium.org/chrome/****trunk/src<http://svn.chromium.org/chrome/**trunk/src>
>>
<http://svn.**chromium.org/chrome/trunk/src<http://svn.chromium.org/chrome/tru...
>> >
>>
>>> >
>>> > Affected files:
>>> >  M ui/views/widget/native_widget_****aura.cc
>>> >
>>> >
>>> > Index: ui/views/widget/native_widget_****aura.cc
>>> > diff --git a/ui/views/widget/native_****widget_aura.cc
>>> > b/ui/views/widget/native_****widget_aura.cc
>>> > index 931eca58a7996542bf207adca9badd****c4161af030..**
>>> > e380806d83ea4c1b1087ffb7b2ddca****ab9db5e4b4 100644
>>> > --- a/ui/views/widget/native_****widget_aura.cc
>>> > +++ b/ui/views/widget/native_****widget_aura.cc
>>> > @@ -528,8 +528,7 @@ void NativeWidgetAura::SetCursor(****
>>> gfx::NativeCursor
>>> > cursor) {
>>> >  }
>>> >
>>> >  void NativeWidgetAura::****ClearNativeFocus() {
>>>
>>> > -  if (window_ && window_->GetFocusManager())
>>> > -    window_->GetFocusManager()->****SetFocusedWindow(window_);
>>>
>>> > +  // There is no native controls that can have a focus.
>>> >  }
>>> >
>>> >  void NativeWidgetAura::****FocusNativeView(gfx::****NativeView
>>> native_view) {
>>> >
>>> >
>>> >
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/8931022/<http://codereview.chromium.org/8931...
>>
>
>

[Ben Goodger (Google), 2011-12-14 21:35:02]

So this would indicate that we need to keep this function for Aura, right?

-Ben

On Wed, Dec 14, 2011 at 1:22 PM, oshima <oshima@chromium.org> wrote:

>
>
> On Wed, Dec 14, 2011 at 1:09 PM, Ben Goodger (Google) <ben@chromium.org>wrote:
>
>> My question is then "why (i.e. under what circumstances) would you want
>> to clear or reset native focus within a window?"
>
>
> Ah, ok. here is the scenario that require resetting focus.
>
> 1) Set focus to RWHVA. (or a native control on gtk/win)
> 2) move the focus back to a view outside of RWHV (or native control on
> gtk/win) using keyboard (tab).
> 3) View::OnFocus calls ClearNativeFocus to make sure all events goes to
> toplevel window.
>
> without resetting, events still goes to RWHVA (or native control).
>
> - oshima
>
>
>>
>> -Ben
>>
>>
>> On Wed, Dec 14, 2011 at 1:07 PM, <oshima@chromium.org> wrote:
>>
>>> On 2011/12/14 18:17:36, Ben Goodger (Google) wrote:
>>>
>>>> Why not just check if active first?
>>>>
>>>
>>> a toplevel with transient parent may not be active but can have focus
>>> (like constrained window, or modal window)
>>>
>>>
>>>  Who calls ClearNativeFocus()?
>>>>
>>>
>>> This is called from FocusManager::ClearFocus, which is called in several
>>> places,
>>> but one that was
>>> causing the issue is FocusManager::**StoreFocusedView, which is called
>>> when a
>>> window lost activation.
>>>
>>> It is my understanding that ClearNativeFocus is to *reset* the native
>>> focus
>>> within the window,
>>> but not to move the focus owned by other toplevel window. At least,
>>> that's how
>>> it used to work
>>> on gtk. Maybe this should be renamed to ResetNativeFocus?
>>>
>>> Let me know what you think and/or have better solution. I'm happy to
>>> update the
>>> change.
>>>
>>> - oshima
>>>
>>>  -Ben
>>>>
>>>
>>>  On Wed, Dec 14, 2011 at 9:54 AM, <mailto:oshima@chromium.org> wrote:
>>>>
>>>
>>>  > Reviewers: Ben Goodger (Google),
>>>> >
>>>> > Description:
>>>> > Set aura window focus in NativeWidgetAura::****ClearNativeFocus only
>>>> if the
>>>>
>>>> > focus is
>>>> > owned by child window.
>>>> >
>>>> >  This was causing a focus to be stolen when non active window calls
>>>> > ClearNativeFocus.
>>>> >
>>>> > BUG=none
>>>> > TEST=OmniboxViewTest.****PopupAccelerators passes with this change
>>>> >
>>>> >
>>>> > Please review this at
>>>>
>>>
>>> http://codereview.chromium.****org/8931022/%3Chttp://coderevi**
>>> ew.chromium.org/8931022/ <http://codereview.chromium.org/8931022/>>
>>>
>>>> >
>>>> > SVN Base:
>>>>
>>>
>>>
svn://svn.chromium.org/chrome/****trunk/src<http://svn.chromium.org/chrome/**trunk/src>
>>>
<http://svn.**chromium.org/chrome/trunk/src<http://svn.chromium.org/chrome/tru...
>>> >
>>>
>>>> >
>>>> > Affected files:
>>>> >  M ui/views/widget/native_widget_****aura.cc
>>>> >
>>>> >
>>>> > Index: ui/views/widget/native_widget_****aura.cc
>>>> > diff --git a/ui/views/widget/native_****widget_aura.cc
>>>> > b/ui/views/widget/native_****widget_aura.cc
>>>> > index 931eca58a7996542bf207adca9badd****c4161af030..**
>>>> > e380806d83ea4c1b1087ffb7b2ddca****ab9db5e4b4 100644
>>>> > --- a/ui/views/widget/native_****widget_aura.cc
>>>> > +++ b/ui/views/widget/native_****widget_aura.cc
>>>> > @@ -528,8 +528,7 @@ void NativeWidgetAura::SetCursor(****
>>>> gfx::NativeCursor
>>>> > cursor) {
>>>> >  }
>>>> >
>>>> >  void NativeWidgetAura::****ClearNativeFocus() {
>>>>
>>>> > -  if (window_ && window_->GetFocusManager())
>>>> > -    window_->GetFocusManager()->****SetFocusedWindow(window_);
>>>>
>>>> > +  // There is no native controls that can have a focus.
>>>> >  }
>>>> >
>>>> >  void NativeWidgetAura::****FocusNativeView(gfx::****NativeView
>>>> native_view) {
>>>> >
>>>> >
>>>> >
>>>>
>>>
>>>
>>>
>>>
http://codereview.chromium.**org/8931022/<http://codereview.chromium.org/8931...
>>>
>>
>>
>

[oshima, 2011-12-14 21:49:14]

On Wed, Dec 14, 2011 at 1:34 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> So this would indicate that we need to keep this function for Aura, right?


Yes, aura still needs to reset the focus, but iff a child window had the
focus.

- oshima


> -Ben
>
>
> On Wed, Dec 14, 2011 at 1:22 PM, oshima <oshima@chromium.org> wrote:
>
>>
>>
>> On Wed, Dec 14, 2011 at 1:09 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>>
>>> My question is then "why (i.e. under what circumstances) would you want
>>> to clear or reset native focus within a window?"
>>
>>
>> Ah, ok. here is the scenario that require resetting focus.
>>
>> 1) Set focus to RWHVA. (or a native control on gtk/win)
>> 2) move the focus back to a view outside of RWHV (or native control on
>> gtk/win) using keyboard (tab).
>> 3) View::OnFocus calls ClearNativeFocus to make sure all events goes to
>> toplevel window.
>>
>> without resetting, events still goes to RWHVA (or native control).
>>
>> - oshima
>>
>>
>>>
>>> -Ben
>>>
>>>
>>> On Wed, Dec 14, 2011 at 1:07 PM, <oshima@chromium.org> wrote:
>>>
>>>> On 2011/12/14 18:17:36, Ben Goodger (Google) wrote:
>>>>
>>>>> Why not just check if active first?
>>>>>
>>>>
>>>> a toplevel with transient parent may not be active but can have focus
>>>> (like constrained window, or modal window)
>>>>
>>>>
>>>>  Who calls ClearNativeFocus()?
>>>>>
>>>>
>>>> This is called from FocusManager::ClearFocus, which is called in
>>>> several places,
>>>> but one that was
>>>> causing the issue is FocusManager::**StoreFocusedView, which is called
>>>> when a
>>>> window lost activation.
>>>>
>>>> It is my understanding that ClearNativeFocus is to *reset* the native
>>>> focus
>>>> within the window,
>>>> but not to move the focus owned by other toplevel window. At least,
>>>> that's how
>>>> it used to work
>>>> on gtk. Maybe this should be renamed to ResetNativeFocus?
>>>>
>>>> Let me know what you think and/or have better solution. I'm happy to
>>>> update the
>>>> change.
>>>>
>>>> - oshima
>>>>
>>>>  -Ben
>>>>>
>>>>
>>>>  On Wed, Dec 14, 2011 at 9:54 AM, <mailto:oshima@chromium.org> wrote:
>>>>>
>>>>
>>>>  > Reviewers: Ben Goodger (Google),
>>>>> >
>>>>> > Description:
>>>>> > Set aura window focus in NativeWidgetAura::****ClearNativeFocus
>>>>> only if the
>>>>>
>>>>> > focus is
>>>>> > owned by child window.
>>>>> >
>>>>> >  This was causing a focus to be stolen when non active window calls
>>>>> > ClearNativeFocus.
>>>>> >
>>>>> > BUG=none
>>>>> > TEST=OmniboxViewTest.****PopupAccelerators passes with this change
>>>>> >
>>>>> >
>>>>> > Please review this at
>>>>>
>>>>
>>>> http://codereview.chromium.****org/8931022/%3Chttp://coderevi**
>>>> ew.chromium.org/8931022/ <http://codereview.chromium.org/8931022/>>
>>>>
>>>>> >
>>>>> > SVN Base:
>>>>>
>>>>
>>>>
svn://svn.chromium.org/chrome/****trunk/src<http://svn.chromium.org/chrome/**trunk/src>
>>>>
<http://svn.**chromium.org/chrome/trunk/src<http://svn.chromium.org/chrome/tru...
>>>> >
>>>>
>>>>> >
>>>>> > Affected files:
>>>>> >  M ui/views/widget/native_widget_****aura.cc
>>>>> >
>>>>> >
>>>>> > Index: ui/views/widget/native_widget_****aura.cc
>>>>> > diff --git a/ui/views/widget/native_****widget_aura.cc
>>>>> > b/ui/views/widget/native_****widget_aura.cc
>>>>> > index 931eca58a7996542bf207adca9badd****c4161af030..**
>>>>> > e380806d83ea4c1b1087ffb7b2ddca****ab9db5e4b4 100644
>>>>> > --- a/ui/views/widget/native_****widget_aura.cc
>>>>> > +++ b/ui/views/widget/native_****widget_aura.cc
>>>>> > @@ -528,8 +528,7 @@ void NativeWidgetAura::SetCursor(****
>>>>> gfx::NativeCursor
>>>>> > cursor) {
>>>>> >  }
>>>>> >
>>>>> >  void NativeWidgetAura::****ClearNativeFocus() {
>>>>>
>>>>> > -  if (window_ && window_->GetFocusManager())
>>>>> > -    window_->GetFocusManager()->****SetFocusedWindow(window_);
>>>>>
>>>>> > +  // There is no native controls that can have a focus.
>>>>> >  }
>>>>> >
>>>>> >  void NativeWidgetAura::****FocusNativeView(gfx::****NativeView
>>>>> native_view) {
>>>>> >
>>>>> >
>>>>> >
>>>>>
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.**org/8931022/<http://codereview.chromium.org/8931...
>>>>
>>>
>>>
>>
>

[Ben Goodger (Google), 2011-12-14 23:54:11]

lgtm

[commit-bot: I haz the power, 2011-12-14 23:57:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/8931022/5002

[commit-bot: I haz the power, 2011-12-15 01:14:17]

Change committed as 114561