Bug 192527 - Web Inspector: provide a way to make searches case sensitive or use a regular expression
Summary: Web Inspector: provide a way to make searches case sensitive or use a regular...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 194673 195017 198897
  Show dependency treegraph
 
Reported: 2018-12-08 10:26 PST by Devin Rousso
Modified: 2019-06-15 18:48 PDT (History)
7 users (show)

See Also:


Attachments
Patch (31.08 KB, patch)
2018-12-08 10:39 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (Search Tab) (117.25 KB, image/png)
2018-12-08 10:40 PST, Devin Rousso
no flags Details
[Image] After Patch is applied (Settings Tab) (718.57 KB, image/png)
2018-12-08 10:40 PST, Devin Rousso
no flags Details
Patch (45.49 KB, patch)
2019-01-01 22:33 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (50.00 KB, patch)
2019-01-07 22:28 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (54.95 KB, patch)
2019-02-14 14:10 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (56.60 KB, patch)
2019-02-14 15:54 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (56.53 KB, patch)
2019-02-24 19:10 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Devin Rousso 2018-12-08 10:39:52 PST
Created attachment 356875 [details]
Patch
Comment 2 Devin Rousso 2018-12-08 10:40:20 PST
Created attachment 356876 [details]
[Image] After Patch is applied (Search Tab)
Comment 3 Devin Rousso 2018-12-08 10:40:39 PST
Created attachment 356877 [details]
[Image] After Patch is applied (Settings Tab)
Comment 4 Joseph Pecoraro 2018-12-12 17:08:30 PST
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!
Comment 5 Matt Baker 2018-12-12 18:03:59 PST
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.
Comment 6 Joseph Pecoraro 2018-12-13 10:58:42 PST
(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.
Comment 7 Devin Rousso 2018-12-13 11:14:16 PST
(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.
Comment 8 Radar WebKit Bug Importer 2018-12-17 21:33:23 PST
<rdar://problem/46800955>
Comment 9 Devin Rousso 2019-01-01 22:33:20 PST
Created attachment 358180 [details]
Patch
Comment 10 Joseph Pecoraro 2019-01-07 19:33:05 PST
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
Comment 11 Devin Rousso 2019-01-07 22:28:31 PST
Created attachment 358575 [details]
Patch

webkit-patch strikes again :(
Comment 12 Joseph Pecoraro 2019-02-14 11:57:23 PST
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.
Comment 13 Devin Rousso 2019-02-14 14:08:53 PST
(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.
Comment 14 Devin Rousso 2019-02-14 14:10:09 PST
Created attachment 362058 [details]
Patch
Comment 15 Joseph Pecoraro 2019-02-14 15:16:33 PST
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
Comment 16 Devin Rousso 2019-02-14 15:43:40 PST
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
Comment 17 Devin Rousso 2019-02-14 15:54:03 PST
Created attachment 362075 [details]
Patch
Comment 18 WebKit Commit Bot 2019-02-24 19:04:10 PST Comment hidden (obsolete)
Comment 19 Devin Rousso 2019-02-24 19:10:55 PST
Created attachment 362872 [details]
Patch
Comment 20 WebKit Commit Bot 2019-02-24 20:27:52 PST
Comment on attachment 362872 [details]
Patch

Clearing flags on attachment: 362872

Committed r242018: <https://trac.webkit.org/changeset/242018>
Comment 21 WebKit Commit Bot 2019-02-24 20:27:54 PST
All reviewed patches have been landed.  Closing bug.