Summary: | Web Inspector: showing the Find banner doesn't immediately focus it | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||
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
Devin Rousso
2017-03-16 18:41:34 PDT
Created attachment 304737 [details]
Patch
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. This regressed somewhere in this range: https://trac.webkit.org/log/trunk/?mode=follow_copy&rev=213695&stop_rev=213686 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 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 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.
*** Bug 170372 has been marked as a duplicate of this bug. *** Created attachment 306119 [details]
Patch
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 on attachment 306119 [details]
Patch
Nice fix! r=me with a minor nit.
Created attachment 306144 [details]
Patch
Comment on attachment 306144 [details] Patch Clearing flags on attachment: 306144 Committed r214856: <http://trac.webkit.org/changeset/214856> All reviewed patches have been landed. Closing bug. |