Allow ESC to cancel ALT+SHIFT+T in Toolbar

Allow ESC to cancel ALT+SHIFT+T in Toolbar.

By overriding SkipDefaultKeyEventProcessing in View, it will ensure that the current hackish implementation of Toolbar request focus will be dealt before any other focus related actions and accelerators.

BUG=8329
TEST=Press ESC while in accessibility mode.

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

[Mohamed Mansour, 2009-10-11 23:05:39]

Please review. ALT+SHIFT+T properly exists now.

[sky, 2009-10-12 16:23:23]

LGTM

[Mohamed Mansour, 2009-10-12 17:12:19]

jcampan, anything you want to add, or safe to commit?

[jcampan, 2009-10-12 17:17:45]

http://codereview.chromium.org/272024/diff/1/2
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/272024/diff/1/2#newcode721
Line 721: return true;
What happens with the focus at that point? Is it still on the ToolbarView?
Should we focus something else?

[Mohamed Mansour, 2009-10-12 18:22:49]

On 2009/10/12 17:17:45, jcampan wrote:
> What happens with the focus at that point? Is it still on the ToolbarView?
> Should we focus something else?

I believe we should focus on something else, like the omnibar? thoughts? We
don't save the state of the last focused view "before" we ALT+SHIFT+T. We
"could" do that, and when you press ESC, it will go back to that view. But that
would require another storage reference for that last focused view.

[Mohamed Mansour, 2009-10-14 21:33:12]

From the discussion on the phone with Jonas, I believe we all agreed that the
last once we press ESC in the Toolbar, it should request focus back to the view
that it had focus.

Please re-review, this CL will properly send focus back to the focused element
before we ALT+SHIFT+T.

[jcampan, 2009-10-14 22:23:25]

http://codereview.chromium.org/272024/diff/4001/4004
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/272024/diff/4001/4004#newcode153
Line 153: acc_focused_view_(NULL),
last_acc_focused_view_ should be set to NULL.

http://codereview.chromium.org/272024/diff/4001/4005
File chrome/browser/views/toolbar_view.h (right):

http://codereview.chromium.org/272024/diff/4001/4005#newcode220
Line 220: views::View* last_acc_focused_view_;
Actually in cases where we need to remember a view, we usually use the
ViewStorage class. I can't think of any case today where the last focused view
might be deleted, but it is still probably safer to do so.

[Mohamed Mansour, 2009-10-14 23:37:47]

Thanks for telling me about ViewStorage! Let me know if its the right way using
it. 

Thanks.

http://codereview.chromium.org/272024/diff/4001/4004
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/272024/diff/4001/4004#newcode153
Line 153: acc_focused_view_(NULL),
On 2009/10/14 22:23:27, jcampan wrote:
> last_acc_focused_view_ should be set to NULL.

Done.

http://codereview.chromium.org/272024/diff/4001/4005
File chrome/browser/views/toolbar_view.h (right):

http://codereview.chromium.org/272024/diff/4001/4005#newcode220
Line 220: views::View* last_acc_focused_view_;
On 2009/10/14 22:23:27, jcampan wrote:
> Actually in cases where we need to remember a view, we usually use the
> ViewStorage class. I can't think of any case today where the last focused view
> might be deleted, but it is still probably safer to do so.
> 

Done.

[jcampan, 2009-10-14 23:47:01]

http://codereview.chromium.org/272024/diff/1003/7003
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/272024/diff/1003/7003#newcode155
Line 155: last_focused_view_storage_id_(-1),
This should be initialized to
ViewStorage::GetSharedInstance()->CreateStorageID().

http://codereview.chromium.org/272024/diff/1003/7003#newcode252
Line 252: if (view_storage->RetrieveView(last_focused_view_storage_id_) != NULL)
If you removed the view from the ViewStorage, you won't have to do that here.

http://codereview.chromium.org/272024/diff/1003/7003#newcode787
Line 787: if (focused_view)
Following my comment above, you should remove the view from the ViewStorage at
that point.

http://codereview.chromium.org/272024/diff/1003/7003#newcode789
Line 789: return true;
If focused_view is NULL, we don't want to leave the focus on the Toolbar. We
should probably focus the location bar.

[Mohamed Mansour, 2009-10-15 05:08:15]

Please re-review.

