Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2805075:
[Mac]Handle edit commands from input methods correctly. (Closed)
Description[Mac]Handle edit commands from input methods correctly.
This CL does the following changes:
1.) It removes the EditCommandMatcher class and folds its behavior into RenderWidgetHostViewCocoa's -doCommandBySelector:. When -doCommandBySelector: is executed during execution of -keyDown: (the normal case), the commands are scheduled to be sent by -keyDown: later on, else they are sent immediately.
2.) If edit commands have been scheduled while IME composition is happening, delay delivery of the real key event (and eventual edit commands) until the fake key event(keycode 229) and IME composition have been sent to webkit. Safari sends the IME composition events first and deliver the key events after that, but according to HTML DOM Level3 Events Spec (http://www.w3.org/TR/DOM-Level-3-Events/#keyset-comp-input), the key events shall be sent before the IME composition events.
BUG=48161
I should press 'enter' twice to search korean keyword in the omnibox and web fields
BUG=48247
Pagedown/pageup inserts characters in textbox
TEST=See bug reports.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52741
Patch Set 1 #Patch Set 2 : Pass all edit commands to the webkit, except noop: #
Total comments: 20
Patch Set 3 : Update according to review feedback. #Patch Set 4 : git-try -b mac #
Messages
Total messages: 16 (0 generated)
Does ctrl-a / ctrl-e etc still work with this? I'm inclined to say that I won't LG any keyboard related CLs until we have unit tests for keyboard handling (which means your regressing CL has to be reverted until then).
On 2010/07/07 19:37:34, Nico wrote: > Does ctrl-a / ctrl-e etc still work with this? Yes. > > I'm inclined to say that I won't LG any keyboard related CLs until we have unit > tests for keyboard handling (which means your regressing CL has to be reverted > until then). I don't think we can implement the unit tests in near term, especially when involving input methods, which requires to have a mock input method built into the tests to simulate all possible situations. And actually bug 48247 is not a regression. My previous CL just revealed a hidden issue, which accidentally makes PageUp/Down work. The real cause of this issue is: we did not handle the edit commands generated by key down events without modifiers correctly. This CL handles them correctly. But I just found that on a Macbook Pro with OSX 10.5, fn-PageUp/Down generate scrollPageUp/Down commands rather than PageUp/Down commands. So I need to update this CL to handle these two new commands.
Hi, This CL has been updated to finally fix issue 48247. Regards James Su On 2010/07/07 20:05:00, James Su wrote: > On 2010/07/07 19:37:34, Nico wrote: > > Does ctrl-a / ctrl-e etc still work with this? > Yes. > > > > > I'm inclined to say that I won't LG any keyboard related CLs until we have > unit > > tests for keyboard handling (which means your regressing CL has to be reverted > > until then). > I don't think we can implement the unit tests in near term, especially when > involving input methods, which requires to have a mock input method built into > the tests to simulate all possible situations. > And actually bug 48247 is not a regression. My previous CL just revealed a > hidden issue, which accidentally makes PageUp/Down work. The real cause of this > issue is: we did not handle the edit commands generated by key down events > without modifiers correctly. This CL handles them correctly. > But I just found that on a Macbook Pro with OSX 10.5, fn-PageUp/Down generate > scrollPageUp/Down commands rather than PageUp/Down commands. So I need to update > this CL to handle these two new commands.
> > tests for keyboard handling (which means your regressing CL has to be reverted > > until then). > I don't think we can implement the unit tests in near term, especially when > involving input methods, which requires to have a mock input method built into > the tests to simulate all possible situations. This wasn't an IME regression. A test for pg down could be added to render_view_tests without a lot of effort. Please add test for at least pgup/down and some of ctrl-a/e/h/k/u/b/f > And actually bug 48247 is not a regression. From a user point of view, it was :-) (Don't get me wrong, I'm very very happy you're working on this keyboard stuff!) >My previous CL just revealed a > hidden issue, which accidentally makes PageUp/Down work. The real cause of this > issue is: we did not handle the edit commands generated by key down events > without modifiers correctly. This CL handles them correctly. > But I just found that on a Macbook Pro with OSX 10.5, fn-PageUp/Down generate > scrollPageUp/Down commands rather than PageUp/Down commands. So I need to update > this CL to handle these two new commands.
And please notice that: the page up/down behavior before revision 50622 was actually wrong, which acted like scrollPageUp/Down instead. The difference is: scrollPageUp/Down do not move input caret while PageUp/Down do. This CL fixes this issue as well. On 2010/07/07 22:12:38, James Su wrote: > Hi, > This CL has been updated to finally fix issue 48247. > > Regards > James Su > > On 2010/07/07 20:05:00, James Su wrote: > > On 2010/07/07 19:37:34, Nico wrote: > > > Does ctrl-a / ctrl-e etc still work with this? > > Yes. > > > > > > > > I'm inclined to say that I won't LG any keyboard related CLs until we have > > unit > > > tests for keyboard handling (which means your regressing CL has to be > reverted > > > until then). > > I don't think we can implement the unit tests in near term, especially when > > involving input methods, which requires to have a mock input method built into > > the tests to simulate all possible situations. > > And actually bug 48247 is not a regression. My previous CL just revealed a > > hidden issue, which accidentally makes PageUp/Down work. The real cause of > this > > issue is: we did not handle the edit commands generated by key down events > > without modifiers correctly. This CL handles them correctly. > > But I just found that on a Macbook Pro with OSX 10.5, fn-PageUp/Down generate > > scrollPageUp/Down commands rather than PageUp/Down commands. So I need to > update > > this CL to handle these two new commands.
On 2010/07/07 22:16:28, Nico wrote: > > > tests for keyboard handling (which means your regressing CL has to be > reverted > > > until then). > > I don't think we can implement the unit tests in near term, especially when > > involving input methods, which requires to have a mock input method built into > > the tests to simulate all possible situations. > > This wasn't an IME regression. A test for pg down could be added to > render_view_tests without a lot of effort. > > Please add test for at least pgup/down and some of ctrl-a/e/h/k/u/b/f It's an issue in renderer_host's input method related code rather than the renderer. So adding tests in render_view_Unittests has no help. > > > And actually bug 48247 is not a regression. > > From a user point of view, it was :-) > > (Don't get me wrong, I'm very very happy you're working on this keyboard stuff!) > > >My previous CL just revealed a > > hidden issue, which accidentally makes PageUp/Down work. The real cause of > this > > issue is: we did not handle the edit commands generated by key down events > > without modifiers correctly. This CL handles them correctly. > > But I just found that on a Macbook Pro with OSX 10.5, fn-PageUp/Down generate > > scrollPageUp/Down commands rather than PageUp/Down commands. So I need to > update > > this CL to handle these two new commands.
Just wondering when can I submit this CL, as the deadline is coming.
(I tried to patch this in to play with it and got merge conflicts. Maybe you can rebase? A few initial nits below. Sorry for taking my time :-/ ) http://codereview.chromium.org/2805075/diff/5001/6002 File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/2805075/diff/5001/6002#newcode973 chrome/browser/renderer_host/render_widget_host_view_mac.mm:973: // otherwise the new marked text might be cancelled by the webkit. s/the webkit/webkit/ http://codereview.chromium.org/2805075/diff/5001/6002#newcode1001 chrome/browser/renderer_host/render_widget_host_view_mac.mm:1001: ((hasMarkedText_ || oldHasMarkedText) ? 0U : 1U)) { s/U/u/ http://codereview.chromium.org/2805075/diff/5001/6002#newcode1030 chrome/browser/renderer_host/render_widget_host_view_mac.mm:1030: // windowsKeyCode == 0xE5 has already been sent to the webkit. s/the webkit/webkit/
There is a bad news of the ViewID CL: http://codereview.chromium.org/2973004 If you want to test all these pieces, please make sure to grab the latest patch of that CL. http://codereview.chromium.org/2805075/diff/5001/6002 File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/2805075/diff/5001/6002#newcode973 chrome/browser/renderer_host/render_widget_host_view_mac.mm:973: // otherwise the new marked text might be cancelled by the webkit. On 2010/07/15 06:00:15, Nico wrote: > s/the webkit/webkit/ Done. http://codereview.chromium.org/2805075/diff/5001/6002#newcode1001 chrome/browser/renderer_host/render_widget_host_view_mac.mm:1001: ((hasMarkedText_ || oldHasMarkedText) ? 0U : 1U)) { On 2010/07/15 06:00:15, Nico wrote: > s/U/u/ Done. http://codereview.chromium.org/2805075/diff/5001/6002#newcode1030 chrome/browser/renderer_host/render_widget_host_view_mac.mm:1030: // windowsKeyCode == 0xE5 has already been sent to the webkit. On 2010/07/15 06:00:15, Nico wrote: > s/the webkit/webkit/ Done.
Any update? On 2010/07/15 06:38:50, James Su wrote: > There is a bad news of the ViewID CL: http://codereview.chromium.org/2973004 > > If you want to test all these pieces, please make sure to grab the latest patch > of that CL. > > http://codereview.chromium.org/2805075/diff/5001/6002 > File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): > > http://codereview.chromium.org/2805075/diff/5001/6002#newcode973 > chrome/browser/renderer_host/render_widget_host_view_mac.mm:973: // otherwise > the new marked text might be cancelled by the webkit. > On 2010/07/15 06:00:15, Nico wrote: > > s/the webkit/webkit/ > > Done. > > http://codereview.chromium.org/2805075/diff/5001/6002#newcode1001 > chrome/browser/renderer_host/render_widget_host_view_mac.mm:1001: > ((hasMarkedText_ || oldHasMarkedText) ? 0U : 1U)) { > On 2010/07/15 06:00:15, Nico wrote: > > s/U/u/ > > Done. > > http://codereview.chromium.org/2805075/diff/5001/6002#newcode1030 > chrome/browser/renderer_host/render_widget_host_view_mac.mm:1030: // > windowsKeyCode == 0xE5 has already been sent to the webkit. > On 2010/07/15 06:00:15, Nico wrote: > > s/the webkit/webkit/ > > Done.
This time it patched in correctly. However, for some reason XCode wants to recompile everything (even though I did that during the day), which takes way over 1h on my MBP :-( Which means I will test this tomorrow morning. I will do this on the shuttle, so you should have the review by 8:30am or so. On Thu, Jul 15, 2010 at 7:35 PM, <suzhe@chromium.org> wrote: > Any update? > > On 2010/07/15 06:38:50, James Su wrote: >> >> There is a bad news of the ViewID CL: >> http://codereview.chromium.org/2973004 > >> If you want to test all these pieces, please make sure to grab the latest > > patch >> >> of that CL. > >> http://codereview.chromium.org/2805075/diff/5001/6002 >> File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): > >> http://codereview.chromium.org/2805075/diff/5001/6002#newcode973 >> chrome/browser/renderer_host/render_widget_host_view_mac.mm:973: // >> otherwise >> the new marked text might be cancelled by the webkit. >> On 2010/07/15 06:00:15, Nico wrote: >> > s/the webkit/webkit/ > >> Done. > >> http://codereview.chromium.org/2805075/diff/5001/6002#newcode1001 >> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1001: >> ((hasMarkedText_ || oldHasMarkedText) ? 0U : 1U)) { >> On 2010/07/15 06:00:15, Nico wrote: >> > s/U/u/ > >> Done. > >> http://codereview.chromium.org/2805075/diff/5001/6002#newcode1030 >> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1030: // >> windowsKeyCode == 0xE5 has already been sent to the webkit. >> On 2010/07/15 06:00:15, Nico wrote: >> > s/the webkit/webkit/ > >> Done. > > > > http://codereview.chromium.org/2805075/show >
LG This needs a better CL description that describes _what_ this changes. Something like: "This CL does the following changes: 1.) It replaces removes the EditCommandMatcher class and folds its behavior into -doCommandBySelector:. When -doCommandBySelector: is executed during execution of -keyDown: (the normal case), the commands are scheduled to be sent by -keyDown: later on, else they are sent immediately." (this is a great improvement, and is what fixes the page down part I believe?) "2.) If edit commands have been scheduled while IME composition is happening, delay delivery of key event (and eventual edit commands) until the IME composition has been sent to webkit." Also update the description to say why we can't always send the IME composition first and deliver the key after that. http://codereview.chromium.org/2805075/diff/5001/6001 File chrome/browser/renderer_host/render_widget_host_view_mac.h (right): http://codereview.chromium.org/2805075/diff/5001/6001#newcode114 chrome/browser/renderer_host/render_widget_host_view_mac.h:114: // Contains edit commands received by doCommandBySelector method when handling s/by doComm/by the -doComm/ http://codereview.chromium.org/2805075/diff/5001/6002 File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/2805075/diff/5001/6002#newcode954 chrome/browser/renderer_host/render_widget_host_view_mac.mm:954: BOOL sendEventLater = NO; maybe "delayEventUntilAfterImeCompostion"? a bit long tho. http://codereview.chromium.org/2805075/diff/5001/6002#newcode968 chrome/browser/renderer_host/render_widget_host_view_mac.mm:968: // If this key event was handled by the input method, but input method sent s/but input/but the input/ http://codereview.chromium.org/2805075/diff/5001/6002#newcode969 chrome/browser/renderer_host/render_widget_host_view_mac.mm:969: // some edit commands back to us, then in order to let the webkit handle s/the webkit/webkit/ Does "edit command" here mean edit command in the cocoa sense (i.e. an NSResponder call, the thing that happens when ctrl-a etc are pressed), or does it mean something else? Can you give an example when this happens in practice? …Oh, I think I understand now. I think a better formulation woud be "If this key event was handled by the input method, but -doCommandBySelector: (invoked by the call to -interpretKeyEvents: above) enqueued edit commands, then..." http://codereview.chromium.org/2805075/diff/5001/6002#newcode972 chrome/browser/renderer_host/render_widget_host_view_mac.mm:972: // We shouldn't do it if a new marked text was set by the input method, s/it/this/ http://codereview.chromium.org/2805075/diff/5001/6002#newcode1049 chrome/browser/renderer_host/render_widget_host_view_mac.mm:1049: // Only send corresponding key press event if there is no marked text. s/send/send a/
http://codereview.chromium.org/2805075/diff/5001/6002 File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/2805075/diff/5001/6002#newcode1045 chrome/browser/renderer_host/render_widget_host_view_mac.mm:1045: if (!renderWidgetHostView_->render_widget_host_) This check is not after every call to ForwardKeyboardEvent(). It looks like it's everywhere where it's necessary (ime compositions won't quit the renderer), so it's probably fine. Maybe you can put "// Not checking |renderWidgetHostView_->render_widget_host_| here because <reason/>" after all ForwardKeyboardEvent() calls that don't do this check?
Ahh, I input "git-try -b mac" into the patch set description. http://codereview.chromium.org/2805075/diff/5001/6001 File chrome/browser/renderer_host/render_widget_host_view_mac.h (right): http://codereview.chromium.org/2805075/diff/5001/6001#newcode114 chrome/browser/renderer_host/render_widget_host_view_mac.h:114: // Contains edit commands received by doCommandBySelector method when handling On 2010/07/16 15:44:05, Nico wrote: > s/by doComm/by the -doComm/ Done. http://codereview.chromium.org/2805075/diff/5001/6002 File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right): http://codereview.chromium.org/2805075/diff/5001/6002#newcode954 chrome/browser/renderer_host/render_widget_host_view_mac.mm:954: BOOL sendEventLater = NO; On 2010/07/16 15:44:05, Nico wrote: > maybe "delayEventUntilAfterImeCompostion"? a bit long tho. Done. http://codereview.chromium.org/2805075/diff/5001/6002#newcode968 chrome/browser/renderer_host/render_widget_host_view_mac.mm:968: // If this key event was handled by the input method, but input method sent On 2010/07/16 15:44:05, Nico wrote: > s/but input/but the input/ Done. http://codereview.chromium.org/2805075/diff/5001/6002#newcode969 chrome/browser/renderer_host/render_widget_host_view_mac.mm:969: // some edit commands back to us, then in order to let the webkit handle On 2010/07/16 15:44:05, Nico wrote: > s/the webkit/webkit/ > > Does "edit command" here mean edit command in the cocoa sense (i.e. an > NSResponder call, the thing that happens when ctrl-a etc are pressed), or does > it mean something else? > > Can you give an example when this happens in practice? > > …Oh, I think I understand now. I think a better formulation woud be > > "If this key event was handled by the input method, but -doCommandBySelector: > (invoked by the call to -interpretKeyEvents: above) enqueued edit commands, > then..." Done. http://codereview.chromium.org/2805075/diff/5001/6002#newcode972 chrome/browser/renderer_host/render_widget_host_view_mac.mm:972: // We shouldn't do it if a new marked text was set by the input method, On 2010/07/16 15:44:05, Nico wrote: > s/it/this/ Done. http://codereview.chromium.org/2805075/diff/5001/6002#newcode1045 chrome/browser/renderer_host/render_widget_host_view_mac.mm:1045: if (!renderWidgetHostView_->render_widget_host_) On 2010/07/16 15:46:45, Nico wrote: > This check is not after every call to ForwardKeyboardEvent(). It looks like it's > everywhere where it's necessary (ime compositions won't quit the renderer), so > it's probably fine. Maybe you can put "// Not checking > |renderWidgetHostView_->render_widget_host_| here because <reason/>" after all > ForwardKeyboardEvent() calls that don't do this check? Done. http://codereview.chromium.org/2805075/diff/5001/6002#newcode1049 chrome/browser/renderer_host/render_widget_host_view_mac.mm:1049: // Only send corresponding key press event if there is no marked text. On 2010/07/16 15:44:05, Nico wrote: > s/send/send a/ Done.
LG, thanks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
