Summary: | Web Inspector: provide a way to make searches case sensitive or use a regular expression | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, joepeck, mattbaker, megan_gardner, webkit-bug-importer | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=194673 https://bugs.webkit.org/show_bug.cgi?id=194680 |
||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||
Bug Blocks: | 194673, 195017, 198897, 210671, 210679 | ||||||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2018-12-08 10:26:45 PST
Created attachment 356875 [details]
Patch
Created attachment 356876 [details]
[Image] After Patch is applied (Search Tab)
Created attachment 356877 [details]
[Image] After Patch is applied (Settings Tab)
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! 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. (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. (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. Created attachment 358180 [details]
Patch
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 Created attachment 358575 [details]
Patch
webkit-patch strikes again :(
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. (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. Created attachment 362058 [details]
Patch
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 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 Created attachment 362075 [details]
Patch
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 commit-queue@webkit.org... 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 Created attachment 362872 [details]
Patch
Comment on attachment 362872 [details] Patch Clearing flags on attachment: 362872 Committed r242018: <https://trac.webkit.org/changeset/242018> All reviewed patches have been landed. Closing bug. |