Bug 192527 – Web Inspector: provide a way to make searches case sensitive or use a regular expression

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

<https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js#L96>

[Devin Rousso]

Created attachment 356875 [details]
Patch

[Devin Rousso]

Created attachment 356876 [details]
[Image] After Patch is applied (Search Tab)

[Devin Rousso]

Created attachment 356877 [details]
[Image] After Patch is applied (Settings Tab)

[Joseph Pecoraro]

Comment on attachment 356875 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356875&action=review

Awesome! r- for now because we should have tests for the regex / case-sensitive functionality, which I'm sure we don't have tests for right now.

> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:37
> +        this._filtersSetting = new WI.Setting(identifier + "-navigation-sidebar-filters", {});
> +        this._filterSearchSettings = WI.SearchUtilities.createSettings("navigation-sidebar", {
> +            handleChanged: (event) => {
> +                this.updateFilter();
> +            },
> +        });

I feel like filter bars should be very basic always case insensitive and straightforward. I don't really have a reasoning for this though, but it would reduce the number of settings gears all over the place. So I think we shouldn't add them to NavigationSidebarPanel filter fields.

> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:34
> +                this.focusSearchField(true);

Nice!

[Matt Baker]

Instead of using the settings gear outside of the search field, I think we should add buttons inside the field, right aligned. "Aa" and ".*" are my suggestions for the icons.

[Joseph Pecoraro]

