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
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.
style-queue ran check-webkit-style on attachment 44053 [details] without any errors.
Comment on attachment 44053 [details] [PATCH] Desired Behavior Do we have any way to write a test for this?
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.
(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?
Comment on attachment 44053 [details] [PATCH] Desired Behavior > + this.performSearch(event); We need this for cleanup, right?
(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.
(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.
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.
(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.
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.
style-queue ran check-webkit-style on attachment 44091 [details] without any errors.
Landed in http://trac.webkit.org/changeset/51553 r51553 = 02237f7508bc456dd797e280182c49e9a6a587ce