RESOLVED FIXED 32005
Web Inspector: Escape key in the Search Field should be more User Friendly
https://bugs.webkit.org/show_bug.cgi?id=32005
Summary Web Inspector: Escape key in the Search Field should be more User Friendly
Joseph Pecoraro
Reported 2009-11-30 20:51:59 PST
The Escape Key in the Search Field should do the following: - When manually focused (via the mouse) Escape should Clear the text and search but remain focused on the textfield. - Otherwise (⌘F) Escape should Clear the text and search but change focus to the previously selected element
Attachments
[PATCH] Desired Behavior (2.20 KB, patch)
2009-11-30 22:12 PST, Joseph Pecoraro
pfeldman: review+
[PATCH] Desired Behavior (2.33 KB, patch)
2009-12-01 11:40 PST, Joseph Pecoraro
timothy: review+
Joseph Pecoraro
Comment 1 2009-11-30 22:12:26 PST
Created attachment 44053 [details] [PATCH] Desired Behavior previousFocusElement doesn't have a setter, and since this was the only special case I didn't think it pertinent to add a setter. I also named the function as descriptively as possible rather then searchOnMouseDown or something like we normally do, again my reasoning being this is a special case and the name is helpful.
WebKit Review Bot
Comment 2 2009-11-30 22:12:55 PST
style-queue ran check-webkit-style on attachment 44053 [details] without any errors.
Eric Seidel (no email)
Comment 3 2009-11-30 22:20:30 PST
Comment on attachment 44053 [details] [PATCH] Desired Behavior Do we have any way to write a test for this?
Timothy Hatcher
Comment 4 2009-12-01 03:25:48 PST
Comment on attachment 44053 [details] [PATCH] Desired Behavior > + searchField.addEventListener("mousedown", this.searchFieldManualFocus.bind(this), false); // when the search field is manually selected > +WebInspector.searchFieldManualFocus = function(event) > +{ > + this.currentFocusElement = event.target; > + this._previousFocusElement = event.target; > +} This should alredy happen when the focus event fires. You shouldn't need this.
Joseph Pecoraro
Comment 5 2009-12-01 08:04:50 PST
(In reply to comment #4) > (From update of attachment 44053 [details]) > > + searchField.addEventListener("mousedown", this.searchFieldManualFocus.bind(this), false); // when the search field is manually selected > > +WebInspector.searchFieldManualFocus = function(event) > > +{ > > + this.currentFocusElement = event.target; > > + this._previousFocusElement = event.target; > > +} > > This should alredy happen when the focus event fires. You shouldn't need this. My goal was to make it so when you manually click on the search field, pushing Escape would keep the focus on the search field. I didn't want to create and delete a flag because that is normally dirty. An easy solution is to set both the previous and current to be the searchField. In this case the searchFieldManualFocus focus event will fire before the other due to the bubbling order. Therefore if I set: - just the previous, then when this bubbles out to the main handler then previous will get overwritten with the "old" current. - just the current, then the previous does not get changed My solution was to just set them both, to be sure they had the same value. Should I attack this in a different way?
Pavel Feldman
Comment 6 2009-12-01 08:06:04 PST
Comment on attachment 44053 [details] [PATCH] Desired Behavior > + this.performSearch(event); We need this for cleanup, right?
Joseph Pecoraro
Comment 7 2009-12-01 08:13:29 PST
(In reply to comment #3) > (From update of attachment 44053 [details]) > Do we have any way to write a test for this? This is mostly a UI feature. Testing it would involve "fake" events or calling frontend functions in a particular order assuming they were fired. We typically haven't included tests for these types of inspector features but if desired I could add a test.
Joseph Pecoraro
Comment 8 2009-12-01 08:15:14 PST
(In reply to comment #6) > (From update of attachment 44053 [details]) > > + this.performSearch(event); > > We need this for cleanup, right? Yes. Due to having just set the text to the empty string, AND in such a way the that the inputs incremental search event doesn't automatically fire, I call performSearch which would clear any existing search.
Timothy Hatcher
Comment 9 2009-12-01 09:18:09 PST
Comment on attachment 44053 [details] [PATCH] Desired Behavior > + this.currentFocusElement = this.previousFocusElement; Why do you need this? Wont the focus stay on the search field like you want if you removed this line? Then you don't need the mousedown listener.
Joseph Pecoraro
Comment 10 2009-12-01 09:26:26 PST
(In reply to comment #9) > (From update of attachment 44053 [details]) > > > + this.currentFocusElement = this.previousFocusElement; > > Why do you need this? Wont the focus stay on the search field like you want if > you removed this line? Then you don't need the mousedown listener. This is for behaviorcase #2, in comment 1. For instance if you had selected a node in the Tree HHierarchy, pushed ⌘F, cancelled with Escape, it would go back to the element you had selected. This is similar to using ⌘F in Safari itself if you start the process with the focus on a text field.
Joseph Pecoraro
Comment 11 2009-12-01 11:40:22 PST
Created attachment 44091 [details] [PATCH] Desired Behavior Although I was pretty certain it was working before after a rebuild in behavior #1 pushing Escape made it so the search field had focus but I couldn't start typing in it! Throwing in a select() made it work as expected.
WebKit Review Bot
Comment 12 2009-12-01 11:41:54 PST
style-queue ran check-webkit-style on attachment 44091 [details] without any errors.
Joseph Pecoraro
Comment 13 2009-12-01 12:09:00 PST
Landed in http://trac.webkit.org/changeset/51553 r51553 = 02237f7508bc456dd797e280182c49e9a6a587ce
Note You need to log in before you can comment on or make changes to this bug.