Fixing the focus issue in open/save dialogs.

Fixing the focus issue in open/save dialogs.

BUG=127222
TEST=manual test.


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

[oshima, 2012-06-22 14:34:41]

lgtm. can you add the comment (see below) and assign the bug to me?

https://chromiumcodereview.appspot.com/10444049/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

https://chromiumcodereview.appspot.com/10444049/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_dialog.cc:194: 
can you please add
// TODO(oshima): Views + aura doesn't seem to update the views focus
// manager when an aura window gets focus. This is a workaround for
// this issue. Fix it this and remove this workaround.
// See bug.com/127222.

[SeRya, 2012-06-22 14:45:27]

https://chromiumcodereview.appspot.com/10444049/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

https://chromiumcodereview.appspot.com/10444049/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_dialog.cc:194: 
On 2012/06/22 14:34:41, oshima wrote:
> can you please add
> // TODO(oshima): Views + aura doesn't seem to update the views focus
> // manager when an aura window gets focus. This is a workaround for
> // this issue. Fix it this and remove this workaround.
> // See bug.com/127222.

Done.

[commit-bot: I haz the power, 2012-06-22 14:45:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/10444049/10001

[commit-bot: I haz the power, 2012-06-22 14:45:55]

Presubmit check for 10444049-10001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui

Presubmit checks took 3.7s to calculate.

[sky, 2012-06-25 14:25:49]

Why aren't we fixing this the right way?

[SeRya, 2012-06-27 10:22:59]

On 2012/06/25 14:25:49, sky wrote:
> Why aren't we fixing this the right way?

Right fix looks more complicated. @oshima is going to implement it later. But we
(including @zelidrag) want to fix it to M21.

[sky, 2012-06-27 14:07:29]

On Wed, Jun 27, 2012 at 3:22 AM,  <serya@chromium.org> wrote:
> On 2012/06/25 14:25:49, sky wrote:
>>
>> Why aren't we fixing this the right way?
>
>
> Right fix looks more complicated. @oshima is going to implement it later.
> But we
> (including @zelidrag) want to fix it to M21.

We should fix it the right way then evaluate whether we should do an
easier backport to 21.

  -Scott

[oshima, 2012-06-27 20:50:50]

On 2012/06/27 14:07:29, sky wrote:
> On Wed, Jun 27, 2012 at 3:22 AM,  <mailto:serya@chromium.org> wrote:
> > On 2012/06/25 14:25:49, sky wrote:
> >>
> >> Why aren't we fixing this the right way?
> >
> >
> > Right fix looks more complicated. @oshima is going to implement it later.
> > But we
> > (including @zelidrag) want to fix it to M21.
> 
> We should fix it the right way then evaluate whether we should do an
> easier backport to 21.

I don't know what's the right fix yet. The comment was my guess from
observation.
I acutally do have CL that seems to work,
(http://codereview.chromium.org/10700009/), but Ben thinks
this isn't right way either because this shouldn't be necessary if aura's focus
works in the same way
as windows, and I need more time to investigate how this is different from
windows.

Sergay, do you have windows box? I can help you investigate this issue further.
Let me know.

> 
>   -Scott

[sky, 2012-06-28 22:47:51]

http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:200: view->Focus();
I have a feeling views doesn't get focus because NativeViewHost doesn't know who
to give focus to when the aura::Window gets focus. Try set_focus_view on
ExtensionView (which is a NativeViewHost).

[SeRya, 2012-06-29 09:35:55]

http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:200: view->Focus();
On 2012/06/28 22:47:51, sky wrote:
> I have a feeling views doesn't get focus because NativeViewHost doesn't know
who
> to give focus to when the aura::Window gets focus. Try set_focus_view on
> ExtensionView (which is a NativeViewHost).

ExtensionView does have focused view (host->view()->focus_view() != NULL in
ShowInternal, set_focus_view doesn't help) but the focus manager anyway has NULL
in focused_view_. So the real problem is somewhere else.

[oshima, 2012-06-29 14:30:20]

On 2012/06/29 09:35:55, SeRya wrote:
>
http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
> File chrome/browser/ui/views/extensions/extension_dialog.cc (right):
> 
>
http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
> chrome/browser/ui/views/extensions/extension_dialog.cc:200: view->Focus();
> On 2012/06/28 22:47:51, sky wrote:
> > I have a feeling views doesn't get focus because NativeViewHost doesn't know
> who
> > to give focus to when the aura::Window gets focus. Try set_focus_view on
> > ExtensionView (which is a NativeViewHost).
> 
> ExtensionView does have focused view (host->view()->focus_view() != NULL in
> ShowInternal, set_focus_view doesn't help) but the focus manager anyway has
NULL
> in focused_view_. So the real problem is somewhere else.

This patch fixes the behavior but not because its doing the right thing. (hence
this is just a workaround and needs real fix)
I just updated the bug with summary of my investigation , so please take a look.

[Ben Goodger (Google), 2012-07-11 16:06:38]

http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:199:
focus_manager->SetFocusedView(GetContentsView());
So who's going to fix the underlying issue?

[SeRya, 2012-07-11 16:39:44]

http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:199:
focus_manager->SetFocusedView(GetContentsView());
On 2012/07/11 16:06:38, Ben Goodger (Google) wrote:
> So who's going to fix the underlying issue?

@oshima

[Ben Goodger (Google), 2012-07-11 16:56:47]

OK. LGTM.


On Wed, Jul 11, 2012 at 9:39 AM, <serya@chromium.org> wrote:

>
> http://codereview.chromium.**org/10444049/diff/10001/**
>
chrome/browser/ui/views/**extensions/extension_dialog.cc<http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/extensions/extension_dialog.cc>
> File chrome/browser/ui/views/**extensions/extension_dialog.cc (right):
>
> http://codereview.chromium.**org/10444049/diff/10001/**
>
chrome/browser/ui/views/**extensions/extension_dialog.**cc#newcode199<http://codereview.chromium.org/10444049/diff/10001/chrome/browser/ui/views/extensions/extension_dialog.cc#newcode199>
> chrome/browser/ui/views/**extensions/extension_dialog.**cc:199:
> focus_manager->SetFocusedView(**GetContentsView());
> On 2012/07/11 16:06:38, Ben Goodger (Google) wrote:
>
>> So who's going to fix the underlying issue?
>>
>
> @oshima
>
>
http://codereview.chromium.**org/10444049/<http://codereview.chromium.org/104...
>

[commit-bot: I haz the power, 2012-07-11 19:46:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/serya@chromium.org/10444049/10001

[commit-bot: I haz the power, 2012-07-11 21:03:37]

Change committed as 146202