(In reply to Matt Baker from comment #5)
> Instead of using the settings gear outside of the search field, I think we
> should add buttons inside the field, right aligned. "Aa" and ".*" are my
> suggestions for the icons.

I don't find "Aa" clear at all. I find it super confusing in Xcode.

[Devin Rousso]

(In reply to Matt Baker from comment #5)
> Instead of using the settings gear outside of the search field, I think we should add buttons inside the field, right aligned. "Aa" and ".*" are my suggestions for the icons.
I don't think we have enough room for that many buttons.  When sidebars are narrow, we're already constrained horizontally, and this will make it that much worse for any text inside the filter bars.

I also agree with @Joe in comment #6.  I'd rather have a single list of options I can enable/disable, that way everything is one one place.

[Radar WebKit Bug Importer]

<rdar://problem/46800955>

[Devin Rousso]

Created attachment 358180 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 358180 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358180&action=review

> Source/WebInspectorUI/ChangeLog:8
> +        * UserInterface/Base/SearchUtilities.js: Added.

r-, this patch doesn't actually include SearchUtilities.js

[Devin Rousso]

Created attachment 358575 [details]
Patch

webkit-patch strikes again :(

[Joseph Pecoraro]

Comment on attachment 358575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358575&action=review

r-, but this patch feels great! I love the quick settings cog in the Search tab.

Here are some issues. Address the Regex one and I think this is worth landing!

• Regex Search does not highlight the expected range. Search for "\d+" which may match "foo0foo" and will highlight "foo|0fo|o" because the search term length was 3 characters, but its a regex not a literal match!
• Case-sensitive search does not work in the DOM Search. A search for "test" finds a text node "Tools / Tests" which it should not. That could  either be addressed now or in a follow-up. Seems this would need to modify `DOMAgent.performSearch`.

I wonder if we should include a color'd UI for regex searches to at least distinguish it easier, and immediately know you're typing a regex and not a literal string. If I saw the same dark orange as in the quick console when I type a regex it would immediately be clear to me that I'm performing a regex search.

> LayoutTests/inspector/debugger/search-scripts-expected.txt:50
> +QUERY: (search|OTHER)TEST {"caseSensitive":true,"isRegex":true}
> +SCRIPT: LayoutTests/inspector/debugger/search-scripts.html
> +RESULTS: 0

Looks like this should have matched the line with "OTHERTEST" in this file.

> LayoutTests/inspector/page/searchInResources.html:132
> +                InspectorTest.expectThat(matches.length === result.matchesCount, "Should find previously mentioned number of matches.");

The "previously mentioned number of matches" makes the output hard to read. I'd prefer to see:

    `Should find ${result.matchesCount} match(es).`

Then you'd immediately know from looking at the text file that 2 MATCH lines is wrong if you expected 3 matches.

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #12)
> • Regex Search does not highlight the expected range. Search for "\d+" which
> may match "foo0foo" and will highlight "foo|0fo|o" because the search term
> length was 3 characters, but its a regex not a literal match!
Yikes!  Will investigate.

> • Case-sensitive search does not work in the DOM Search. A search for "test"
> finds a text node "Tools / Tests" which it should not. That could  either be
> addressed now or in a follow-up. Seems this would need to modify
> `DOMAgent.performSearch`.
<https://webkit.org/b/194673>

> I wonder if we should include a color'd UI for regex searches to at least
> distinguish it easier, and immediately know you're typing a regex and not a
> literal string. If I saw the same dark orange as in the quick console when I
> type a regex it would immediately be clear to me that I'm performing a regex
> search.
Unless we can do something _really_ fancy, like matching different colors for different groups (e.g. <https://regex101.com>), I'd say we leave it as is.

[Devin Rousso]

Created attachment 362058 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 362058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362058&action=review

r=me

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:190
> +localizedStrings["Case Sensitive"] = "Case Sensitive";

We can include a UIString comment for these strings that they are referring to different "Text search option"s. I suspect "Regular Expressions" might be confusing to some localizers.

> Source/WebInspectorUI/UserInterface/Base/SearchUtilities.js:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

Maybe 2019

> Source/WebInspectorUI/UserInterface/Views/SearchResultTreeElement.js:49
> +        let searchTermLength = textRange.endColumn - textRange.startColumn;

Is it possible that a search may span multiple lines (for a RegExp search for example?)

What happens if you do a regex search for ".+" how bad are the results?

> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:101
> -        searchQuery = searchQuery.trim();
>          if (!searchQuery.length)
>              return;

Does the trim() happen elsewhere or do you want to allow searching for whitespace?

> LayoutTests/inspector/debugger/search-scripts.html:17
> +    InspectorTest.debug();

Oops

[Devin Rousso]

Comment on attachment 362058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362058&action=review

>> Source/WebInspectorUI/UserInterface/Base/SearchUtilities.js:2
>> + * Copyright (C) 2018 Apple Inc. All rights reserved.
> 
> Maybe 2019

Considering that this file was authored in 2018, I feel like this is accurate.

>> Source/WebInspectorUI/UserInterface/Views/SearchResultTreeElement.js:49
>> +        let searchTermLength = textRange.endColumn - textRange.startColumn;
> 
> Is it possible that a search may span multiple lines (for a RegExp search for example?)
> 
> What happens if you do a regex search for ".+" how bad are the results?

We don't support multi-line searching as of now.  As such, searching for /.+/ will result in every line of every file being completely highlighted once.

<https://webkit.org/b/194680>

>> Source/WebInspectorUI/UserInterface/Views/SearchSidebarPanel.js:101
>>              return;
> 
> Does the trim() happen elsewhere or do you want to allow searching for whitespace?

I think we should let people search for whitespace.  It seems quite arbitrary not to.

>> LayoutTests/inspector/debugger/search-scripts.html:17
>> +    InspectorTest.debug();
> 
> Oops

Oops

[Devin Rousso]

Created attachment 362075 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362075 [details]
Patch

Rejecting attachment 362075 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 362075, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=362075&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=192527&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 362075 from bug 192527.
Fetching: https://bugs.webkit.org/attachment.cgi?id=362075
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	Source/WebInspectorUI/UserInterface/Base/SearchUtilities.js
	M	LayoutTests/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/ChangeLog' is out of date
W: 36a6554657feac45c3703b1bc63ecc1d8e84a8d0 and refs/remotes/origin/master differ, using rebase:
:040000 040000 7f74e141023e98e18a2cb4f765e9442a22abb086 70969c91286c71c2634812bccdf98b37d3ec4b18 M	LayoutTests
:040000 040000 e4a1a7b76c509028d3b5a58bca5559a835a8b51d bd7edca337fadf2c600e8f9e3c94e71b1d428ff4 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	Source/WebInspectorUI/UserInterface/Base/SearchUtilities.js
	M	LayoutTests/ChangeLog

ERROR from SVN:
Item is out of date: File '/trunk/LayoutTests/ChangeLog' is out of date
W: 36a6554657feac45c3703b1bc63ecc1d8e84a8d0 and refs/remotes/origin/master differ, using rebase:
:040000 040000 7f74e141023e98e18a2cb4f765e9442a22abb086 70969c91286c71c2634812bccdf98b37d3ec4b18 M	LayoutTests
:040000 040000 e4a1a7b76c509028d3b5a58bca5559a835a8b51d bd7edca337fadf2c600e8f9e3c94e71b1d428ff4 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: https://webkit-queues.webkit.org/results/11272251

[Devin Rousso]

Created attachment 362872 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362872 [details]
Patch

Clearing flags on attachment: 362872

Committed r242018: <https://trac.webkit.org/changeset/242018>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.