Bug 30660 - WebInspector: Can't use IME inside console
Summary: WebInspector: Can't use IME inside console
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-21 20:39 PDT by Keishi Hattori
Modified: 2009-10-24 21:57 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch (3.10 KB, patch)
2009-10-21 21:01 PDT, Keishi Hattori
pfeldman: review-
Details | Formatted Diff | Diff
proposed patch 2 (4.82 KB, patch)
2009-10-22 18:07 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
proposed patch 3 (4.96 KB, patch)
2009-10-24 20:22 PDT, Keishi Hattori
timothy: review-
Details | Formatted Diff | Diff
proposed patch 4 (4.93 KB, patch)
2009-10-24 20:45 PDT, Keishi Hattori
timothy: review-
Details | Formatted Diff | Diff
proposed patch 5 (5.07 KB, patch)
2009-10-24 21:39 PDT, Keishi Hattori
timothy: review+
timothy: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 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.
Comment 1 Keishi Hattori 2009-10-21 21:01:36 PDT
Created attachment 41635 [details]
proposed patch

event.keyCode will be 229 if it is from an IME.
Comment 2 Joseph Pecoraro 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.
Comment 3 Joseph Pecoraro 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)
Comment 4 Pavel Feldman 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?
Comment 5 Timothy Hatcher 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.
Comment 6 Alexey Proskuryakov 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.
Comment 7 Joseph Pecoraro 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
Comment 8 Keishi Hattori 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?
Comment 9 Timothy Hatcher 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.
Comment 10 Keishi Hattori 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.
Comment 11 Brian Weinstein 2009-10-24 20:33:47 PDT
Don't forget to add the newline at the end of utilities.js.
Comment 12 Timothy Hatcher 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.
Comment 13 Keishi Hattori 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.
Comment 14 Timothy Hatcher 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);
Comment 15 Keishi Hattori 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.
Comment 16 Timothy Hatcher 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.
Comment 17 Keishi Hattori 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
Comment 18 Keishi Hattori 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.
Comment 19 Timothy Hatcher 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.
Comment 20 Timothy Hatcher 2009-10-24 21:52:24 PDT
Comment on attachment 41813 [details]
proposed patch 5

I'll land and remove the listener.
Comment 21 Timothy Hatcher 2009-10-24 21:57:03 PDT
Committed r50039: <http://trac.webkit.org/changeset/50039>