Bug 32005 – Web Inspector: Escape key in the Search Field should be more User Friendly

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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]

style-queue ran check-webkit-style on attachment 44053 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 44053 [details]
[PATCH] Desired Behavior

Do we have any way to write a test for this?

[Timothy Hatcher]

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]

(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 on attachment 44053 [details]
[PATCH] Desired Behavior

> +        this.performSearch(event);

We need this for cleanup, right?

[Joseph Pecoraro]

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

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

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

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]

style-queue ran check-webkit-style on attachment 44091 [details] without any errors.

[Joseph Pecoraro]

Landed in http://trac.webkit.org/changeset/51553
r51553 = 02237f7508bc456dd797e280182c49e9a6a587ce