Bug 147767 - Web Inspector: Don't include zero-width space into a copied text from the console
Summary: Web Inspector: Don't include zero-width space into a copied text from the con...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Devin Rousso
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-06 20:28 PDT by Nikita Vasilyev
Modified: 2015-08-07 20:45 PDT (History)
9 users (show)

See Also:


Attachments
[Animated GIF] Bug (768.69 KB, image/gif)
2015-08-06 20:28 PDT, Nikita Vasilyev
no flags Details
Patch (4.11 KB, patch)
2015-08-07 19:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.30 KB, patch)
2015-08-07 19:23 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.31 KB, patch)
2015-08-07 19:33 PDT, 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.
Description Nikita Vasilyev 2015-08-06 20:28:32 PDT
Created attachment 258439 [details]
[Animated GIF] Bug

Steps:
1.
    var img = new Image;
    img.src = "http://33.media.tumblr.com/tumblr_m6t5rzAfVU1qc0s10o1_500.gif";
    img;
2. Click on the logged console message to select it
3. Press Command-C to copy it

Expected clipboard content: 
<img src="http://33.media.tumblr.com/tumblr_m6t5rzAfVU1qc0s10o1_500.gif">

Actual clipboard content:
<img src=​"http:​/​/​33.media.tumblr.com/​tumblr_m6t5rzAfVU1qc0s10o1_500.gif">​
 = $1

1. There are a few zero-width spaces (\u200B) in the copied message; they are marked as red dots if you paste this message to the console prompt (see the attached image).

2. " = $1" shouldn't be copied.
Comment 1 Timothy Hatcher 2015-08-07 10:04:09 PDT
WebInspector._copy should already be doing this, I guess it isn't being called?

    // Remove word break characters from the selection before putting it on the pasteboard.
    var selectionString = selection.toString().removeWordBreakCharacters();
    event.clipboardData.setData("text/plain", selectionString);
    event.preventDefault();
Comment 2 Devin Rousso 2015-08-07 19:08:11 PDT
Created attachment 258553 [details]
Patch
Comment 3 Timothy Hatcher 2015-08-07 19:16:46 PDT
Comment on attachment 258553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258553&action=review

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:190
> +            clipboardString = clipboardString.replace(/(\s*=\s*\$\d+)$/, "");

No need for the () in the regex.
Comment 4 Devin Rousso 2015-08-07 19:18:57 PDT
Should this also remove console information?  For example, entering

console.log("test");

If I copy the resulting line (which should just say "text") and paste it, I get

[Log] test

Should that [Log] be there?  I have also seen source location information at the end for functions.
Comment 5 Timothy Hatcher 2015-08-07 19:22:18 PDT
[Log[ is added on purpose. The location is also added. I think it makes sense to strip out the = $n parts.
Comment 6 Devin Rousso 2015-08-07 19:23:15 PDT
Created attachment 258554 [details]
Patch
Comment 7 Timothy Hatcher 2015-08-07 19:24:18 PDT
Comment on attachment 258554 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258554&action=review

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:190
> +        if (this._message.savedResultIndex)
> +            clipboardString = clipboardString.replace(/\s*=\s*\$\d+$/, "");

Wait, never mind. I think $n should stay. If you use $n in a future line (like $2.foo.bar), you want to be able to look back and see where it came from. So remote this stripping.
Comment 8 Devin Rousso 2015-08-07 19:33:20 PDT
Created attachment 258555 [details]
Patch
Comment 9 Devin Rousso 2015-08-07 19:34:16 PDT
Comment on attachment 258555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258555&action=review

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:190
> +            clipboardString = clipboardString.replace(/\s*=\s*(\$\d+)$/, " = $1");

By the way, I added this so that, as seen in Nikita's example, the " = $1" doesn't wrap to the next line.
Comment 10 Timothy Hatcher 2015-08-07 19:54:15 PDT
Comment on attachment 258555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258555&action=review

>> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:190
>> +            clipboardString = clipboardString.replace(/\s*=\s*(\$\d+)$/, " = $1");
> 
> By the way, I added this so that, as seen in Nikita's example, the " = $1" doesn't wrap to the next line.

Oh, I see. My bad.
Comment 11 Timothy Hatcher 2015-08-07 19:55:17 PDT
Comment on attachment 258555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258555&action=review

>>> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:190
>>> +            clipboardString = clipboardString.replace(/\s*=\s*(\$\d+)$/, " = $1");
>> 
>> By the way, I added this so that, as seen in Nikita's example, the " = $1" doesn't wrap to the next line.
> 
> Oh, I see. My bad.

Oh, you were stripping before. Now you just strip linebreaks there. Nice!
Comment 12 WebKit Commit Bot 2015-08-07 20:45:37 PDT
Comment on attachment 258555 [details]
Patch

Clearing flags on attachment: 258555

Committed r188184: <http://trac.webkit.org/changeset/188184>
Comment 13 WebKit Commit Bot 2015-08-07 20:45:41 PDT
All reviewed patches have been landed.  Closing bug.