Bug 220773

Summary:

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

Product:

WebKit

Reporter:

BJ Burg <bburg>

Component:

Web Inspector

Assignee:

Razvan Caliman <rcaliman>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, ews-watchlist, hi, inspector-bugzilla-changes, rcaliman, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Description BJ Burg 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."

"""

Comment 1 Radar WebKit Bug Importer 2021-01-20 10:37:12 PST

<rdar://problem/73410423>

Comment 2 Razvan Caliman 2021-01-25 11:17:20 PST

Created attachment 418313 [details]
Patch

Comment 3 Razvan Caliman 2021-01-25 11:25:20 PST

Created attachment 418315 [details]
Patch

Comment 4 Devin Rousso 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);
```

Comment 5 Razvan Caliman 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?

Comment 6 Razvan Caliman 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?

Comment 7 Devin Rousso 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".

Comment 8 Razvan Caliman 2021-01-26 05:10:16 PST

Created attachment 418400 [details]
Patch

Comment 9 Devin Rousso 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 😅

Comment 10 Razvan Caliman 2021-03-09 08:58:52 PST

Created attachment 422711 [details]
Patch

Carry over R+; address review comments

Comment 11 EWS 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].