Bug 220773 – [Cocoa] Web Inspector: Console search box is broken, advancing to next result instead pastes from system find pasteboard

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

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."

"""

[Radar WebKit Bug Importer]

<rdar://problem/73410423>

[Razvan Caliman]

Created attachment 418313 [details]
Patch

[Razvan Caliman]

Created attachment 418315 [details]
Patch

[Devin Rousso]

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 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 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 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]

Created attachment 418400 [details]
Patch

[Devin Rousso]

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]

Created attachment 422711 [details]
Patch

Carry over R+; address review comments

[EWS]

Committed r274151: <https://commits.webkit.org/r274151>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422711 [details].