Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(384)

Issue 272024: Allow ESC to cancel ALT+SHIFT+T in Toolbar (Closed)

Created:
11 years, 2 months ago by Mohamed Mansour
Modified:
9 years, 6 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 1

Patch Set 2 : Preserve last focused view upon focusing toolbar #

Total comments: 4

Patch Set 3 : Usage of ViewStorage #

Total comments: 9

Patch Set 4 : Change default state when no focused view present. #

Patch Set 5 : Only allow one traversal at a time #

Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -16 lines) Patch
M chrome/browser/views/frame/browser_view.cc View 2 3 1 chunk +1 line, -7 lines 0 comments Download
M chrome/browser/views/toolbar_view.h View 1 2 3 3 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/views/toolbar_view.cc View 1 2 3 4 6 chunks +51 lines, -9 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
Mohamed Mansour
Please review. ALT+SHIFT+T properly exists now.
11 years, 2 months ago (2009-10-11 23:05:39 UTC) #1
sky
LGTM
11 years, 2 months ago (2009-10-12 16:23:23 UTC) #2
Mohamed Mansour
jcampan, anything you want to add, or safe to commit?
11 years, 2 months ago (2009-10-12 17:12:19 UTC) #3
jcampan
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 ...
11 years, 2 months ago (2009-10-12 17:17:45 UTC) #4
Mohamed Mansour
On 2009/10/12 17:17:45, jcampan wrote: > What happens with the focus at that point? Is ...
11 years, 2 months ago (2009-10-12 18:22:49 UTC) #5
Mohamed Mansour
From the discussion on the phone with Jonas, I believe we all agreed that the ...
11 years, 2 months ago (2009-10-14 21:33:12 UTC) #6
jcampan
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 ...
11 years, 2 months ago (2009-10-14 22:23:25 UTC) #7
jcampan
11 years, 2 months ago (2009-10-14 22:24:25 UTC) #8
Mohamed Mansour
Thanks for telling me about ViewStorage! Let me know if its the right way using ...
11 years, 2 months ago (2009-10-14 23:37:47 UTC) #9
jcampan
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 ...
11 years, 2 months ago (2009-10-14 23:47:01 UTC) #10
Mohamed Mansour
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: ...
11 years, 2 months ago (2009-10-15 05:08:15 UTC) #11
jcampan
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: ...
11 years, 2 months ago (2009-10-15 16:36:49 UTC) #12
Mohamed Mansour
I had some free time today, so I looked at this again. I changed the ...
11 years, 2 months ago (2009-10-18 22:28:01 UTC) #13
Mohamed Mansour
It now works as intended! Discussed with Jay on IM regarding my last issue, and ...
11 years, 2 months ago (2009-10-19 23:39:06 UTC) #14
Mohamed Mansour
Safe to commit? I want to commit this so I can start working on other ...
11 years, 2 months ago (2009-10-20 01:36:53 UTC) #15
jcampan
LGTM Sorry for the delay
11 years, 2 months ago (2009-10-20 15:41:58 UTC) #16
Jonas Klink (Google)
11 years, 2 months ago (2009-10-20 17:56:26 UTC) #17
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
>

Powered by Google App Engine
This is Rietveld 408576698