|
|
Created:
11 years, 3 months ago by jam Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, John Grabowski, Erik does not do reviews, pam+watch_chromium.org, Paweł Hajdan Jr., darin (slow to review), tim (not reviewing) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionDon't send tab switching/killing/creating keyboard accelerators to pages. This avoids tabs maliciously preventing closing using ctrl+f4/ctrl+w/alt+f4, and also hung/slow renderers from making tab cycling sluggish.
BUG=5496
TEST=added ui test
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27814
Patch Set 1 : '' #
Total comments: 3
Patch Set 2 : '' #
Total comments: 6
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 5
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : add missing accelerators and fix crash with deleted RWH being used #Patch Set 8 : no code change, just sync to head #Patch Set 9 : switched to interactive ui test since the browser test couldn't wait for the tab to close in linux #Patch Set 10 : call Browser:IsReservedAccelerator from tab_contents_view.cc instead of each platform file #
Total comments: 1
Messages
Total messages: 41 (0 generated)
http://codereview.chromium.org/224023/diff/1029/1034 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/224023/diff/1029/1034#newcode624 Line 624: command_id == IDC_CLOSE_TAB || Why not IDC_CLOSE_WINDOW? Seems like it should be here too. Another possibility: IDC_DUPLICATE_TAB (Not sure there is a shortcut for this ATM but there could be someday)
http://codereview.chromium.org/224023/diff/1029/1034 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/224023/diff/1029/1034#newcode624 Line 624: command_id == IDC_CLOSE_TAB || On 2009/09/24 23:08:26, Peter Kasting wrote: > Why not IDC_CLOSE_WINDOW? Seems like it should be here too. > > Another possibility: IDC_DUPLICATE_TAB (Not sure there is a shortcut for this > ATM but there could be someday) > both IDC_CLOSE_WINDOW and IDC_DUPLICATE_TAB don't seem to have associated accelerators, looking at chrome_dll.rc
http://codereview.chromium.org/224023/diff/27/47 File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right): http://codereview.chromium.org/224023/diff/27/47#newcode573 Line 573: return browser_view->IsReservedAccelerator(accelerator); I think this is incorrect. The code here is Gtk-only Linux FE and as such there is no BrowserView. You need to talk to some Gtk-specific object here instead. The ChromeOS code lives in a similarly named file in chrome/browser/views/tab_contents/* Also, it's icky that this code touches the BrowserView directly. http://codereview.chromium.org/224023/diff/27/44 File chrome/browser/tab_contents/tab_contents_view_mac.mm (right): http://codereview.chromium.org/224023/diff/27/44#newcode206 Line 206: // When done, undef Browser.ReserveKeyboardAccelerators test. Can you file a bug on this or make sure the respective bug is marked OS-Mac and Mstone:4 ReleaseBlock:Beta ? http://codereview.chromium.org/224023/diff/27/41 File chrome/browser/views/tab_contents/tab_contents_view_gtk.cc (right): http://codereview.chromium.org/224023/diff/27/41#newcode306 Line 306: return browser_view->IsReservedAccelerator(accelerator); I don't like the fact that this touches the BrowserView directly. You should go through the TabContentsDelegate. http://codereview.chromium.org/224023/diff/27/31 File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right): http://codereview.chromium.org/224023/diff/27/31#newcode403 Line 403: return browser_view->IsReservedAccelerator(accelerator); The reason why is that TabContentsViewWin is used by ChromeFrame, which has no BrowserView.
http://codereview.chromium.org/224023/diff/1029/1034 File chrome/browser/views/frame/browser_view.cc (right): http://codereview.chromium.org/224023/diff/1029/1034#newcode624 Line 624: command_id == IDC_CLOSE_TAB || On 2009/09/24 23:15:47, John Abd-El-Malek wrote: > On 2009/09/24 23:08:26, Peter Kasting wrote: > > Why not IDC_CLOSE_WINDOW? Seems like it should be here too. > > > > Another possibility: IDC_DUPLICATE_TAB (Not sure there is a shortcut for this > > ATM but there could be someday) > > > > both IDC_CLOSE_WINDOW and IDC_DUPLICATE_TAB don't seem to have associated > accelerators, looking at chrome_dll.rc IDC_CLOSE_WINDOW has one on GTK, and it's supposed to have one (ctrl-shift-w) on Windows; I'll track down why that's not there anymore but you should definitely add this.
On 2009/09/24 23:22:03, Peter Kasting wrote: > IDC_CLOSE_WINDOW has one on GTK, and it's supposed to have one (ctrl-shift-w) on > Windows; I'll track down why that's not there anymore but you should definitely > add this. http://codereview.chromium.org/231025 should re-add this. Also, regarding IDC_DUPLICATE_TAB, the question should be whether this is functionality we want to be unblockable (which I'm not sure of), not whether it currently has a shortcut; it might have one in the future, for example. It's too subtle to not add commands that we want to be unblockable but that don't currently have shortcuts.
http://codereview.chromium.org/224023/diff/27/47 File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right): http://codereview.chromium.org/224023/diff/27/47#newcode573 Line 573: return browser_view->IsReservedAccelerator(accelerator); On 2009/09/24 23:18:36, Ben Goodger wrote: > I think this is incorrect. The code here is Gtk-only Linux FE and as such there > is no BrowserView. You need to talk to some Gtk-specific object here instead. > The ChromeOS code lives in a similarly named file in > chrome/browser/views/tab_contents/* I see, ok I'll sync up with some Chrome OS folks tomorrow. > > Also, it's icky that this code touches the BrowserView directly. Agreed. But I didn't want to duplicate the knowledge of what keys map to which accelerators. I tried looking for a separate place to do this, or to have BrowserView create a singleton somewhere with the info about all the accelerators but I wasn't familiar enough with the code to know where to put it. Any suggestions? http://codereview.chromium.org/224023/diff/27/44 File chrome/browser/tab_contents/tab_contents_view_mac.mm (right): http://codereview.chromium.org/224023/diff/27/44#newcode206 Line 206: // When done, undef Browser.ReserveKeyboardAccelerators test. On 2009/09/24 23:18:36, Ben Goodger wrote: > Can you file a bug on this or make sure the respective bug is marked OS-Mac and > Mstone:4 ReleaseBlock:Beta ? I was planning on doing this before checkin, just wanted to send as is to the trybot to make sure I haven't regressed anything/broke the build on mac so far.
On Thu, Sep 24, 2009 at 4:28 PM, <pkasting@chromium.org> wrote: > On 2009/09/24 23:22:03, Peter Kasting wrote: > >> IDC_CLOSE_WINDOW has one on GTK, and it's supposed to have one >> (ctrl-shift-w) >> > on > >> Windows; I'll track down why that's not there anymore but you should >> > definitely > >> add this. >> > sure, done (haven't uploaded yet) > > http://codereview.chromium.org/231025 should re-add this. > > Also, regarding IDC_DUPLICATE_TAB, the question should be whether this is > functionality we want to be unblockable (which I'm not sure of), I'm not sure of that either. I think it depends on what the shortcut will be. > not whether it > currently has a shortcut; it might have one in the future, for example. > It's > too subtle to not add commands that we want to be unblockable but that > don't > currently have shortcuts. > > > http://codereview.chromium.org/224023 >
On 2009/09/24 23:31:03, John Abd-El-Malek wrote: > I see, ok I'll sync up with some Chrome OS folks tomorrow. ChromeOS actually just uses BrowserView ;-) Note that views::Accelerator is Windows/ChromeOS only... so if you want to share code you need to not translate into this type.
IMHO, this change may not be appropriate. According to http://www.w3.org/TR/DOM-Level-3-Events/#event-flow-cancelation, the default action of an event should be performed after the event dispatch has been completed, so that it can be canceled from DOM. With this change, these keyboard accelerators are no longer be cancelable, which might be a surprise. And I just tested IE7, Safari and Firefox, all of them allow to cancel these accelerators. Regards James Su On 2009/09/24 22:43:40, John Abd-El-Malek wrote: >
On 2009/09/25 23:23:23, James Su wrote: > IMHO, this change may not be appropriate. According to > http://www.w3.org/TR/DOM-Level-3-Events/#event-flow-cancelation, the default > action of an event should be performed after the event dispatch has been > completed, so that it can be canceled from DOM. > > With this change, these keyboard accelerators are no longer be cancelable, which > might be a surprise. > > And I just tested IE7, Safari and Firefox, all of them allow to cancel these > accelerators. I'm not sure what you used as your test case. But if you use this one, which is a slightly modified version of the test case I added that works in all browsers, you can see that they don't allow the page to cancel these accelerators. Check out the bug for the exact details and try this out yourself: <html><script> document.onkeydown = function() { var evt = event || window.event; evt.preventDefault(); return false; } </script></html> > > Regards > James Su > > On 2009/09/24 22:43:40, John Abd-El-Malek wrote: > >
I tested these browsers on this page: http://www.quirksmode.org/js/keys.html. You can try just return false in onkeydown handler. On 2009/09/25 23:33:03, John Abd-El-Malek wrote: > On 2009/09/25 23:23:23, James Su wrote: > > IMHO, this change may not be appropriate. According to > > http://www.w3.org/TR/DOM-Level-3-Events/#event-flow-cancelation, the default > > action of an event should be performed after the event dispatch has been > > completed, so that it can be canceled from DOM. > > > > With this change, these keyboard accelerators are no longer be cancelable, > which > > might be a surprise. > > > > And I just tested IE7, Safari and Firefox, all of them allow to cancel these > > accelerators. > > I'm not sure what you used as your test case. But if you use this one, which is > a slightly modified version of the test case I added that works in all browsers, > you can see that they don't allow the page to cancel these accelerators. Check > out the bug for the exact details and try this out yourself: > > <html><script> > document.onkeydown = function() { > var evt = event || window.event; > evt.preventDefault(); > return false; > } > </script></html> > > > > Regards > > James Su > > > > On 2009/09/24 22:43:40, John Abd-El-Malek wrote: > > >
On Fri, Sep 25, 2009 at 4:23 PM, <suzhe@chromium.org> wrote: > IMHO, this change may not be appropriate. According to > http://www.w3.org/TR/DOM-Level-3-Events/#event-flow-cancelation, the > default > action of an event should be performed after the event dispatch has been > completed, so that it can be canceled from DOM. > I understand that's in the spec, but per our discussion, and looking at what other browsers do, we feel that not allowing pages to swallow ctrl+tab, ctrl+w etc is better for the user. I think it's the same thing like Windows not letting a malicious/hung application disable alt+f4 or alt+tab. > With this change, these keyboard accelerators are no longer be cancelable, > which > might be a surprise. > > And I just tested IE7, Safari and Firefox, all of them allow to cancel > these > accelerators. > I had checked the test url in the bug which you referenced. ctrl+tab and ctrl+f4 works in all of them except Chrome. > Regards > James Su > > > On 2009/09/24 22:43:40, John Abd-El-Malek wrote: > > > > > http://codereview.chromium.org/224023 >
Thanks for the review comments. New patch up with suggested changes. I also switched to an in-process browser test instead of a ui_test since the latter would need to be an interactive test and I thought this would be more robust.
Hi, Just tested on another website: http://unixpapa.com/js/testkey.html. The keyboard events handling in browsers is a real mess! In this page, ctrl-tab, ctrl-p, ctrl-o, ctrl-f, ctrl-minus, ctrl-plus, ctrl-f3, ctrl-f4 can't be suppressed in both IE7 and Safari. All other ctrl keys can be suppressed. All keys, including ctrl-tab and ctrl-f4 can be suppressed in firefox. But no matter if an accelerator key can be suppressed in JavaScript, the key down event will always be sent to DOM first in all browsers. So I still think it's necessary to send all key down events to DOM before handling them in the browser, as stated in the spec. While when processing the key events in the browser, a few key bindings might always be handled no matter if they were suppressed or handled in DOM. But we may strict the list of insuppressible key events as short as we can. Regards James Su On 2009/09/29 19:49:31, John Abd-El-Malek wrote: > On Fri, Sep 25, 2009 at 4:23 PM, <mailto:suzhe@chromium.org> wrote: > > > IMHO, this change may not be appropriate. According to > > http://www.w3.org/TR/DOM-Level-3-Events/#event-flow-cancelation, the > > default > > action of an event should be performed after the event dispatch has been > > completed, so that it can be canceled from DOM. > > > > I understand that's in the spec, but per our discussion, and looking at what > other browsers do, we feel that not allowing pages to swallow ctrl+tab, > ctrl+w etc is better for the user. I think it's the same thing like Windows > not letting a malicious/hung application disable alt+f4 or alt+tab. > > > > With this change, these keyboard accelerators are no longer be cancelable, > > which > > might be a surprise. > > > > And I just tested IE7, Safari and Firefox, all of them allow to cancel > > these > > accelerators. > > > > I had checked the test url in the bug which you referenced. ctrl+tab and > ctrl+f4 works in all of them except Chrome. > > > > Regards > > James Su > > > > > > On 2009/09/24 22:43:40, John Abd-El-Malek wrote: > > > > > > > > > > http://codereview.chromium.org/224023 > > >
On Tue, Sep 29, 2009 at 6:26 PM, <suzhe@chromium.org> wrote: > Hi, > Just tested on another website: http://unixpapa.com/js/testkey.html. The > keyboard events handling in browsers is a real mess! > In this page, ctrl-tab, ctrl-p, ctrl-o, ctrl-f, ctrl-minus, ctrl-plus, > ctrl-f3, ctrl-f4 can't be suppressed in both IE7 and Safari. All other > ctrl > keys can be suppressed. All keys, including ctrl-tab and ctrl-f4 can be > suppressed in firefox. > But no matter if an accelerator key can be suppressed in JavaScript, the > key > down event will always be sent to DOM first in all browsers. > So I still think it's necessary to send all key down events to DOM before > handling them in the browser, as stated in the spec. While when processing > the > key events in the browser, a few key bindings might always be handled no > matter > if they were suppressed or handled in DOM. But we may strict the list of > insuppressible key events as short as we can. > I disagree :) What I'm saying above is that we're making a conscious decision to ignore part of the spec, similar to what was done to block annoying things like pop-ups. If the event goes to the page first, then there's no point in giving it to the browser until the renderer says it's done. But that's the crux of this bug, in which a hung/malicious renderer can prevent closing the tab (even alt+f4) or navigating away. I had double checked that this logic is fine with Ben/Darin and per the bug/thread they're ok with this approach. Let's see if we actually break any sites. > > Regards > James Su > > On 2009/09/29 19:49:31, John Abd-El-Malek wrote: > > On Fri, Sep 25, 2009 at 4:23 PM, <mailto:suzhe@chromium.org> wrote: >> > > > IMHO, this change may not be appropriate. According to >> > http://www.w3.org/TR/DOM-Level-3-Events/#event-flow-cancelation, the >> > default >> > action of an event should be performed after the event dispatch has been >> > completed, so that it can be canceled from DOM. >> > >> > > I understand that's in the spec, but per our discussion, and looking at >> what >> other browsers do, we feel that not allowing pages to swallow ctrl+tab, >> ctrl+w etc is better for the user. I think it's the same thing like >> Windows >> not letting a malicious/hung application disable alt+f4 or alt+tab. >> > > > > With this change, these keyboard accelerators are no longer be >> cancelable, >> > which >> > might be a surprise. >> > >> > And I just tested IE7, Safari and Firefox, all of them allow to cancel >> > these >> > accelerators. >> > >> > > I had checked the test url in the bug which you referenced. ctrl+tab and >> ctrl+f4 works in all of them except Chrome. >> > > > > Regards >> > James Su >> > >> > >> > On 2009/09/24 22:43:40, John Abd-El-Malek wrote: >> > >> > >> > >> > >> > http://codereview.chromium.org/224023 >> > >> > > > > > http://codereview.chromium.org/224023 >
On Tue, Sep 29, 2009 at 6:44 PM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Tue, Sep 29, 2009 at 6:26 PM, <suzhe@chromium.org> wrote: > >> Hi, >> Just tested on another website: http://unixpapa.com/js/testkey.html. The >> keyboard events handling in browsers is a real mess! >> In this page, ctrl-tab, ctrl-p, ctrl-o, ctrl-f, ctrl-minus, ctrl-plus, >> ctrl-f3, ctrl-f4 can't be suppressed in both IE7 and Safari. All other >> ctrl >> keys can be suppressed. All keys, including ctrl-tab and ctrl-f4 can be >> suppressed in firefox. >> But no matter if an accelerator key can be suppressed in JavaScript, the >> key >> down event will always be sent to DOM first in all browsers. >> So I still think it's necessary to send all key down events to DOM before >> handling them in the browser, as stated in the spec. While when processing >> the >> key events in the browser, a few key bindings might always be handled no >> matter >> if they were suppressed or handled in DOM. But we may strict the list of >> insuppressible key events as short as we can. >> > > I disagree :) What I'm saying above is that we're making > a conscious decision to ignore part of the spec, similar to what was done to > block annoying things like pop-ups. If the event goes to the page first, > then there's no point in giving it to the browser until the renderer says > it's done. But that's the crux of this bug, in which a hung/malicious > renderer can prevent closing the tab (even alt+f4) or navigating away. I > had double checked that this logic is fine with Ben/Darin and per the > bug/thread they're ok with this approach. Let's see if we actually break > any sites. > One more point: I think one other point that makes us unique to other browsers is the sandbox. Chrome's security model warrants that we don't trust the application with accelerators to "get out" or close it. We assume that the renderer process can be exploited. > > > >> >> Regards >> James Su >> >> On 2009/09/29 19:49:31, John Abd-El-Malek wrote: >> >> On Fri, Sep 25, 2009 at 4:23 PM, <mailto:suzhe@chromium.org> wrote: >>> >> >> > IMHO, this change may not be appropriate. According to >>> > http://www.w3.org/TR/DOM-Level-3-Events/#event-flow-cancelation, the >>> > default >>> > action of an event should be performed after the event dispatch has >>> been >>> > completed, so that it can be canceled from DOM. >>> > >>> >> >> I understand that's in the spec, but per our discussion, and looking at >>> what >>> other browsers do, we feel that not allowing pages to swallow ctrl+tab, >>> ctrl+w etc is better for the user. I think it's the same thing like >>> Windows >>> not letting a malicious/hung application disable alt+f4 or alt+tab. >>> >> >> >> > With this change, these keyboard accelerators are no longer be >>> cancelable, >>> > which >>> > might be a surprise. >>> > >>> > And I just tested IE7, Safari and Firefox, all of them allow to cancel >>> > these >>> > accelerators. >>> > >>> >> >> I had checked the test url in the bug which you referenced. ctrl+tab and >>> ctrl+f4 works in all of them except Chrome. >>> >> >> >> > Regards >>> > James Su >>> > >>> > >>> > On 2009/09/24 22:43:40, John Abd-El-Malek wrote: >>> > >>> > >>> > >>> > >>> > http://codereview.chromium.org/224023 >>> > >>> >> >> >> >> >> http://codereview.chromium.org/224023 >> > >
I agree with James. I don't think he was suggesting we wait till the renderer returns the event, but that we send the event and handle it ourselves in parallel. This allows us to satisfy compatibility (to an extent) and has no drawbacks that I can see. (I don't understand the security argument. The motivation for this change is not security; is there any plausible attack that a compromised renderer can make by knowing that we're navigating away from it?) http://codereview.chromium.org/224023/diff/14043/9067 File chrome/browser/browser.cc (right): http://codereview.chromium.org/224023/diff/14043/9067#newcode2119 Line 2119: event.windowsKeyCode == VK_F4) nit: curly braces for multi-line conditionals http://codereview.chromium.org/224023/diff/14043/9066 File chrome/browser/gtk/browser_window_gtk.cc (right): http://codereview.chromium.org/224023/diff/14043/9066#newcode1900 Line 1900: int command_id = ::GetCommandId(keyval, modifier); nit: no leading ::. We shouldn't be shadowing function names.
Another way to satisfy the use cases I can think of would be to make IsReservedAccelerator only return true if IsCommandEnabled(id) is also true (but it would also require improving the logic for IsCommandEnabled since it appears that some commands such as IDC_NEXT_TAB are always enabled, even when they shouldn't be.)
Hi, I agree that we should handle some accelerator keys specially to deal with a hung/malicious renderer, just like what we've done in RenderWidgetHostViewGtk for popups. However, I still think such accelerator keys should be very few, maybe only ctrl-f4. For all other key events I'd still prefer to send them to renderer first, though some of them may still be processed by the browser no matter if it's suppressed by the renderer or not. IMHO, this issue is more complicated than what you though. Following aspects shall be considered carefully: 1. Supports different keyboard layout. Like my comment below, keyboard layout support is a mess, at least on Linux. We need handle it carefully, to make sure our solution works on all if not most keyboard layouts. Actually, our views/gtk code branch doesn't support it for accelerator keys (We may address it separately). 2. In webkit, accesskeys are triggered by Char events rather than RawKeyDown events. It's actually the root cause of such conflict. To solve this conflict, we have following to different approaches: a) Only handles a key event as an accelerator if both the RawKeyDown and corresponding Char events are not handled/suppressed by the renderer. Firefox follows this approach. b) Don't send corresponding Char event to the renderer if preceding RawKeyDown event was handled as an accelerator. Both IE and Safari follows this approach. c) Don't send RawKeyDown/Char/KeyUp to the renderer at all if the RawKeyDown is an accelerator. When involving IME, things get even more complicated, because a Char event might be a fabricated event generated by IME, which may not have os_event at all, or have different keycode value. Though I'm not familiar with Windows programming, according to msdn, WM_CHAR message's wparam is a character code rather than a virtual keycode. So with your approach, it's hardly to block both RawKeyDown and corresponding Char event in RVH, which then may cause problem. So I suggest following solution: 1. Handle a (or maybe more) Tab close accelerator specially somewhere (maybe in RWHV), so that hung/malicious renderer can be closed correctly. The special tab close key must not be affected by keyboard layout. ctrl-F4 would be good. 2. Send all other key events to the renderer first. Then after receiving ACK message from the renderer, deliver the key event along with the renderer's processing result (processed or not) to RenderViewHostDelegate to let it decide whether or not to handle this event as an accelerator. If RenderViewHostDelegate handled a RawKeyDown event as an accelerator, then we shall discard the following Char event in RWH to prevent the renderer from handling it. In RenderViewHostDelegate and associated objects, because it knows if a RawKeyDown event was handled by the renderer or not, it can decide whether or not to handle it as an accelerator at the very last stage. For example, for gtk port, the decision can be done in BrowserWindowGtk::OnGtkAccelerator(), so that we don't need to handle the keyboard layout mess by ourselves. Is my above expression clear enough? Please forgive my poor English. Regards James Su http://codereview.chromium.org/224023/diff/14043/9066 File chrome/browser/gtk/browser_window_gtk.cc (right): http://codereview.chromium.org/224023/diff/14043/9066#newcode1170 Line 1170: return command; This piece of code may not correctly handle keyboard layouts other than US. In the normal key event handling path, gtk_window_activate_key() function handles the keyboard layout mess for us (See BrowserWindowGtk::HandleKeyboardEvent). But if GetCommandId() gets called directly against a keyval, it may not work in non-US layout. For example, in Russian keyboard kayout, ctrl-w generates keyval 0x06c3 (GDK_Cyrillic_tse) rather than 0x77 (GDK_w), then this method won't return IDC_CLOSE_TAB.
No. It's not just Ctrl+F4. There are several window management controls that are commonly used, and pages overriding these commands interfere with all of them. I think John's change covers this minimal set. -Ben On Tue, Sep 29, 2009 at 9:30 PM, <suzhe@chromium.org> wrote: > Hi, > =A0I agree that we should handle some accelerator keys specially to deal = with > a > hung/malicious renderer, just like what we've done in > RenderWidgetHostViewGtk > for popups. However, I still think such accelerator keys should be very f= ew, > maybe only ctrl-f4. For all other key events I'd still prefer to send the= m > to > renderer first, though some of them may still be processed by the browser= no > matter if it's suppressed by the renderer or not. > =A0IMHO, this issue is more complicated than what you though. =A0 =A0Foll= owing > aspects shall be considered carefully: > > 1. Supports different keyboard layout. Like my comment below, keyboard > layout > support is a mess, at least on Linux. We need handle it carefully, to mak= e > sure > our solution works on all if not most keyboard layouts. > =A0Actually, our views/gtk code branch doesn't support it for accelerator= keys > (We may address it separately). > > 2. In webkit, accesskeys are triggered by Char events rather than RawKeyD= own > events. It's actually the root cause of such conflict. To solve this > conflict, > we have following to different approaches: > > a) Only handles a key event as an accelerator if both the RawKeyDown and > corresponding Char events are not handled/suppressed by the renderer. > Firefox > follows this approach. > > b) Don't send corresponding Char event to the renderer if preceding > RawKeyDown > event was handled as an accelerator. Both IE and Safari follows this > approach. > > c) Don't send RawKeyDown/Char/KeyUp to the renderer at all if the RawKeyD= own > is > an accelerator. > > When involving IME, things get even more complicated, because a Char even= t > might > be a fabricated event generated by IME, which may not have os_event at al= l, > or > have different keycode value. Though I'm not familiar with Windows > programming, > according to msdn, WM_CHAR message's wparam is a character code rather th= an > a > virtual keycode. > So with your approach, it's hardly to block both RawKeyDown and > corresponding > Char event in RVH, which then may cause problem. > > > So I suggest following solution: > 1. Handle a (or maybe more) Tab close accelerator specially somewhere (ma= ybe > in > RWHV), so that hung/malicious renderer can be closed correctly. The speci= al > tab > close key must not be affected by keyboard layout. ctrl-F4 would be good. > > 2. Send all other key events to the renderer first. Then after receiving = ACK > message from the renderer, deliver the key event along with the renderer'= s > processing result (processed or not) to RenderViewHostDelegate to let it > decide > whether or not to handle this event as an accelerator. If > RenderViewHostDelegate > handled a RawKeyDown event as an accelerator, then we shall discard the > following Char event in RWH to prevent the renderer from handling it. > > In RenderViewHostDelegate and associated objects, because it knows if a > RawKeyDown event was handled by the renderer or not, it can decide whethe= r > or > not to handle it as an accelerator at the very last stage. For example, f= or > gtk > port, the decision can be done in BrowserWindowGtk::OnGtkAccelerator(), s= o > that > we don't need to handle the keyboard layout mess by ourselves. > > Is my above expression clear enough? Please forgive my poor English. > > Regards > James Su > > > http://codereview.chromium.org/224023/diff/14043/9066 > File chrome/browser/gtk/browser_window_gtk.cc (right): > > http://codereview.chromium.org/224023/diff/14043/9066#newcode1170 > Line 1170: return command; > This piece of code may not correctly handle keyboard layouts other than > US. > In the normal key event handling path, gtk_window_activate_key() > function handles the keyboard layout mess for us (See > BrowserWindowGtk::HandleKeyboardEvent). > But if GetCommandId() gets called directly against a keyval, =A0it may no= t > work in non-US layout. > For example, in Russian keyboard kayout, ctrl-w generates keyval 0x06c3 > (GDK_Cyrillic_tse) rather than 0x77 (GDK_w), then this method won't > return IDC_CLOSE_TAB. > > http://codereview.chromium.org/224023 >
Let me be more specific: Any accelerator that maps to a window management command should be filtered. We should not be in the business of deciding which keyboard shortcuts are filtered or not. You might use Ctrl+F4, but I use Ctrl+W. If there are other bugs, separate those out from this point at least. -Ben On Tue, Sep 29, 2009 at 9:31 PM, Ben Goodger (Google) <ben@chromium.org> wr= ote: > No. It's not just Ctrl+F4. There are several window management > controls that are commonly used, and pages overriding these commands > interfere with all of them. I think John's change covers this minimal > set. > > -Ben > > On Tue, Sep 29, 2009 at 9:30 PM, =A0<suzhe@chromium.org> wrote: >> Hi, >> =A0I agree that we should handle some accelerator keys specially to deal= with >> a >> hung/malicious renderer, just like what we've done in >> RenderWidgetHostViewGtk >> for popups. However, I still think such accelerator keys should be very = few, >> maybe only ctrl-f4. For all other key events I'd still prefer to send th= em >> to >> renderer first, though some of them may still be processed by the browse= r no >> matter if it's suppressed by the renderer or not. >> =A0IMHO, this issue is more complicated than what you though. =A0 =A0Fol= lowing >> aspects shall be considered carefully: >> >> 1. Supports different keyboard layout. Like my comment below, keyboard >> layout >> support is a mess, at least on Linux. We need handle it carefully, to ma= ke >> sure >> our solution works on all if not most keyboard layouts. >> =A0Actually, our views/gtk code branch doesn't support it for accelerato= r keys >> (We may address it separately). >> >> 2. In webkit, accesskeys are triggered by Char events rather than RawKey= Down >> events. It's actually the root cause of such conflict. To solve this >> conflict, >> we have following to different approaches: >> >> a) Only handles a key event as an accelerator if both the RawKeyDown and >> corresponding Char events are not handled/suppressed by the renderer. >> Firefox >> follows this approach. >> >> b) Don't send corresponding Char event to the renderer if preceding >> RawKeyDown >> event was handled as an accelerator. Both IE and Safari follows this >> approach. >> >> c) Don't send RawKeyDown/Char/KeyUp to the renderer at all if the RawKey= Down >> is >> an accelerator. >> >> When involving IME, things get even more complicated, because a Char eve= nt >> might >> be a fabricated event generated by IME, which may not have os_event at a= ll, >> or >> have different keycode value. Though I'm not familiar with Windows >> programming, >> according to msdn, WM_CHAR message's wparam is a character code rather t= han >> a >> virtual keycode. >> So with your approach, it's hardly to block both RawKeyDown and >> corresponding >> Char event in RVH, which then may cause problem. >> >> >> So I suggest following solution: >> 1. Handle a (or maybe more) Tab close accelerator specially somewhere (m= aybe >> in >> RWHV), so that hung/malicious renderer can be closed correctly. The spec= ial >> tab >> close key must not be affected by keyboard layout. ctrl-F4 would be good= . >> >> 2. Send all other key events to the renderer first. Then after receiving= ACK >> message from the renderer, deliver the key event along with the renderer= 's >> processing result (processed or not) to RenderViewHostDelegate to let it >> decide >> whether or not to handle this event as an accelerator. If >> RenderViewHostDelegate >> handled a RawKeyDown event as an accelerator, then we shall discard the >> following Char event in RWH to prevent the renderer from handling it. >> >> In RenderViewHostDelegate and associated objects, because it knows if a >> RawKeyDown event was handled by the renderer or not, it can decide wheth= er >> or >> not to handle it as an accelerator at the very last stage. For example, = for >> gtk >> port, the decision can be done in BrowserWindowGtk::OnGtkAccelerator(), = so >> that >> we don't need to handle the keyboard layout mess by ourselves. >> >> Is my above expression clear enough? Please forgive my poor English. >> >> Regards >> James Su >> >> >> http://codereview.chromium.org/224023/diff/14043/9066 >> File chrome/browser/gtk/browser_window_gtk.cc (right): >> >> http://codereview.chromium.org/224023/diff/14043/9066#newcode1170 >> Line 1170: return command; >> This piece of code may not correctly handle keyboard layouts other than >> US. >> In the normal key event handling path, gtk_window_activate_key() >> function handles the keyboard layout mess for us (See >> BrowserWindowGtk::HandleKeyboardEvent). >> But if GetCommandId() gets called directly against a keyval, =A0it may n= ot >> work in non-US layout. >> For example, in Russian keyboard kayout, ctrl-w generates keyval 0x06c3 >> (GDK_Cyrillic_tse) rather than 0x77 (GDK_w), then this method won't >> return IDC_CLOSE_TAB. >> >> http://codereview.chromium.org/224023 >> >
Hi, I agree to filter all Tab closing accelerators. The question is: How to do the job correctly. After reading my comments, you may notice that John's approach can't cover all situations, eg. the keyboard layout issue on Linux, and the Char event issue on all platforms. In a short word: If you want to filter a key event completely from the browser side, you must filter corresponding RawKeyDown and Char event altogether (KeyUp is probably harmless, but I'm not sure) and make it works correctly on all keyboard layouts! Filtering Tab closing key would be easy, because the tab will be closed afterwards, you don't need to care about following Char event anymore. But you still need to handle the keyboard layout issue correctly. Otherwise, ctrl-w may not work for a Russian user. But filtering other accelerator keys, like tab switching, is much more complicated. Because the original tab is still there, users don't expect the tab content to be changed when switching back. So both RawKeyDown and corresponding Char event must be filtered, as if there is no such event at all. Regards James Su On 2009/09/30 04:38:43, Ben Goodger wrote: > Let me be more specific: > > Any accelerator that maps to a window management command should be > filtered. We should not be in the business of deciding which keyboard > shortcuts are filtered or not. You might use Ctrl+F4, but I use > Ctrl+W. > > If there are other bugs, separate those out from this point at least. > > -Ben > > On Tue, Sep 29, 2009 at 9:31 PM, Ben Goodger (Google) <mailto:ben@chromium.org> wr= > ote: > > No. It's not just Ctrl+F4. There are several window management > > controls that are commonly used, and pages overriding these commands > > interfere with all of them. I think John's change covers this minimal > > set. > > > > -Ben > > > > On Tue, Sep 29, 2009 at 9:30 PM, mailto:=A0<suzhe@chromium.org> wrote: > >> Hi, > >> =A0I agree that we should handle some accelerator keys specially to deal= > with > >> a > >> hung/malicious renderer, just like what we've done in > >> RenderWidgetHostViewGtk > >> for popups. However, I still think such accelerator keys should be very = > few, > >> maybe only ctrl-f4. For all other key events I'd still prefer to send th= > em > >> to > >> renderer first, though some of them may still be processed by the browse= > r no > >> matter if it's suppressed by the renderer or not. > >> =A0IMHO, this issue is more complicated than what you though. =A0 =A0Fol= > lowing > >> aspects shall be considered carefully: > >> > >> 1. Supports different keyboard layout. Like my comment below, keyboard > >> layout > >> support is a mess, at least on Linux. We need handle it carefully, to ma= > ke > >> sure > >> our solution works on all if not most keyboard layouts. > >> =A0Actually, our views/gtk code branch doesn't support it for accelerato= > r keys > >> (We may address it separately). > >> > >> 2. In webkit, accesskeys are triggered by Char events rather than RawKey= > Down > >> events. It's actually the root cause of such conflict. To solve this > >> conflict, > >> we have following to different approaches: > >> > >> a) Only handles a key event as an accelerator if both the RawKeyDown and > >> corresponding Char events are not handled/suppressed by the renderer. > >> Firefox > >> follows this approach. > >> > >> b) Don't send corresponding Char event to the renderer if preceding > >> RawKeyDown > >> event was handled as an accelerator. Both IE and Safari follows this > >> approach. > >> > >> c) Don't send RawKeyDown/Char/KeyUp to the renderer at all if the RawKey= > Down > >> is > >> an accelerator. > >> > >> When involving IME, things get even more complicated, because a Char eve= > nt > >> might > >> be a fabricated event generated by IME, which may not have os_event at a= > ll, > >> or > >> have different keycode value. Though I'm not familiar with Windows > >> programming, > >> according to msdn, WM_CHAR message's wparam is a character code rather t= > han > >> a > >> virtual keycode. > >> So with your approach, it's hardly to block both RawKeyDown and > >> corresponding > >> Char event in RVH, which then may cause problem. > >> > >> > >> So I suggest following solution: > >> 1. Handle a (or maybe more) Tab close accelerator specially somewhere (m= > aybe > >> in > >> RWHV), so that hung/malicious renderer can be closed correctly. The spec= > ial > >> tab > >> close key must not be affected by keyboard layout. ctrl-F4 would be good= > . > >> > >> 2. Send all other key events to the renderer first. Then after receiving= > ACK > >> message from the renderer, deliver the key event along with the renderer= > 's > >> processing result (processed or not) to RenderViewHostDelegate to let it > >> decide > >> whether or not to handle this event as an accelerator. If > >> RenderViewHostDelegate > >> handled a RawKeyDown event as an accelerator, then we shall discard the > >> following Char event in RWH to prevent the renderer from handling it. > >> > >> In RenderViewHostDelegate and associated objects, because it knows if a > >> RawKeyDown event was handled by the renderer or not, it can decide wheth= > er > >> or > >> not to handle it as an accelerator at the very last stage. For example, = > for > >> gtk > >> port, the decision can be done in BrowserWindowGtk::OnGtkAccelerator(), = > so > >> that > >> we don't need to handle the keyboard layout mess by ourselves. > >> > >> Is my above expression clear enough? Please forgive my poor English. > >> > >> Regards > >> James Su > >> > >> > >> http://codereview.chromium.org/224023/diff/14043/9066 > >> File chrome/browser/gtk/browser_window_gtk.cc (right): > >> > >> http://codereview.chromium.org/224023/diff/14043/9066#newcode1170 > >> Line 1170: return command; > >> This piece of code may not correctly handle keyboard layouts other than > >> US. > >> In the normal key event handling path, gtk_window_activate_key() > >> function handles the keyboard layout mess for us (See > >> BrowserWindowGtk::HandleKeyboardEvent). > >> But if GetCommandId() gets called directly against a keyval, =A0it may n= > ot > >> work in non-US layout. > >> For example, in Russian keyboard kayout, ctrl-w generates keyval 0x06c3 > >> (GDK_Cyrillic_tse) rather than 0x77 (GDK_w), then this method won't > >> return IDC_CLOSE_TAB. > >> > >> http://codereview.chromium.org/224023 > >> > >
As I saw it, John's change passes a NativeWebKeyboardEvent to a platform-specific translation function that translates the native event into a command ID. It is up to the platform to define what keyboard shortcuts map to what command ID. How does this not work? -Ben On Tue, Sep 29, 2009 at 9:49 PM, <suzhe@chromium.org> wrote: > Hi, > =A0I agree to filter all Tab closing accelerators. The question is: How t= o do > the > job correctly. After reading my comments, you may notice that John's > approach > can't cover all situations, eg. the keyboard layout issue on Linux, and t= he > Char > event issue on all platforms. > =A0In a short word: If you want to filter a key event completely from the > browser > side, you must filter corresponding RawKeyDown and Char event altogether > (KeyUp > is probably harmless, but I'm not sure) and make it works correctly on al= l > keyboard layouts! > =A0Filtering Tab closing key would be easy, because the tab will be close= d > afterwards, you don't need to care about following Char event anymore. Bu= t > you > still need to handle the keyboard layout issue correctly. Otherwise, ctrl= -w > may > not work for a Russian user. > =A0But filtering other accelerator keys, like tab switching, is much more > complicated. Because the original tab is still there, users don't expect = the > tab > content to be changed when switching back. So both RawKeyDown and > corresponding > Char event must be filtered, as if there is no such event at all. > > Regards > James Su > > On 2009/09/30 04:38:43, Ben Goodger wrote: >> >> Let me be more specific: > >> Any accelerator that maps to a window management command should be >> filtered. We should not be in the business of deciding which keyboard >> shortcuts are filtered or not. You might use Ctrl+F4, but I use >> Ctrl+W. > >> If there are other bugs, separate those out from this point at least. > >> -Ben > >> On Tue, Sep 29, 2009 at 9:31 PM, Ben Goodger (Google) > > <mailto:ben@chromium.org> wr=3D >> >> ote: >> > No. It's not just Ctrl+F4. There are several window management >> > controls that are commonly used, and pages overriding these commands >> > interfere with all of them. I think John's change covers this minimal >> > set. >> > >> > -Ben >> > >> > On Tue, Sep 29, 2009 at 9:30 PM, mailto:=3DA0<suzhe@chromium.org> wrot= e: >> >> Hi, >> >> =3DA0I agree that we should handle some accelerator keys specially to >> >> deal=3D >> =A0with >> >> a >> >> hung/malicious renderer, just like what we've done in >> >> RenderWidgetHostViewGtk >> >> for popups. However, I still think such accelerator keys should be ve= ry >> >> =3D >> few, >> >> maybe only ctrl-f4. For all other key events I'd still prefer to send >> >> th=3D >> em >> >> to >> >> renderer first, though some of them may still be processed by the >> >> browse=3D >> r no >> >> matter if it's suppressed by the renderer or not. >> >> =3DA0IMHO, this issue is more complicated than what you though. =3DA0 >> >> =3DA0Fol=3D >> lowing >> >> aspects shall be considered carefully: >> >> >> >> 1. Supports different keyboard layout. Like my comment below, keyboar= d >> >> layout >> >> support is a mess, at least on Linux. We need handle it carefully, to >> >> ma=3D >> ke >> >> sure >> >> our solution works on all if not most keyboard layouts. >> >> =3DA0Actually, our views/gtk code branch doesn't support it for >> >> accelerato=3D >> r keys >> >> (We may address it separately). >> >> >> >> 2. In webkit, accesskeys are triggered by Char events rather than >> >> RawKey=3D >> Down >> >> events. It's actually the root cause of such conflict. To solve this >> >> conflict, >> >> we have following to different approaches: >> >> >> >> a) Only handles a key event as an accelerator if both the RawKeyDown >> >> and >> >> corresponding Char events are not handled/suppressed by the renderer. >> >> Firefox >> >> follows this approach. >> >> >> >> b) Don't send corresponding Char event to the renderer if preceding >> >> RawKeyDown >> >> event was handled as an accelerator. Both IE and Safari follows this >> >> approach. >> >> >> >> c) Don't send RawKeyDown/Char/KeyUp to the renderer at all if the >> >> RawKey=3D >> Down >> >> is >> >> an accelerator. >> >> >> >> When involving IME, things get even more complicated, because a Char >> >> eve=3D >> nt >> >> might >> >> be a fabricated event generated by IME, which may not have os_event a= t >> >> a=3D >> ll, >> >> or >> >> have different keycode value. Though I'm not familiar with Windows >> >> programming, >> >> according to msdn, WM_CHAR message's wparam is a character code rathe= r >> >> t=3D >> han >> >> a >> >> virtual keycode. >> >> So with your approach, it's hardly to block both RawKeyDown and >> >> corresponding >> >> Char event in RVH, which then may cause problem. >> >> >> >> >> >> So I suggest following solution: >> >> 1. Handle a (or maybe more) Tab close accelerator specially somewhere >> >> (m=3D >> aybe >> >> in >> >> RWHV), so that hung/malicious renderer can be closed correctly. The >> >> spec=3D >> ial >> >> tab >> >> close key must not be affected by keyboard layout. ctrl-F4 would be >> >> good=3D >> . >> >> >> >> 2. Send all other key events to the renderer first. Then after >> >> receiving=3D >> =A0ACK >> >> message from the renderer, deliver the key event along with the >> >> renderer=3D >> 's >> >> processing result (processed or not) to RenderViewHostDelegate to let >> >> it >> >> decide >> >> whether or not to handle this event as an accelerator. If >> >> RenderViewHostDelegate >> >> handled a RawKeyDown event as an accelerator, then we shall discard t= he >> >> following Char event in RWH to prevent the renderer from handling it. >> >> >> >> In RenderViewHostDelegate and associated objects, because it knows if= a >> >> RawKeyDown event was handled by the renderer or not, it can decide >> >> wheth=3D >> er >> >> or >> >> not to handle it as an accelerator at the very last stage. For exampl= e, >> >> =3D >> for >> >> gtk >> >> port, the decision can be done in BrowserWindowGtk::OnGtkAccelerator(= ), >> >> =3D >> so >> >> that >> >> we don't need to handle the keyboard layout mess by ourselves. >> >> >> >> Is my above expression clear enough? Please forgive my poor English. >> >> >> >> Regards >> >> James Su >> >> >> >> >> >> http://codereview.chromium.org/224023/diff/14043/9066 >> >> File chrome/browser/gtk/browser_window_gtk.cc (right): >> >> >> >> http://codereview.chromium.org/224023/diff/14043/9066#newcode1170 >> >> Line 1170: return command; >> >> This piece of code may not correctly handle keyboard layouts other th= an >> >> US. >> >> In the normal key event handling path, gtk_window_activate_key() >> >> function handles the keyboard layout mess for us (See >> >> BrowserWindowGtk::HandleKeyboardEvent). >> >> But if GetCommandId() gets called directly against a keyval, =3DA0it = may >> >> n=3D >> ot >> >> work in non-US layout. >> >> For example, in Russian keyboard kayout, ctrl-w generates keyval 0x06= c3 >> >> (GDK_Cyrillic_tse) rather than 0x77 (GDK_w), then this method won't >> >> return IDC_CLOSE_TAB. >> >> >> >> http://codereview.chromium.org/224023 >> >> >> > > > > > http://codereview.chromium.org/224023 >
On 2009/09/30 05:11:47, Ben Goodger wrote: > As I saw it, John's change passes a NativeWebKeyboardEvent to a > platform-specific translation function that translates the native > event into a command ID. It is up to the platform to define what > keyboard shortcuts map to what command ID. How does this not work? On Linux, we use keyval to define accelerators. But unfortunately, keyval is not keyboard layout neutral. But fortunately, if we use gtk_window_activate_key() method to activate an accelerator, gtk helps us workaround this problem by reversely mapping the keyval to hardware keycode and looking up the accelerator using hardware keycode, which is keyboard neutral. John's approach doesn't go down this path, so won't take advantage of this gtk feature. Regards James Su > > -Ben > > On Tue, Sep 29, 2009 at 9:49 PM, <mailto:suzhe@chromium.org> wrote: > > Hi, > > =A0I agree to filter all Tab closing accelerators. The question is: How t= > o do > > the > > job correctly. After reading my comments, you may notice that John's > > approach > > can't cover all situations, eg. the keyboard layout issue on Linux, and t= > he > > Char > > event issue on all platforms. > > =A0In a short word: If you want to filter a key event completely from the > > browser > > side, you must filter corresponding RawKeyDown and Char event altogether > > (KeyUp > > is probably harmless, but I'm not sure) and make it works correctly on al= > l > > keyboard layouts! > > =A0Filtering Tab closing key would be easy, because the tab will be close= > d > > afterwards, you don't need to care about following Char event anymore. Bu= > t > > you > > still need to handle the keyboard layout issue correctly. Otherwise, ctrl= > -w > > may > > not work for a Russian user. > > =A0But filtering other accelerator keys, like tab switching, is much more > > complicated. Because the original tab is still there, users don't expect = > the > > tab > > content to be changed when switching back. So both RawKeyDown and > > corresponding > > Char event must be filtered, as if there is no such event at all. > > > > Regards > > James Su > > > > On 2009/09/30 04:38:43, Ben Goodger wrote: > >> > >> Let me be more specific: > > > >> Any accelerator that maps to a window management command should be > >> filtered. We should not be in the business of deciding which keyboard > >> shortcuts are filtered or not. You might use Ctrl+F4, but I use > >> Ctrl+W. > > > >> If there are other bugs, separate those out from this point at least. > > > >> -Ben > > > >> On Tue, Sep 29, 2009 at 9:31 PM, Ben Goodger (Google) > > > > <mailto:ben@chromium.org> wr=3D > >> > >> ote: > >> > No. It's not just Ctrl+F4. There are several window management > >> > controls that are commonly used, and pages overriding these commands > >> > interfere with all of them. I think John's change covers this minimal > >> > set. > >> > > >> > -Ben > >> > > >> > On Tue, Sep 29, 2009 at 9:30 PM, mailto:=3DA0<suzhe@chromium.org> wrot= > e: > >> >> Hi, > >> >> =3DA0I agree that we should handle some accelerator keys specially to > >> >> deal=3D > >> =A0with > >> >> a > >> >> hung/malicious renderer, just like what we've done in > >> >> RenderWidgetHostViewGtk > >> >> for popups. However, I still think such accelerator keys should be ve= > ry > >> >> =3D > >> few, > >> >> maybe only ctrl-f4. For all other key events I'd still prefer to send > >> >> th=3D > >> em > >> >> to > >> >> renderer first, though some of them may still be processed by the > >> >> browse=3D > >> r no > >> >> matter if it's suppressed by the renderer or not. > >> >> =3DA0IMHO, this issue is more complicated than what you though. =3DA0 > >> >> =3DA0Fol=3D > >> lowing > >> >> aspects shall be considered carefully: > >> >> > >> >> 1. Supports different keyboard layout. Like my comment below, keyboar= > d > >> >> layout > >> >> support is a mess, at least on Linux. We need handle it carefully, to > >> >> ma=3D > >> ke > >> >> sure > >> >> our solution works on all if not most keyboard layouts. > >> >> =3DA0Actually, our views/gtk code branch doesn't support it for > >> >> accelerato=3D > >> r keys > >> >> (We may address it separately). > >> >> > >> >> 2. In webkit, accesskeys are triggered by Char events rather than > >> >> RawKey=3D > >> Down > >> >> events. It's actually the root cause of such conflict. To solve this > >> >> conflict, > >> >> we have following to different approaches: > >> >> > >> >> a) Only handles a key event as an accelerator if both the RawKeyDown > >> >> and > >> >> corresponding Char events are not handled/suppressed by the renderer. > >> >> Firefox > >> >> follows this approach. > >> >> > >> >> b) Don't send corresponding Char event to the renderer if preceding > >> >> RawKeyDown > >> >> event was handled as an accelerator. Both IE and Safari follows this > >> >> approach. > >> >> > >> >> c) Don't send RawKeyDown/Char/KeyUp to the renderer at all if the > >> >> RawKey=3D > >> Down > >> >> is > >> >> an accelerator. > >> >> > >> >> When involving IME, things get even more complicated, because a Char > >> >> eve=3D > >> nt > >> >> might > >> >> be a fabricated event generated by IME, which may not have os_event a= > t > >> >> a=3D > >> ll, > >> >> or > >> >> have different keycode value. Though I'm not familiar with Windows > >> >> programming, > >> >> according to msdn, WM_CHAR message's wparam is a character code rathe= > r > >> >> t=3D > >> han > >> >> a > >> >> virtual keycode. > >> >> So with your approach, it's hardly to block both RawKeyDown and > >> >> corresponding > >> >> Char event in RVH, which then may cause problem. > >> >> > >> >> > >> >> So I suggest following solution: > >> >> 1. Handle a (or maybe more) Tab close accelerator specially somewhere > >> >> (m=3D > >> aybe > >> >> in > >> >> RWHV), so that hung/malicious renderer can be closed correctly. The > >> >> spec=3D > >> ial > >> >> tab > >> >> close key must not be affected by keyboard layout. ctrl-F4 would be > >> >> good=3D > >> . > >> >> > >> >> 2. Send all other key events to the renderer first. Then after > >> >> receiving=3D > >> =A0ACK > >> >> message from the renderer, deliver the key event along with the > >> >> renderer=3D > >> 's > >> >> processing result (processed or not) to RenderViewHostDelegate to let > >> >> it > >> >> decide > >> >> whether or not to handle this event as an accelerator. If > >> >> RenderViewHostDelegate > >> >> handled a RawKeyDown event as an accelerator, then we shall discard t= > he > >> >> following Char event in RWH to prevent the renderer from handling it. > >> >> > >> >> In RenderViewHostDelegate and associated objects, because it knows if= > a > >> >> RawKeyDown event was handled by the renderer or not, it can decide > >> >> wheth=3D > >> er > >> >> or > >> >> not to handle it as an accelerator at the very last stage. For exampl= > e, > >> >> =3D > >> for > >> >> gtk > >> >> port, the decision can be done in BrowserWindowGtk::OnGtkAccelerator(= > ), > >> >> =3D > >> so > >> >> that > >> >> we don't need to handle the keyboard layout mess by ourselves. > >> >> > >> >> Is my above expression clear enough? Please forgive my poor English. > >> >> > >> >> Regards > >> >> James Su > >> >> > >> >> > >> >> http://codereview.chromium.org/224023/diff/14043/9066 > >> >> File chrome/browser/gtk/browser_window_gtk.cc (right): > >> >> > >> >> http://codereview.chromium.org/224023/diff/14043/9066#newcode1170 > >> >> Line 1170: return command; > >> >> This piece of code may not correctly handle keyboard layouts other th= > an > >> >> US. > >> >> In the normal key event handling path, gtk_window_activate_key() > >> >> function handles the keyboard layout mess for us (See > >> >> BrowserWindowGtk::HandleKeyboardEvent). > >> >> But if GetCommandId() gets called directly against a keyval, =3DA0it = > may > >> >> n=3D > >> ot > >> >> work in non-US layout. > >> >> For example, in Russian keyboard kayout, ctrl-w generates keyval 0x06= > c3 > >> >> (GDK_Cyrillic_tse) rather than 0x77 (GDK_w), then this method won't > >> >> return IDC_CLOSE_TAB. > >> >> > >> >> http://codereview.chromium.org/224023 > >> >> > >> > > > > > > > > > http://codereview.chromium.org/224023 > >
I don't know anything about these APIs so forgive me, but as I understand it the NativeWebKeyboardEvent has the exploded key data in it, can't you use that and your functions to generate a command ID? -Ben On Tue, Sep 29, 2009 at 10:22 PM, <suzhe@chromium.org> wrote: > On 2009/09/30 05:11:47, Ben Goodger wrote: >> >> As I saw it, John's change passes a NativeWebKeyboardEvent to a >> platform-specific translation function that translates the native >> event into a command ID. It is up to the platform to define what >> keyboard shortcuts map to what command ID. How does this not work? > > On Linux, we use keyval to define accelerators. But unfortunately, keyval= is > not > keyboard layout neutral. But fortunately, if we use > gtk_window_activate_key() > method to activate an accelerator, gtk helps us workaround this problem b= y > reversely mapping the keyval to hardware keycode and looking up the > accelerator > using hardware keycode, which is keyboard neutral. John's approach doesn'= t > go > down this path, so won't take advantage of this gtk feature. > > Regards > James Su > >> -Ben > >> On Tue, Sep 29, 2009 at 9:49 PM, =A0<mailto:suzhe@chromium.org> wrote: >> > Hi, >> > =3DA0I agree to filter all Tab closing accelerators. The question is: = How >> > t=3D >> o do >> > the >> > job correctly. After reading my comments, you may notice that John's >> > approach >> > can't cover all situations, eg. the keyboard layout issue on Linux, an= d >> > t=3D >> he >> > Char >> > event issue on all platforms. >> > =3DA0In a short word: If you want to filter a key event completely fro= m >> > the >> > browser >> > side, you must filter corresponding RawKeyDown and Char event altogeth= er >> > (KeyUp >> > is probably harmless, but I'm not sure) and make it works correctly on >> > al=3D >> l >> > keyboard layouts! >> > =3DA0Filtering Tab closing key would be easy, because the tab will be >> > close=3D >> d >> > afterwards, you don't need to care about following Char event anymore. >> > Bu=3D >> t >> > you >> > still need to handle the keyboard layout issue correctly. Otherwise, >> > ctrl=3D >> -w >> > may >> > not work for a Russian user. >> > =3DA0But filtering other accelerator keys, like tab switching, is much >> > more >> > complicated. Because the original tab is still there, users don't expe= ct >> > =3D >> the >> > tab >> > content to be changed when switching back. So both RawKeyDown and >> > corresponding >> > Char event must be filtered, as if there is no such event at all. >> > >> > Regards >> > James Su >> > >> > On 2009/09/30 04:38:43, Ben Goodger wrote: >> >> >> >> Let me be more specific: >> > >> >> Any accelerator that maps to a window management command should be >> >> filtered. We should not be in the business of deciding which keyboard >> >> shortcuts are filtered or not. You might use Ctrl+F4, but I use >> >> Ctrl+W. >> > >> >> If there are other bugs, separate those out from this point at least. >> > >> >> -Ben >> > >> >> On Tue, Sep 29, 2009 at 9:31 PM, Ben Goodger (Google) >> > >> > <mailto:ben@chromium.org> wr=3D3D >> >> >> >> ote: >> >> > No. It's not just Ctrl+F4. There are several window management >> >> > controls that are commonly used, and pages overriding these command= s >> >> > interfere with all of them. I think John's change covers this minim= al >> >> > set. >> >> > >> >> > -Ben >> >> > >> >> > On Tue, Sep 29, 2009 at 9:30 PM, mailto:=3D3DA0<suzhe@chromium.org> >> >> > wrot=3D >> e: >> >> >> Hi, >> >> >> =3D3DA0I agree that we should handle some accelerator keys special= ly >> >> >> to >> >> >> deal=3D3D >> >> =3DA0with >> >> >> a >> >> >> hung/malicious renderer, just like what we've done in >> >> >> RenderWidgetHostViewGtk >> >> >> for popups. However, I still think such accelerator keys should be >> >> >> ve=3D >> ry >> >> >> =3D3D >> >> few, >> >> >> maybe only ctrl-f4. For all other key events I'd still prefer to >> >> >> send >> >> >> th=3D3D >> >> em >> >> >> to >> >> >> renderer first, though some of them may still be processed by the >> >> >> browse=3D3D >> >> r no >> >> >> matter if it's suppressed by the renderer or not. >> >> >> =3D3DA0IMHO, this issue is more complicated than what you though. >> >> >> =3D3DA0 >> >> >> =3D3DA0Fol=3D3D >> >> lowing >> >> >> aspects shall be considered carefully: >> >> >> >> >> >> 1. Supports different keyboard layout. Like my comment below, >> >> >> keyboar=3D >> d >> >> >> layout >> >> >> support is a mess, at least on Linux. We need handle it carefully, >> >> >> to >> >> >> ma=3D3D >> >> ke >> >> >> sure >> >> >> our solution works on all if not most keyboard layouts. >> >> >> =3D3DA0Actually, our views/gtk code branch doesn't support it for >> >> >> accelerato=3D3D >> >> r keys >> >> >> (We may address it separately). >> >> >> >> >> >> 2. In webkit, accesskeys are triggered by Char events rather than >> >> >> RawKey=3D3D >> >> Down >> >> >> events. It's actually the root cause of such conflict. To solve th= is >> >> >> conflict, >> >> >> we have following to different approaches: >> >> >> >> >> >> a) Only handles a key event as an accelerator if both the RawKeyDo= wn >> >> >> and >> >> >> corresponding Char events are not handled/suppressed by the >> >> >> renderer. >> >> >> Firefox >> >> >> follows this approach. >> >> >> >> >> >> b) Don't send corresponding Char event to the renderer if precedin= g >> >> >> RawKeyDown >> >> >> event was handled as an accelerator. Both IE and Safari follows th= is >> >> >> approach. >> >> >> >> >> >> c) Don't send RawKeyDown/Char/KeyUp to the renderer at all if the >> >> >> RawKey=3D3D >> >> Down >> >> >> is >> >> >> an accelerator. >> >> >> >> >> >> When involving IME, things get even more complicated, because a Ch= ar >> >> >> eve=3D3D >> >> nt >> >> >> might >> >> >> be a fabricated event generated by IME, which may not have os_even= t >> >> >> a=3D >> t >> >> >> a=3D3D >> >> ll, >> >> >> or >> >> >> have different keycode value. Though I'm not familiar with Windows >> >> >> programming, >> >> >> according to msdn, WM_CHAR message's wparam is a character code >> >> >> rathe=3D >> r >> >> >> t=3D3D >> >> han >> >> >> a >> >> >> virtual keycode. >> >> >> So with your approach, it's hardly to block both RawKeyDown and >> >> >> corresponding >> >> >> Char event in RVH, which then may cause problem. >> >> >> >> >> >> >> >> >> So I suggest following solution: >> >> >> 1. Handle a (or maybe more) Tab close accelerator specially >> >> >> somewhere >> >> >> (m=3D3D >> >> aybe >> >> >> in >> >> >> RWHV), so that hung/malicious renderer can be closed correctly. Th= e >> >> >> spec=3D3D >> >> ial >> >> >> tab >> >> >> close key must not be affected by keyboard layout. ctrl-F4 would b= e >> >> >> good=3D3D >> >> . >> >> >> >> >> >> 2. Send all other key events to the renderer first. Then after >> >> >> receiving=3D3D >> >> =3DA0ACK >> >> >> message from the renderer, deliver the key event along with the >> >> >> renderer=3D3D >> >> 's >> >> >> processing result (processed or not) to RenderViewHostDelegate to >> >> >> let >> >> >> it >> >> >> decide >> >> >> whether or not to handle this event as an accelerator. If >> >> >> RenderViewHostDelegate >> >> >> handled a RawKeyDown event as an accelerator, then we shall discar= d >> >> >> t=3D >> he >> >> >> following Char event in RWH to prevent the renderer from handling >> >> >> it. >> >> >> >> >> >> In RenderViewHostDelegate and associated objects, because it knows >> >> >> if=3D >> =A0a >> >> >> RawKeyDown event was handled by the renderer or not, it can decide >> >> >> wheth=3D3D >> >> er >> >> >> or >> >> >> not to handle it as an accelerator at the very last stage. For >> >> >> exampl=3D >> e, >> >> >> =3D3D >> >> for >> >> >> gtk >> >> >> port, the decision can be done in >> >> >> BrowserWindowGtk::OnGtkAccelerator(=3D >> ), >> >> >> =3D3D >> >> so >> >> >> that >> >> >> we don't need to handle the keyboard layout mess by ourselves. >> >> >> >> >> >> Is my above expression clear enough? Please forgive my poor Englis= h. >> >> >> >> >> >> Regards >> >> >> James Su >> >> >> >> >> >> >> >> >> http://codereview.chromium.org/224023/diff/14043/9066 >> >> >> File chrome/browser/gtk/browser_window_gtk.cc (right): >> >> >> >> >> >> http://codereview.chromium.org/224023/diff/14043/9066#newcode1170 >> >> >> Line 1170: return command; >> >> >> This piece of code may not correctly handle keyboard layouts other >> >> >> th=3D >> an >> >> >> US. >> >> >> In the normal key event handling path, gtk_window_activate_key() >> >> >> function handles the keyboard layout mess for us (See >> >> >> BrowserWindowGtk::HandleKeyboardEvent). >> >> >> But if GetCommandId() gets called directly against a keyval, =3D3D= A0it >> >> >> =3D >> may >> >> >> n=3D3D >> >> ot >> >> >> work in non-US layout. >> >> >> For example, in Russian keyboard kayout, ctrl-w generates keyval >> >> >> 0x06=3D >> c3 >> >> >> (GDK_Cyrillic_tse) rather than 0x77 (GDK_w), then this method won'= t >> >> >> return IDC_CLOSE_TAB. >> >> >> >> >> >> http://codereview.chromium.org/224023 >> >> >> >> >> > >> > >> > >> > >> > http://codereview.chromium.org/224023 >> > > > > > http://codereview.chromium.org/224023 >
It's possible, but it's more complicated than going down current code path. However, it would be easy if we just want to handle a few known tab closing accelerators specially, because we can do it with an ad-hoc solution rather than a generic one. Besides the keyboard layout issue, John's approach can't filter out Char event correctly (this CL doesn't filter Char event at all). Regards James Su On 2009/09/30 05:25:13, Ben Goodger wrote: > I don't know anything about these APIs so forgive me, but as I > understand it the NativeWebKeyboardEvent has the exploded key data in > it, can't you use that and your functions to generate a command ID? > > -Ben >
Trying to respond to the slew of messages: estade re security, my point was that we consider a renderer to be low-privilege, and as such, we don't want it to prevent closing/navigating a tab. regarding sending the key to both renderer and browser: as has been mentioned a bunch of times, this is very confusing. what's the point of sending a key combination to a page if the user is closing/leaving it? there are already events that pages can use to determine when the tab is switched or closed. having a key combination do something that you don't see is confusing and non-intuitive. Only reserving it if IsCommandEnabled() is also confusing, why should the page get ctrl+tab if there's one tab but not if there are two? this makes testing/developing web apps more complicated/random. regarding doing only ctrl+f4: as others have noted, this is not enough. you might use ctrl+f4, while others might use ctrl+w. it's unintuitive and confusing to the user to have some close accelerators work in all pages but not others. also, as I mentioned earlier there's consensus that we need to prevent ctrl+tab from not working. IMO not doing this would be just like if Windows decided to allow applications to prevent alt+tab from working. regarding waiting for the renderer: I really think, and I think others have voiced their agreement, in that this is a no-no. we should not let a malicious/hung/corrupted app prevent closing of a tab or going away. regarding RawKeyDown and Char event, it's trivial to add this if needed, I'll look at the given test case when I get in On Tue, Sep 29, 2009 at 10:43 PM, <suzhe@chromium.org> wrote: > It's possible, but it's more complicated than going down current code path. > However, it would be easy if we just want to handle a few known tab closing > accelerators specially, because we can do it with an ad-hoc solution rather > than > a generic one. > > Besides the keyboard layout issue, John's approach can't filter out Char > event > correctly (this CL doesn't filter Char event at all). > > Regards > James Su > > On 2009/09/30 05:25:13, Ben Goodger wrote: > >> I don't know anything about these APIs so forgive me, but as I >> understand it the NativeWebKeyboardEvent has the exploded key data in >> it, can't you use that and your functions to generate a command ID? >> > > -Ben >> > > > > http://codereview.chromium.org/224023 >
also regarding keyboard layout, thanks for the heads up I wasn't aware of that. Doest that mean that BrowserWindowGtk::OnGtkAccelerator is broken too, or the passed in keyval already converted? I'm not familiar with gtk so I don't know how I can do the similar conversion that gtk_window_activate_key does. Anyone know, or is there an example of where we do this now? On Wed, Sep 30, 2009 at 10:07 AM, John Abd-El-Malek <jam@chromium.org>wrote: > Trying to respond to the slew of messages: > estade re security, my point was that we consider a renderer to be > low-privilege, and as such, we don't want it to prevent closing/navigating a > tab. > > regarding sending the key to both renderer and browser: as has been > mentioned a bunch of times, this is very confusing. what's the point of > sending a key combination to a page if the user is closing/leaving it? > there are already events that pages can use to determine when the tab is > switched or closed. having a key combination do something that you don't > see is confusing and non-intuitive. Only reserving it if IsCommandEnabled() > is also confusing, why should the page get ctrl+tab if there's one tab but > not if there are two? this makes testing/developing web apps more > complicated/random. > > regarding doing only ctrl+f4: as others have noted, this is not enough. > you might use ctrl+f4, while others might use ctrl+w. it's unintuitive and > confusing to the user to have some close accelerators work in all pages but > not others. also, as I mentioned earlier there's consensus that we need to > prevent ctrl+tab from not working. IMO not doing this would be just like if > Windows decided to allow applications to prevent alt+tab from working. > > regarding waiting for the renderer: I really think, and I think others have > voiced their agreement, in that this is a no-no. we should not let a > malicious/hung/corrupted app prevent closing of a tab or going away. > > regarding RawKeyDown and Char event, it's trivial to add this if needed, > I'll look at the given test case when I get in > > On Tue, Sep 29, 2009 at 10:43 PM, <suzhe@chromium.org> wrote: > >> It's possible, but it's more complicated than going down current code >> path. >> However, it would be easy if we just want to handle a few known tab >> closing >> accelerators specially, because we can do it with an ad-hoc solution >> rather than >> a generic one. >> >> Besides the keyboard layout issue, John's approach can't filter out Char >> event >> correctly (this CL doesn't filter Char event at all). >> >> Regards >> James Su >> >> On 2009/09/30 05:25:13, Ben Goodger wrote: >> >>> I don't know anything about these APIs so forgive me, but as I >>> understand it the NativeWebKeyboardEvent has the exploded key data in >>> it, can't you use that and your functions to generate a command ID? >>> >> >> -Ben >>> >> >> >> >> http://codereview.chromium.org/224023 >> > >
On 2009/09/30 17:07:30, John Abd-El-Malek wrote: > Trying to respond to the slew of messages: > estade re security, my point was that we consider a renderer to be > low-privilege, and as such, we don't want it to prevent closing/navigating a > tab. > > regarding sending the key to both renderer and browser: as has been > mentioned a bunch of times, this is very confusing. what's the point of > sending a key combination to a page if the user is closing/leaving it? > there are already events that pages can use to determine when the tab is > switched or closed. having a key combination do something that you don't > see is confusing and non-intuitive. Only reserving it if IsCommandEnabled() > is also confusing, why should the page get ctrl+tab if there's one tab but > not if there are two? this makes testing/developing web apps more > complicated/random. > > regarding doing only ctrl+f4: as others have noted, this is not enough. you > might use ctrl+f4, while others might use ctrl+w. it's unintuitive and > confusing to the user to have some close accelerators work in all pages but > not others. also, as I mentioned earlier there's consensus that we need to > prevent ctrl+tab from not working. IMO not doing this would be just like if > Windows decided to allow applications to prevent alt+tab from working. > > regarding waiting for the renderer: I really think, and I think others have > voiced their agreement, in that this is a no-no. we should not let a > malicious/hung/corrupted app prevent closing of a tab or going away. I don't think a malicious/hung/corrupted app can prevent closing of a tab or going away. shortcut keys is just one way to do it, others are mouse and hang monitor dialog. Anyway, I agree to make some window/tab management keys insuppressible, as long as we keep the list minimal to make sure we won't break existing web applications (at least popular ones). But we should do things right, eg. the Char event and keyboard layout issue. > > regarding RawKeyDown and Char event, it's trivial to add this if needed, > I'll look at the given test case when I get in > > On Tue, Sep 29, 2009 at 10:43 PM, <mailto:suzhe@chromium.org> wrote: > > > It's possible, but it's more complicated than going down current code path. > > However, it would be easy if we just want to handle a few known tab closing > > accelerators specially, because we can do it with an ad-hoc solution rather > > than > > a generic one. > > > > Besides the keyboard layout issue, John's approach can't filter out Char > > event > > correctly (this CL doesn't filter Char event at all). > > > > Regards > > James Su > > > > On 2009/09/30 05:25:13, Ben Goodger wrote: > > > >> I don't know anything about these APIs so forgive me, but as I > >> understand it the NativeWebKeyboardEvent has the exploded key data in > >> it, can't you use that and your functions to generate a command ID? > >> > > > > -Ben > >> > > > > > > > > http://codereview.chromium.org/224023 > > >
And if you have time, please have a look at the CL I just sent out for review: http://codereview.chromium.org/235039, which handles the Char event suppressing issue. Actually I'm thinking about a solution for this issue based on my CL: 1. Add a boolean parameter to RenderViewHostDelegate::View::HandleKeyboardEvent(), indicating if the event has been sent to the renderer or not. The name can be pre_handle or something similar. 2. Modify RWH and RVH's UnhandledKeyboardEvent to pass the pre_handle parameter. Or add a new method PrehandleKeyboardEvent to do that. 3. In RWH's ForwardKeyboardEvent method, before sending the key event to the renderer, sends it to the delegate with pre_handle = true. And then set suppress_next_char_events_ to true and discard this event, if it was handled by the delegate. Otherwise goes to the normal code path. After receiving ACK from the renderer, sends the key event to the delegate again with pre_handle = false, if the renderer did not handle it. 4. Then in the delegate's implementation (eg. TCVs), we can decide whether or not to handle the key event according to pre_handle's value in platform dependent way. For example in TabContentsViewGtk, we can just deliver pre_handle to BrowserWindowGtk along with the key event, then in BrowserWindowGtk, we go down the normal code path to handle the key by calling gtk_window_activate_key(). Then in OnGtkAccelerator method we can determine whether or not to execute the command according to pre_handle's value. For windows, we can use your CL's approach to do it in TabContentsViewWin. What's your opinion about this approach? Regards James Su
OnGtkAccelerator is ok. It's called by gtk_window_activate_key() function, which handles the keyboard layout mess for us. That's why I suggest to reuse this code path for those reserved accelerators. Regards James Su On 2009/09/30 17:28:13, John Abd-El-Malek wrote: > also regarding keyboard layout, thanks for the heads up I wasn't aware of > that. Doest that mean that BrowserWindowGtk::OnGtkAccelerator is broken > too, or the passed in keyval already converted? > I'm not familiar with gtk so I don't know how I can do the similar > conversion that gtk_window_activate_key does. Anyone know, or is there an > example of where we do this now? > > On Wed, Sep 30, 2009 at 10:07 AM, John Abd-El-Malek <jam@chromium.org>wrote: > > > Trying to respond to the slew of messages: > > estade re security, my point was that we consider a renderer to be > > low-privilege, and as such, we don't want it to prevent closing/navigating a > > tab. > > > > regarding sending the key to both renderer and browser: as has been > > mentioned a bunch of times, this is very confusing. what's the point of > > sending a key combination to a page if the user is closing/leaving it? > > there are already events that pages can use to determine when the tab is > > switched or closed. having a key combination do something that you don't > > see is confusing and non-intuitive. Only reserving it if IsCommandEnabled() > > is also confusing, why should the page get ctrl+tab if there's one tab but > > not if there are two? this makes testing/developing web apps more > > complicated/random. > > > > regarding doing only ctrl+f4: as others have noted, this is not enough. > > you might use ctrl+f4, while others might use ctrl+w. it's unintuitive and > > confusing to the user to have some close accelerators work in all pages but > > not others. also, as I mentioned earlier there's consensus that we need to > > prevent ctrl+tab from not working. IMO not doing this would be just like if > > Windows decided to allow applications to prevent alt+tab from working. > > > > regarding waiting for the renderer: I really think, and I think others have > > voiced their agreement, in that this is a no-no. we should not let a > > malicious/hung/corrupted app prevent closing of a tab or going away. > > > > regarding RawKeyDown and Char event, it's trivial to add this if needed, > > I'll look at the given test case when I get in > > > > On Tue, Sep 29, 2009 at 10:43 PM, <mailto:suzhe@chromium.org> wrote: > > > >> It's possible, but it's more complicated than going down current code > >> path. > >> However, it would be easy if we just want to handle a few known tab > >> closing > >> accelerators specially, because we can do it with an ad-hoc solution > >> rather than > >> a generic one. > >> > >> Besides the keyboard layout issue, John's approach can't filter out Char > >> event > >> correctly (this CL doesn't filter Char event at all). > >> > >> Regards > >> James Su > >> > >> On 2009/09/30 05:25:13, Ben Goodger wrote: > >> > >>> I don't know anything about these APIs so forgive me, but as I > >>> understand it the NativeWebKeyboardEvent has the exploded key data in > >>> it, can't you use that and your functions to generate a command ID? > >>> > >> > >> -Ben > >>> > >> > >> > >> > >> http://codereview.chromium.org/224023 > >> > > > > >
http://codereview.chromium.org/224023/diff/14043/9067 File chrome/browser/browser.cc (right): http://codereview.chromium.org/224023/diff/14043/9067#newcode2124 Line 2124: return command_id == IDC_CLOSE_TAB || perhaps we should include IDC_SELECT_TAB_*?
http://codereview.chromium.org/224023/diff/14043/9067 File chrome/browser/browser.cc (right): http://codereview.chromium.org/224023/diff/14043/9067#newcode2124 Line 2124: return command_id == IDC_CLOSE_TAB || On 2009/09/30 19:16:10, Evan Stade wrote: > perhaps we should include IDC_SELECT_TAB_*? SELECT_TAB_? are more risky than what's here now because there are web apps that are known to use these accelerators for other things (e.g. docs). It kind of drives me nuts when these don't work, but I think it's better to stick with this more focused set for now, and consider the more risky ones separately.
FYI, Spreadsheets overrides Ctrl+PageUp/Dn and it's enfuriating when tab switching suddenly stops working because the webapp decided it needed them. -Ben On Wed, Sep 30, 2009 at 12:21 PM, <brettw@chromium.org> wrote: > > http://codereview.chromium.org/224023/diff/14043/9067 > File chrome/browser/browser.cc (right): > > http://codereview.chromium.org/224023/diff/14043/9067#newcode2124 > Line 2124: return command_id == IDC_CLOSE_TAB || > On 2009/09/30 19:16:10, Evan Stade wrote: >> >> perhaps we should include IDC_SELECT_TAB_*? > > SELECT_TAB_? are more risky than what's here now because there are web > apps that are known to use these accelerators for other things (e.g. > docs). It kind of drives me nuts when these don't work, but I think it's > better to stick with this more focused set for now, and consider the > more risky ones separately. > > http://codereview.chromium.org/224023 >
regarding ctrl+pgup/ctrl+pgdown and ctrl+1..9: I too would prefer to do them, but given that Spreadsheets and Docs use them I'm inclined to not take them over and break those apps. regarding keyboard layouts: I tested my change on Linux with Russian keyboards, and ctrl+w worked fine regarding Char key: that was a mistake I had in my code, I fixed it by testing all keyboard events to check if they're accelerators, not just keydown (the fix removed the if check) regarding alt+1..9 on linux: the fix with my change is a few lines below to Browser::IsReservedAccelerator, which I verified works with the given test page that overrided alt+#. I put it in patchset 5 to test it on linux and took it out in patchset 6, since it's not related to this specific bug. #if defined(OS_LINUX) // On Linux we want to reserve alt+0...9. if (command_id >= IDC_SELECT_TAB_0 && command_id <= IDC_SELECT_LAST_TAB && event.modifiers == NativeWebKeyboardEvent::AltKey) { return true; } #endif On Wed, Sep 30, 2009 at 12:34 PM, Ben Goodger (Google) <ben@chromium.org>wrote: > FYI, Spreadsheets overrides Ctrl+PageUp/Dn and it's enfuriating when > tab switching suddenly stops working because the webapp decided it > needed them. > > -Ben > > On Wed, Sep 30, 2009 at 12:21 PM, <brettw@chromium.org> wrote: > > > > http://codereview.chromium.org/224023/diff/14043/9067 > > File chrome/browser/browser.cc (right): > > > > http://codereview.chromium.org/224023/diff/14043/9067#newcode2124 > > Line 2124: return command_id == IDC_CLOSE_TAB || > > On 2009/09/30 19:16:10, Evan Stade wrote: > >> > >> perhaps we should include IDC_SELECT_TAB_*? > > > > SELECT_TAB_? are more risky than what's here now because there are web > > apps that are known to use these accelerators for other things (e.g. > > docs). It kind of drives me nuts when these don't work, but I think it's > > better to stick with this more focused set for now, and consider the > > more risky ones separately. > > > > http://codereview.chromium.org/224023 > > >
On Wed, Sep 30, 2009 at 1:45 PM, John Abd-El-Malek <jam@chromium.org> wrote: > regarding ctrl+pgup/ctrl+pgdown and ctrl+1..9: I too would prefer to do > them, but given that Spreadsheets and Docs use them I'm inclined to not take > them over and break those apps. Clarity: your change actually will reserve ctrl-pgup/pgdn, since those are hooked to the same command IDs as the other "next/previous tab" accelerators. But I think this is the correct behavior. I also think it is correct to _not_ override ctrl-1 - ctrl-9. > http://codereview.chromium.org/224023 >> >
On Wed, Sep 30, 2009 at 1:49 PM, Peter Kasting <pkasting@chromium.org>wrote: > On Wed, Sep 30, 2009 at 1:45 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> regarding ctrl+pgup/ctrl+pgdown and ctrl+1..9: I too would prefer to do >> them, but given that Spreadsheets and Docs use them I'm inclined to not take >> them over and break those apps. > > > Clarity: your change actually will reserve ctrl-pgup/pgdn, since those are > hooked to the same command IDs as the other "next/previous tab" > accelerators. But I think this is the correct behavior. I also think it is > correct to _not_ override ctrl-1 - ctrl-9. > ah, you're right, I didn't realize that VK_NEXT/VK_PRIOR are page down and page up. I can block them directly, but I don't feel that strongly and it seems you and Ben do, so I'm fine with leaving as is. > > http://codereview.chromium.org/224023 >>> >>
LGTM. -Ben On 2009/09/30 21:04:00, John Abd-El-Malek wrote: > On Wed, Sep 30, 2009 at 1:49 PM, Peter Kasting <pkasting@chromium.org>wrote: > > > On Wed, Sep 30, 2009 at 1:45 PM, John Abd-El-Malek <jam@chromium.org>wrote: > > > >> regarding ctrl+pgup/ctrl+pgdown and ctrl+1..9: I too would prefer to do > >> them, but given that Spreadsheets and Docs use them I'm inclined to not take > >> them over and break those apps. > > > > > > Clarity: your change actually will reserve ctrl-pgup/pgdn, since those are > > hooked to the same command IDs as the other "next/previous tab" > > accelerators. But I think this is the correct behavior. I also think it is > > correct to _not_ override ctrl-1 - ctrl-9. > > > > ah, you're right, I didn't realize that VK_NEXT/VK_PRIOR are page down and > page up. I can block them directly, but I don't feel that strongly and it > seems you and Ben do, so I'm fine with leaving as is. > > > > > http://codereview.chromium.org/224023 > >>> > >> >
Are you really sure ctrl-w works fine? How about other keys and layouts? I can't see any code in your CL dealing with keyboard layout issue, how can it work? Please double check. Maybe we should consider to block these keys exactly by their key codes, rather than command ids. So that we can know exactly what keys are blocked. And we don't need to care about keyboard layout issue if we block them using their platform independent windowsKeyCode instead of native ones. Another thing to be confirmed: I'm not sure if Mac port can go with this approach, it might not be easy to implement GetCommandId() on Mac. Please find a Mac guru to help confirm. Regards James Su On 2009/09/30 20:45:35, John Abd-El-Malek wrote: > regarding ctrl+pgup/ctrl+pgdown and ctrl+1..9: I too would prefer to do > them, but given that Spreadsheets and Docs use them I'm inclined to not take > them over and break those apps. > regarding keyboard layouts: I tested my change on Linux with Russian > keyboards, and ctrl+w worked fine > regarding Char key: that was a mistake I had in my code, I fixed it by > testing all keyboard events to check if they're accelerators, not just > keydown (the fix removed the if check) > regarding alt+1..9 on linux: the fix with my change is a few lines below to > Browser::IsReservedAccelerator, which I verified works with the given test > page that overrided alt+#. I put it in patchset 5 to test it on linux and > took it out in patchset 6, since it's not related to this specific bug. > > #if defined(OS_LINUX) > // On Linux we want to reserve alt+0...9. > if (command_id >= IDC_SELECT_TAB_0 && > command_id <= IDC_SELECT_LAST_TAB && > event.modifiers == NativeWebKeyboardEvent::AltKey) { > return true; > } > #endif > > On Wed, Sep 30, 2009 at 12:34 PM, Ben Goodger (Google) <ben@chromium.org>wrote: > > > FYI, Spreadsheets overrides Ctrl+PageUp/Dn and it's enfuriating when > > tab switching suddenly stops working because the webapp decided it > > needed them. > > > > -Ben > > > > On Wed, Sep 30, 2009 at 12:21 PM, <mailto:brettw@chromium.org> wrote: > > > > > > http://codereview.chromium.org/224023/diff/14043/9067 > > > File chrome/browser/browser.cc (right): > > > > > > http://codereview.chromium.org/224023/diff/14043/9067#newcode2124 > > > Line 2124: return command_id == IDC_CLOSE_TAB || > > > On 2009/09/30 19:16:10, Evan Stade wrote: > > >> > > >> perhaps we should include IDC_SELECT_TAB_*? > > > > > > SELECT_TAB_? are more risky than what's here now because there are web > > > apps that are known to use these accelerators for other things (e.g. > > > docs). It kind of drives me nuts when these don't work, but I think it's > > > better to stick with this more focused set for now, and consider the > > > more risky ones separately. > > > > > > http://codereview.chromium.org/224023 > > > > > >
suzhe, thanks for worrying about the mac side. I think this is a viable approach. http://codereview.chromium.org/224023/diff/21011/21029 File chrome/browser/cocoa/browser_window_cocoa.mm (right): http://codereview.chromium.org/224023/diff/21011/21029#newcode325 Line 325: // CommandForKeyboardShortcut() doesn't have the full list. Yes, that's because in cocoa most keyboard shortcuts are bound to menu items which in turn are bound to "actions" (basically function pointers). suzhe: I think this is ok, we can walk the menu and for all menu items that have target commandDispatch: get the tag (which is the command id) and the accelerator and use that to implement this function. |