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
Keishi Hattori
2009-10-21 20:39:50 PDT
Created attachment 41635 [details]
proposed patch
event.keyCode will be 229 if it is from an IME.
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. 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 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? (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. Could Web Inspector listen for keypress, not for keydown? The latter is very low level, as you discovered. (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 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 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.
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. Don't forget to add the newline at the end of utilities.js. 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. 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. (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); 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. (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. (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 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 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 on attachment 41813 [details]
proposed patch 5
I'll land and remove the listener.
Committed r50039: <http://trac.webkit.org/changeset/50039> |