WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(7.17 KB, patch)
2021-01-25 11:25 PST
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch
(3.04 KB, patch)
2021-01-26 05:10 PST
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Patch
(3.62 KB, patch)
2021-03-09 08:58 PST
,
Razvan Caliman
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-01-20 10:37:12 PST
<
rdar://problem/73410423
>
Razvan Caliman
Comment 2
2021-01-25 11:17:20 PST
Created
attachment 418313
[details]
Patch
Razvan Caliman
Comment 3
2021-01-25 11:25:20 PST
Created
attachment 418315
[details]
Patch
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
Created
attachment 418400
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug