Bug 169804 – Web Inspector: showing the Find banner doesn't immediately focus it

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

Created attachment 304737 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 304737 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304737&action=review

> Source/WebInspectorUI/ChangeLog:11
> +        If the FindBanner was now showing before, focus it before selecting.

Typo: "now" => "not"

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:207
>              this._inputField.select();

Select should focus, right?! Seems this could be a WebCore regression if that is not the case.

[Joseph Pecoraro]

This regressed somewhere in this range:
https://trac.webkit.org/log/trunk/?mode=follow_copy&rev=213695&stop_rev=213686

[Joseph Pecoraro]

This sounds super suspicious:
https://trac.webkit.org/changeset/213689/trunk

The ChangeLog says:

    This subtly changed the behavior of WebInspector._focusChanged,
    which expected all editable fields to be backed by a CodeMirror instance.

I don't believe that statement is true.

In any case, I don't know which is the better place to fix this. I feel like if we take a fix like yours we will need to do this in a lot of places.

[Joseph Pecoraro]

Comment on attachment 304737 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304737&action=review

>> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:207
>>              this._inputField.select();
> 
> Select should focus, right?! Seems this could be a WebCore regression if that is not the case.

Further, since we expect select() to focus(), then we can just always call focus() regardless of if we were showing or not. The keyboard shortcut should always focus the field.

[Matt Baker]

Comment on attachment 304737 [details]
Patch

r-, as the fix doesn't seem to address the root cause of the regression (the suspicious patch Joe mentioned). The fix should probably be in WebInspector._focusChanged.

[Joseph Pecoraro]

*** Bug 170372 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

<rdar://problem/31383181>

[Devin Rousso]

Created attachment 306119 [details]
Patch

[Matt Baker]

Comment on attachment 306119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306119&action=review

> Source/WebInspectorUI/UserInterface/Base/Main.js:1341
> +        // will also get run when WebInspector.startEditing is called on an element.  We do not want

Nit: use single-space after period.

[Matt Baker]

Comment on attachment 306119 [details]
Patch

Nice fix! r=me with a minor nit.

[Devin Rousso]

Created attachment 306144 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 306144 [details]
Patch

Clearing flags on attachment: 306144

Committed r214856: <http://trac.webkit.org/changeset/214856>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.