http://codereview.chromium.org/272024/diff/1003/7003
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/272024/diff/1003/7003#newcode155
Line 155: last_focused_view_storage_id_(-1),
On 2009/10/14 23:47:01, jcampan wrote:
> This should be initialized to
> ViewStorage::GetSharedInstance()->CreateStorageID().

Done.

http://codereview.chromium.org/272024/diff/1003/7003#newcode252
Line 252: if (view_storage->RetrieveView(last_focused_view_storage_id_) != NULL)
On 2009/10/14 23:47:01, jcampan wrote:
> If you removed the view from the ViewStorage, you won't have to do that here.

Done.

http://codereview.chromium.org/272024/diff/1003/7003#newcode787
Line 787: if (focused_view)
On 2009/10/14 23:47:01, jcampan wrote:
> Following my comment above, you should remove the view from the ViewStorage at
> that point.

Done.

http://codereview.chromium.org/272024/diff/1003/7003#newcode789
Line 789: return true;
On 2009/10/14 23:47:01, jcampan wrote:
> If focused_view is NULL, we don't want to leave the focus on the Toolbar. We
> should probably focus the location bar.

Done. Arrg, nice catch! So what I did here, I moved the acc_focused_view down
into the if condition, so if the focused view is null, the toolbar will stay
focused.

[jcampan, 2009-10-15 16:36:49]

http://codereview.chromium.org/272024/diff/1003/7003
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/272024/diff/1003/7003#newcode789
Line 789: return true;
On 2009/10/15 05:08:15, Mohamed Mansour wrote:
> On 2009/10/14 23:47:01, jcampan wrote:
> > If focused_view is NULL, we don't want to leave the focus on the Toolbar. We
> > should probably focus the location bar.
> 
> Done. Arrg, nice catch! So what I did here, I moved the acc_focused_view down
> into the if condition, so if the focused view is null, the toolbar will stay
> focused.

But that is not what we want. 
We don't want to leave the location bar focused (as it appears as if nothing was
focused). If focused_view is NULL, we want to default to focusing the location
bar.
And we should always exit the traversal mode when ESC is pressed (so we should
always SetHotTracked to false on acc_focused_view).

[Mohamed Mansour, 2009-10-18 22:28:01]

I had some free time today, so I looked at this again. I changed the default
focusing to the location bar. 

On 2009/10/15 16:36:49, jcampan wrote:
> And we should always exit the traversal mode when ESC is pressed (so we should
> always SetHotTracked to false on acc_focused_view).

I believe we don't even need SetHotTracked when we press ESC, simply because it
is already handled in WillLoseFocus, since we are always requesting focus to
another view (which is focusable) WillLoseFocus will "always" get fired.

BUT ... There is one problem, if we click on the Frame border, maximize,
minimize, close buttons (even by a simple hover). WillLoseFocus will not be
triggered. I believe this is by design, and that causes problems with the
toolbar focusing. If a user presses ALT+SHIFT+T again when he clicks on the
frame, hottracked items will still visible (since resetting them depends on the
lost of focus), and  ViewStorage will hit a DCHECK since it will try to add a
new id again which already exists.

I can fix the DCHECK hit by just doing what I did in my PatchSet#2 (checking if
it exists, then remove), but that is ugly. Fixing the hottracked issue is
different.

Any suggestions?

[Mohamed Mansour, 2009-10-19 23:39:06]

It now works as intended! Discussed with Jay on IM regarding my last issue, and
this fix will work, yay!

[Mohamed Mansour, 2009-10-20 01:36:53]

Safe to commit? I want to commit this so I can start working on other areas
which require this :)
 - Mohamed Mansour


On Mon, Oct 19, 2009 at 7:39 PM, <mhm@chromium.org> wrote:

> It now works as intended! Discussed with Jay on IM regarding my last issue,
> and
> this fix will work, yay!
>
>
> http://codereview.chromium.org/272024
>

[jcampan, 2009-10-20 15:41:58]

LGTM
Sorry for the delay

[Jonas Klink (Google), 2009-10-20 17:56:26]

LGTM, too.

Thanks for doing this guys! I've been following the discussion, but since it
was mostly Focus-related issues, I did not want to make for an even
lengthier discussion by chiming in (and potentially adding to the
confusion).

On Tue, Oct 20, 2009 at 8:41 AM, <jcampan@chromium.org> wrote:

> LGTM
> Sorry for the delay
>
>
> http://codereview.chromium.org/272024
>