Add a keyboard shortcut on Escape that emits the IDC_STOP command....

Add a keyboard shortcut on Escape that emits the IDC_STOP command.

BUG=20916
TEST=Load something big (http://cnn.com/) and hit escape. Page load will stop.

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

[Nico, 2009-12-10 23:43:25]

Did you test that if you have a slow-loading HTML page with a e.g. a <select>
popup or a completion popup, esc closes that popup and doesn't stop loading?

(Easiest way to test this is probably by running your own local html server.)

http://codereview.chromium.org/491023/diff/1/2
File chrome/browser/global_keyboard_shortcuts_mac.mm (right):

http://codereview.chromium.org/491023/diff/1/2#newcode45
chrome/browser/global_keyboard_shortcuts_mac.mm:45: {false, false, false, false,
kVK_Escape,        IDC_STOP},
That way esc only stops loading if the web has focus. In Safari, it seems to
stop also if the url bar has focus.

[Scott Hess - ex-Googler, 2009-12-11 01:10:39]

http://codereview.chromium.org/491023/diff/1/2
File chrome/browser/global_keyboard_shortcuts_mac.mm (right):

http://codereview.chromium.org/491023/diff/1/2#newcode45
chrome/browser/global_keyboard_shortcuts_mac.mm:45: {false, false, false, false,
kVK_Escape,        IDC_STOP},
On 2009/12/10 23:43:25, Nico wrote:
> That way esc only stops loading if the web has focus. In Safari, it seems to
> stop also if the url bar has focus.

The AutocompleteEditViewMac::OnDoCommandBySelector() should return the result
from model_->OnEscapeKeyPressed() rather than returning true all the time. 
Don't know if something changed at some point, there.

[At least this looks like the intent of the model code.]

[Nico, 2009-12-11 01:23:56]

I think it's ok to get this in and then look at the omnibox esc handling later.

Hence, LGTM if the <select> thing doesn't break (it shouldn't, but please make
sure). Also, make sure that javascript can intercept the esc key (this should
Just Work, go to e.g. http://unixpapa.com/js/testkey.html and check that esc is
still sent as a key with your patch

[plz use chromium.org account, 2009-12-11 02:04:28]

Ok, I did the following test:

o Go to http://unixpapa.com/js/testkey.html and hit escape a few times ->
keypress events are displayed

o Load a huge site and hit escape -> page load stops

o Load a huge site, focus the omnibox, hit escape -> page load stops

o Load a huge site, open a <select> box, hit escape -> box closes, page
continues to load

o Load a huge site, start typing in the omnibox until suggestions pop up, hit
escape -> old URL is displayed, page continues to load

[Scott Hess - ex-Googler, 2009-12-11 02:07:58]

LGTM

http://codereview.chromium.org/491023/diff/2002/2004
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/491023/diff/2002/2004#newcode603
chrome/browser/autocomplete/autocomplete_edit_view_mac.mm:603: return
model_->OnEscapeKeyPressed();
I think this is right.  The things that should continue to work are:

 - type in omnibox.  arrow down into the popup.  ESC should take you back to
what you typed (with completion), another ESC should take you back to the url of
the page.

 - type in omnibox and ESC should take you back to the url of the page.

[And, of course, your case of ESC after you're back to the url of the page.]

[Nico, 2009-12-11 02:10:50]

Another test case:

o Open find bar, hit esc. It should close, loading shouldn't stop.

[plz use chromium.org account, 2009-12-11 02:21:08]

ok, the tests shess suggested work. The find bar doesn't however. When I move
the Escape definition to the lower block, however, hitting escape when the
omnibox is focused doesn't make the page stop loading...

I'll try to figure out what's going on there.

[plz use chromium.org account, 2009-12-16 16:41:20]

Ok, I've introduced a new hook to catch window level keyboard shortcuts which is
executed *after* all other widgets got the chance to handle the event. With this
change, all described test cases pass.

[Scott Hess - ex-Googler, 2009-12-16 19:45:49]

LGTM, but I feel sad that someday I'll have to debug a problem related to
event-dispatch being so funky.

[Nico, 2009-12-16 20:03:08]

LGTM. Doesn't look as bad as it sounds.

http://codereview.chromium.org/491023/diff/6001/7004
File chrome/browser/global_keyboard_shortcuts_mac.h (right):

http://codereview.chromium.org/491023/diff/6001/7004#newcode31
chrome/browser/global_keyboard_shortcuts_mac.h:31: // without rewriting event
dispatching ( http://crbug.com/251069 ).
Can you update this comment? The flow it currently describes is for key
equivalents; also describe the normal key flow (keyDown:, then pKE: if keyDown:
doesn't consume the event)

http://codereview.chromium.org/491023/diff/6001/7004#newcode36
chrome/browser/global_keyboard_shortcuts_mac.h:36: // which first checks if the
current web page wants to handle the shortcut).
I guess what I really want is a comment that explains which table you'd want to
add a new event to. Should probably in the mm file.

[plz use chromium.org account, 2009-12-17 10:34:50]

ok, here are the updated comments