Bug 187020

Summary: Web Inspector: REGRESSION (r213000): copy from Search results content view broken
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, joepeck, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch none

Description Matt Baker 2018-06-25 15:07:48 PDT 
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.
Comment 1 Matt Baker 2018-06-25 15:08:02 PDT 
<rdar://problem/40928766>
Comment 2 Matt Baker 2018-06-25 15:11:27 PDT 
Created attachment 343547 [details]
Patch
Comment 3 Joseph Pecoraro 2018-06-25 15:25:10 PDT 
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 4 Timothy Hatcher 2018-06-25 15:26:39 PDT 
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 5 Matt Baker 2018-06-25 16:26:35 PDT 
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 6 Matt Baker 2018-06-25 16:34:05 PDT 
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?
Comment 7 Matt Baker 2018-06-28 14:02:24 PDT 
Created attachment 343847 [details]
Patch
Comment 8 Timothy Hatcher 2018-06-28 15:11:14 PDT 
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 9 WebKit Commit Bot 2018-06-28 15:38:48 PDT 
Comment on attachment 343847 [details]
Patch

Clearing flags on attachment: 343847

Committed r233334: <https://trac.webkit.org/changeset/233334>
Comment 10 WebKit Commit Bot 2018-06-28 15:38:49 PDT 
All reviewed patches have been landed.  Closing bug.