Bug 187020 – Web Inspector: REGRESSION (r213000): copy from Search results content view broken

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

[Matt Baker]

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.

[Matt Baker]

<rdar://problem/40928766>

[Matt Baker]

Created attachment 343547 [details]
Patch

[Joseph Pecoraro]

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?

[Timothy Hatcher]

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.

[Matt Baker]

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.

[Matt Baker]

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?

[Matt Baker]

Created attachment 343847 [details]
Patch

[Timothy Hatcher]

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.

[WebKit Commit Bot]

Comment on attachment 343847 [details]
Patch

Clearing flags on attachment: 343847

Committed r233334: <https://trac.webkit.org/changeset/233334>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.