RESOLVED FIXED 220773
[Cocoa] Web Inspector: Console search box is broken, advancing to next result instead pastes from system find pasteboard
https://bugs.webkit.org/show_bug.cgi?id=220773
Summary [Cocoa] Web Inspector: Console search box is broken, advancing to next result...
Blaze Burg
Reported 2021-01-20 10:36:56 PST
From a user report: """ I'm having trouble navigating to different instances of a word using ⌘F and ⌘G/⇧⌘G while inside of the console. Simple example/steps to reproduce without needing to set up any environment: Go to a random website. Here's a page that you can use: Safari Web Inspector Console Open the Safari Dev Tools using ⌘I. Navigate to the "Console" tab. Log an object by using console.dir(Object). Inside the console, use the shortcut ⌘F (find) or put your cursor inside the search box. Search for something. For example, I searched for "function". Try navigating to the next instance of the word using ⌘G or by clicking on the right arrow next to the search bar. After using ⌘G, you should notice that it locks up and does not navigate to the next instance of the word. In specific, it seems to replace part of the word that I'm searching for with a chunk of a word that I used from my last session. For example, when I search for the "function," it replaces the word with "loc" because in my previous session, I searched for "location." """
Attachments
Patch (6.62 KB, patch)
2021-01-25 11:17 PST, Razvan Caliman
no flags
Patch (7.17 KB, patch)
2021-01-25 11:25 PST, Razvan Caliman
no flags
Patch (3.04 KB, patch)
2021-01-26 05:10 PST, Razvan Caliman
no flags
Patch (3.62 KB, patch)
2021-03-09 08:58 PST, Razvan Caliman
no flags
Radar WebKit Bug Importer
Comment 1 2021-01-20 10:37:12 PST
Razvan Caliman
Comment 2 2021-01-25 11:17:20 PST
Razvan Caliman
Comment 3 2021-01-25 11:25:20 PST
Devin Rousso
Comment 4 2021-01-25 11:32:45 PST
Comment on attachment 418313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418313&action=review > Source/WebInspectorUI/UserInterface/Views/LogContentView.js:64 > this._findBanner = new WI.FindBanner(this, "console-find-banner", fixed); Instead of modifying `showing` to have all this other logic and modification sites, can we instead save `fixed` and use it inside `set targetElement` like so ``` - this._targetElement.classList.remove(WI.FindBanner.ShowingFindBannerStyleClassName); + this._targetElement.classList.toggle(WI.FindBanner.ShowingFindBannerStyleClassName, this._fixed); ```
Razvan Caliman
Comment 5 2021-01-25 12:22:43 PST
Comment on attachment 418313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418313&action=review >> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:64 >> this._findBanner = new WI.FindBanner(this, "console-find-banner", fixed); > > Instead of modifying `showing` to have all this other logic and modification sites, can we instead save `fixed` and use it inside `set targetElement` like so > ``` > - this._targetElement.classList.remove(WI.FindBanner.ShowingFindBannerStyleClassName); > + this._targetElement.classList.toggle(WI.FindBanner.ShowingFindBannerStyleClassName, this._fixed); > ``` It can work, but it still relies on checking style classes to determine the component's state. The `fixed` is not straightforward in what it implies. Rename to `alwaysShowing`? Also, we'd need to update either the element checked in the `showing` getter from this.element to this._targetElement (and change the class name) ``` get showing() { - return this.element.classList.contains(WI.FindBanner.ShowingStyleClassName); + return this._targetElement.classList.contains(WI.FindBanner.ShowingFindBannerStyleClassName); } ``` Or toggle the appropriate class on the other element in the `targetElement` setter. ``` if (this._targetElement) this._targetElement.classList.add(WI.FindBanner.SupportsFindBannerStyleClassName); + this.element.classList.toggle(WI.FindBanner.ShowingStyleClassName, this._fixed); ``` Any preference?
Razvan Caliman
Comment 6 2021-01-25 12:22:44 PST
Comment on attachment 418313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418313&action=review >> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:64 >> this._findBanner = new WI.FindBanner(this, "console-find-banner", fixed); > > Instead of modifying `showing` to have all this other logic and modification sites, can we instead save `fixed` and use it inside `set targetElement` like so > ``` > - this._targetElement.classList.remove(WI.FindBanner.ShowingFindBannerStyleClassName); > + this._targetElement.classList.toggle(WI.FindBanner.ShowingFindBannerStyleClassName, this._fixed); > ``` It can work, but it still relies on checking style classes to determine the component's state. The `fixed` is not straightforward in what it implies. Rename to `alwaysShowing`? Also, we'd need to update either the element checked in the `showing` getter from this.element to this._targetElement (and change the class name) ``` get showing() { - return this.element.classList.contains(WI.FindBanner.ShowingStyleClassName); + return this._targetElement.classList.contains(WI.FindBanner.ShowingFindBannerStyleClassName); } ``` Or toggle the appropriate class on the other element in the `targetElement` setter. ``` if (this._targetElement) this._targetElement.classList.add(WI.FindBanner.SupportsFindBannerStyleClassName); + this.element.classList.toggle(WI.FindBanner.ShowingStyleClassName, this._fixed); ``` Any preference?
Devin Rousso
Comment 7 2021-01-25 12:57:14 PST
Comment on attachment 418313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418313&action=review >>>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:64 >>>> this._findBanner = new WI.FindBanner(this, "console-find-banner", fixed); >>> >>> Instead of modifying `showing` to have all this other logic and modification sites, can we instead save `fixed` and use it inside `set targetElement` like so >>> ``` >>> - this._targetElement.classList.remove(WI.FindBanner.ShowingFindBannerStyleClassName); >>> + this._targetElement.classList.toggle(WI.FindBanner.ShowingFindBannerStyleClassName, this._fixed); >>> ``` >> >> It can work, but it still relies on checking style classes to determine the component's state. >> >> The `fixed` is not straightforward in what it implies. Rename to `alwaysShowing`? >> >> Also, we'd need to update either the element checked in the `showing` getter from this.element to this._targetElement (and change the class name) >> >> >> ``` >> get showing() >> { >> - return this.element.classList.contains(WI.FindBanner.ShowingStyleClassName); >> + return this._targetElement.classList.contains(WI.FindBanner.ShowingFindBannerStyleClassName); >> } >> ``` >> >> Or toggle the appropriate class on the other element in the `targetElement` setter. >> >> >> ``` >> if (this._targetElement) >> this._targetElement.classList.add(WI.FindBanner.SupportsFindBannerStyleClassName); >> >> + this.element.classList.toggle(WI.FindBanner.ShowingStyleClassName, this._fixed); >> ``` >> >> Any preference? > > It can work, but it still relies on checking style classes to determine the component's state. > > The `fixed` is not straightforward in what it implies. Rename to `alwaysShowing`? > > Also, we'd need to update either the element checked in the `showing` getter from this.element to this._targetElement (and change the class name) > > > ``` > get showing() > { > - return this.element.classList.contains(WI.FindBanner.ShowingStyleClassName); > + return this._targetElement.classList.contains(WI.FindBanner.ShowingFindBannerStyleClassName); > } > ``` > > Or toggle the appropriate class on the other element in the `targetElement` setter. > > > ``` > if (this._targetElement) > this._targetElement.classList.add(WI.FindBanner.SupportsFindBannerStyleClassName); > > + this.element.classList.toggle(WI.FindBanner.ShowingStyleClassName, this._fixed); > ``` > > Any preference? Oh oops sorry copypasted the wrong code 😅. Yeah it should use `this.element` and `WI.FindBanner.ShowingStyleClassName` (but still in `set targetElement()`). Also, you'd probably want to prevent `WI.FindBanner.ShowingStyleClassName` from being removed in `hide()` if `this._alwaysShowing`. Renaming to `alwaysShowing` sounds fine. I agree that keeping state in the DOM is not ideal, but I personally prefer keeping patches small and focused so as to make scanning `git log` more useful and make rollouts less problematic. As such, I'd rather fix the issue as described in this bug and then have a separate bug to "move `WI.FindBanner` state out of the DOM".
Razvan Caliman
Comment 8 2021-01-26 05:10:16 PST
Devin Rousso
Comment 9 2021-01-26 10:01:06 PST
Comment on attachment 418400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418400&action=review r=me :) > Source/WebInspectorUI/UserInterface/Views/FindBanner.js:153 > + this.element.classList.toggle(WI.FindBanner.ShowingStyleClassName, this._alwaysShowing); NIT: it's possible/likely that we'd remove and then re-add `WI.FindBanner.ShowingStyleClassName` inside a single call to `set targetElement`. I think it may be simpler to guard the `this.element.classList.remove(WI.FindBanner.ShowingStyleClassName);` above inside an `if (!this._alwaysShowing)` (like you have in `hide()`) and then add `this.element.classList.add(WI.FindBanner.ShowingStyleClassName);` inside the constructor in the `if (this._alwaysShowing)`. I'd originally suggested `set targetElement` because I'd incorrectly thought that we were adding/removing this class from `this._targetElement`, so my apologies if I lead you astray 😅
Razvan Caliman
Comment 10 2021-03-09 08:58:52 PST
Created attachment 422711 [details] Patch Carry over R+; address review comments
EWS
Comment 11 2021-03-09 09:35:37 PST
Committed r274151: <https://commits.webkit.org/r274151> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422711 [details].
Note You need to log in before you can comment on or make changes to this bug.