Summary: Copy from Search results content view broken. The selected tree element always takes precedence over the TextEditor selection. Steps to Reproduce: 1. Inspect webkit.org, search for "body" 2. Select a tree element in the Search tab 3. Select some text in the editor 4. Copy Expected: => Selection in editor is copied Actual: => Tree element is copied Regressed in https://bugs.webkit.org/show_bug.cgi?id=167074.
<rdar://problem/40928766>
Created attachment 343547 [details] Patch
Comment on attachment 343547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343547&action=review > Source/WebInspectorUI/UserInterface/Views/SearchTabContentView.js:102 > handleCopyEvent(event) > { > + let currentContentView = this.contentBrowser.currentContentView; Is this specific to the SearchTab? If so why doesn't it happen in other tabs?
Comment on attachment 343547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343547&action=review > Source/WebInspectorUI/UserInterface/Views/SearchTabContentView.js:113 > + let currentContentView = this.contentBrowser.currentContentView; > + if (currentContentView) { > + if (currentContentView instanceof WI.ClusterContentView) > + currentContentView = currentContentView.contentViewContainer.currentContentView; > + > + if (currentContentView.textEditor) { > + let range = currentContentView.textEditor.selectedTextRange; > + // If the TextEditor contains a selection, don't override the default copy behavior. > + if (range.startLine !== range.endLine || range.startColumn !== range.endColumn) > + return; > + } > + } This seems like it should be passed and handled at another level, since Search isn't the only client of cluster views and nested content browsers. For example, WI.ClusterContentView passes through handleCopyEvent already. Maybe ContentBrowser needs this most of this code? But it shouldn't know about TextEditor either. Hmm.
Comment on attachment 343547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343547&action=review >> Source/WebInspectorUI/UserInterface/Views/SearchTabContentView.js:102 >> + let currentContentView = this.contentBrowser.currentContentView; > > Is this specific to the SearchTab? If so why doesn't it happen in other tabs? It is. A check for handleCopyEvent support was added for TabContentView, and SearchTabContentView is the only implementor.
Comment on attachment 343547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343547&action=review >> Source/WebInspectorUI/UserInterface/Views/SearchTabContentView.js:113 >> + } > > This seems like it should be passed and handled at another level, since Search isn't the only client of cluster views and nested content browsers. > > For example, WI.ClusterContentView passes through handleCopyEvent already. Maybe ContentBrowser needs this most of this code? But it shouldn't know about TextEditor either. Hmm. This doesn't have anything to do with ClusterContentView or nesting. SearchTabContentView is implementing handleCopyEvent, which causes WI._copy to override the default behavior (coping the CodeMirror selection). What would your expectation be if this were performed at a higher level? Should WI._copy bail out early if focusedContentView has a text selection?
Created attachment 343847 [details] Patch
Comment on attachment 343847 [details] Patch I like this better. This might be something we should do elsewhere or generally in WI._copy. But I don't see any other handleCopyEvent code paths that would have this same issue.
Comment on attachment 343847 [details] Patch Clearing flags on attachment: 343847 Committed r233334: <https://trac.webkit.org/changeset/233334>
All reviewed patches have been landed. Closing bug.