|Summary:||Web Inspector: showing the Find banner doesn't immediately focus it|
|Product:||WebKit||Reporter:||Devin Rousso <drousso>|
|Component:||Web Inspector||Assignee:||Devin Rousso <drousso>|
|Severity:||Normal||CC:||commit-queue, inspector-bugzilla-changes, joepeck, mattbaker, nvasilyev, webkit-bug-importer|
Description Devin Rousso 2017-03-16 18:41:34 PDT
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 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 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.