Bug 30660 – WebInspector: Can't use IME inside console

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Keishi Hattori]

The console misinterprets an Enter inside an IME and executes the command when you're not finished typing.

[Keishi Hattori]

Created attachment 41635 [details]
proposed patch

event.keyCode will be 229 if it is from an IME.

[Joseph Pecoraro]

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]

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 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]

(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]

Could Web Inspector listen for keypress, not for keydown? The latter is very low level, as you discovered.

[Joseph Pecoraro]

(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]

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 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]

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]

Don't forget to add the newline at the end of utilities.js.

[Timothy Hatcher]

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]

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]

(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]

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]

(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]

(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]

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 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 on attachment 41813 [details]
proposed patch 5

I'll land and remove the listener.

[Timothy Hatcher]

Committed r50039: <http://trac.webkit.org/changeset/50039>