Bug 169804

Summary: Web Inspector: showing the Find banner doesn't immediately focus it
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, inspector-bugzilla-changes, joepeck, mattbaker, nvasilyev, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
mattbaker: review+, mattbaker: commit-queue-
Patch none

Description Devin Rousso 2017-03-16 18:41:34 PDT
.
Comment 1 Devin Rousso 2017-03-16 18:46:12 PDT
Created attachment 304737 [details]
Patch
Comment 2 Joseph Pecoraro 2017-03-17 11:32:46 PDT
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.
Comment 3 Joseph Pecoraro 2017-03-17 11:43:21 PDT
This regressed somewhere in this range:
https://trac.webkit.org/log/trunk/?mode=follow_copy&rev=213695&stop_rev=213686
Comment 4 Joseph Pecoraro 2017-03-17 11:47:06 PDT
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.
Comment 5 Joseph Pecoraro 2017-03-17 11:48:15 PDT
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.
Comment 6 Matt Baker 2017-03-20 13:29:37 PDT
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.
Comment 7 Joseph Pecoraro 2017-04-03 14:35:22 PDT
*** Bug 170372 has been marked as a duplicate of this bug. ***
Comment 8 Joseph Pecoraro 2017-04-03 14:36:28 PDT
<rdar://problem/31383181>
Comment 9 Devin Rousso 2017-04-03 14:50:26 PDT
Created attachment 306119 [details]
Patch
Comment 10 Matt Baker 2017-04-03 15:00:20 PDT
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.
Comment 11 Matt Baker 2017-04-03 17:07:35 PDT
Comment on attachment 306119 [details]
Patch

Nice fix! r=me with a minor nit.
Comment 12 Devin Rousso 2017-04-03 17:13:18 PDT
Created attachment 306144 [details]
Patch
Comment 13 WebKit Commit Bot 2017-04-03 17:41:22 PDT
Comment on attachment 306144 [details]
Patch

Clearing flags on attachment: 306144

Committed r214856: <http://trac.webkit.org/changeset/214856>
Comment 14 WebKit Commit Bot 2017-04-03 17:41:24 PDT
All reviewed patches have been landed.  Closing bug.