Bug 30660

Summary: WebInspector: Can't use IME inside console
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, aroben, bweinstein, joepeck, pfeldman, rik, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.5   
Attachments:
Description Flags
proposed patch
pfeldman: review-
proposed patch 2
none
proposed patch 3
timothy: review-
proposed patch 4
timothy: review-
proposed patch 5 timothy: review+, timothy: commit-queue-

Keishi Hattori
Reported 2009-10-21 20:39:50 PDT
The console misinterprets an Enter inside an IME and executes the command when you're not finished typing.
Attachments
proposed patch (3.10 KB, patch)
2009-10-21 21:01 PDT, Keishi Hattori
pfeldman: review-
proposed patch 2 (4.82 KB, patch)
2009-10-22 18:07 PDT, Keishi Hattori
no flags
proposed patch 3 (4.96 KB, patch)
2009-10-24 20:22 PDT, Keishi Hattori
timothy: review-
proposed patch 4 (4.93 KB, patch)
2009-10-24 20:45 PDT, Keishi Hattori
timothy: review-
proposed patch 5 (5.07 KB, patch)
2009-10-24 21:39 PDT, Keishi Hattori
timothy: review+
timothy: commit-queue-
Keishi Hattori
Comment 1 2009-10-21 21:01:36 PDT
Created attachment 41635 [details] proposed patch event.keyCode will be 229 if it is from an IME.
Joseph Pecoraro
Comment 2 2009-10-22 00:09:58 PDT
Hey keishi! Long time no see! Looks like you may missed have a recently added case in ElementsTreeOutline.js where pushing Enter starts editing the applicable part of the selected node: http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/ElementsTreeOutline.js?rev=49709#L202 Also I think it would be best if there was a comment on those lines that this handles an Enter inside an IME as it is not immediately obvious.
Joseph Pecoraro
Comment 3 2009-10-22 00:14:17 PDT
You forgot CCs on this one. I'll add them. You can bookmark these quick links to get some useful people CC'd: http://bit.ly/wi-bug or http://bit.ly/SFGz (the previous + Pavel and Brian)
Pavel Feldman
Comment 4 2009-10-22 07:37:09 PDT
Comment on attachment 41635 [details] proposed patch > + if (event.keyCode !== 229) { > + this._enterKeyPressed(event); 229 all over the place is a bit confusing. Maybe you could add something like function isEnterKey(event) { // Check if in IME. return event.keyCode !== 229 && event.keyIdentifier === "Enter"; } into the utilities.js?
Timothy Hatcher
Comment 5 2009-10-22 08:50:53 PDT
(In reply to comment #4) > (From update of attachment 41635 [details]) > > + if (event.keyCode !== 229) { > > + this._enterKeyPressed(event); > > 229 all over the place is a bit confusing. Maybe you could add something like > > function isEnterKey(event) { > // Check if in IME. > return event.keyCode !== 229 && event.keyIdentifier === "Enter"; > } > > into the utilities.js? Good idea. I agree.
Alexey Proskuryakov
Comment 6 2009-10-22 17:16:17 PDT
Could Web Inspector listen for keypress, not for keydown? The latter is very low level, as you discovered.
Joseph Pecoraro
Comment 7 2009-10-22 17:24:22 PDT
(In reply to comment #6) > Could Web Inspector listen for keypress, not for keydown? The latter is very > low level, as you discovered. I know that onkeypress does not handle the ESC key, but onkeydown does. This would matter for WebInspector.documentKeyDown which does watch for the ESC key. I posted a bug on this a while back: https://bugs.webkit.org/show_bug.cgi?id=25147 The test case on that bug would be applicable for testing some other keypress/keydown differences: https://bug-25147-attachments.webkit.org/attachment.cgi?id=29418
Keishi Hattori
Comment 8 2009-10-22 18:07:13 PDT
Created attachment 41707 [details] proposed patch 2 > http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/ElementsTreeOutline.js?rev=49709#L202 I don't think we need to check here because we can't be in IME. We only need to check if we want to detect an Enter key while the user is typing text. Turns out that keyup doesn't return keycode=229 and we can't tell if we are in an IME or not. So I moved performSearch() to searchKeyDown. The comment says that "Only call performSearch if the Enter key was pressed." but I think we are doing incremental search. Is this a regression? Or is the comment old?
Timothy Hatcher
Comment 9 2009-10-22 23:30:14 PDT
Comment on attachment 41707 [details] proposed patch 2 I worry about eliminating WebInspector.searchKeyUp. How did you test search? - // Select all of the text so the user can easily type an entirely new query. Why did you delete this? The code it applies to is still there.
Keishi Hattori
Comment 10 2009-10-24 20:22:15 PDT
Created attachment 41808 [details] proposed patch 3 > Why did you delete this? The code it applies to is still there. Sorry, added back the comments. I was misreading the comment. So incremental search *is* the desired behavior.
Brian Weinstein
Comment 11 2009-10-24 20:33:47 PDT
Don't forget to add the newline at the end of utilities.js.
Timothy Hatcher
Comment 12 2009-10-24 20:34:11 PDT
Comment on attachment 41808 [details] proposed patch 3 > - searchField.addEventListener("keydown", this.searchKeyDown.bind(this), false); > - searchField.addEventListener("keyup", this.searchKeyUp.bind(this), false); You only want to unregister keyup. > +function isEnterKey(event) { > + // Check if in IME. > + return event.keyCode !== 229 && event.keyIdentifier === "Enter"; > +} > \ No newline at end of file Add a new line.
Keishi Hattori
Comment 13 2009-10-24 20:45:29 PDT
Created attachment 41809 [details] proposed patch 4 > > - searchField.addEventListener("keydown", this.searchKeyDown.bind(this), false); > > - searchField.addEventListener("keyup", this.searchKeyUp.bind(this), false); > You only want to unregister keyup. I removed WebInspector.searchKeyUp because I moved performSearch to searchKeyDown. Do I need it? Patch just adds the newline.
Timothy Hatcher
Comment 14 2009-10-24 20:52:10 PDT
(In reply to comment #13) > Created an attachment (id=41809) [details] > proposed patch 4 > > > > - searchField.addEventListener("keydown", this.searchKeyDown.bind(this), false); > > > - searchField.addEventListener("keyup", this.searchKeyUp.bind(this), false); > > You only want to unregister keyup. > > I removed WebInspector.searchKeyUp because I moved performSearch to > searchKeyDown. Do I need it? > > Patch just adds the newline. Yes but you unregisted keydown, which still is needed. - searchField.addEventListener("keydown", this.searchKeyDown.bind(this), false);
Keishi Hattori
Comment 15 2009-10-24 21:03:17 PDT
WebInspector.documentKeyDown was calling WebInspector.searchKeyDown So when I registered here WebInspector.searchKeyDown was getting called twice. I just noticed I shouldn't have removed this comment either. Patch coming up. - // Call preventDefault since this was the Enter key. This prevents a "search" event - // from firing for key down. We handle the Enter key on key up in searchKeyUp. This - // stops performSearch from being called twice in a row.
Timothy Hatcher
Comment 16 2009-10-24 21:06:50 PDT
(In reply to comment #15) > WebInspector.documentKeyDown was calling WebInspector.searchKeyDown > So when I registered here WebInspector.searchKeyDown was getting called twice. > > > I just noticed I shouldn't have removed this comment either. Patch coming up. > - // Call preventDefault since this was the Enter key. This prevents a > "search" event > - // from firing for key down. We handle the Enter key on key up in > searchKeyUp. This > - // stops performSearch from being called twice in a row. I don't see WebInspector.documentKeyDown call ing searchKeyDown.
Keishi Hattori
Comment 17 2009-10-24 21:24:13 PDT
(In reply to comment #16) > I don't see WebInspector.documentKeyDown call ing searchKeyDown. Hmm… I put a breakpoint on WebInspector.searchKeyDown for ToT and type in to the search box and it is getting called twice. Stack trace shows it's getting called here http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/inspector.js#L597
Keishi Hattori
Comment 18 2009-10-24 21:39:07 PDT
Created attachment 41813 [details] proposed patch 5 Re-added for now to be safe. searchField.addEventListener("keydown", this.searchKeyDown.bind(this), false); Added comment + // Call preventDefault since this was the Enter key. This prevents a "search" event + // from firing for key down. This stops performSearch from being called twice in a row.
Timothy Hatcher
Comment 19 2009-10-24 21:46:55 PDT
Comment on attachment 41813 [details] proposed patch 5 You can remove searchField.addEventListener("keydown", this.searchKeyDown.bind(this), false); then! Sorry, I forgot about the documentKeyDown magic.
Timothy Hatcher
Comment 20 2009-10-24 21:52:24 PDT
Comment on attachment 41813 [details] proposed patch 5 I'll land and remove the listener.
Timothy Hatcher
Comment 21 2009-10-24 21:57:03 PDT
Note You need to log in before you can comment on or make changes to this bug.