Bug 104155 - Sanitize content on copy in the code review tool
Summary: Sanitize content on copy in the code review tool
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-05 12:15 PST by Ojan Vafai
Modified: 2012-12-06 18:45 PST (History)
6 users (show)

See Also:


Attachments
Patch (9.85 KB, patch)
2012-12-05 12:30 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (9.90 KB, patch)
2012-12-05 15:35 PST, Ojan Vafai
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-12-05 12:15:06 PST
Sanitize content on copy in the code review tool
Comment 1 Ojan Vafai 2012-12-05 12:30:36 PST
Created attachment 177810 [details]
Patch
Comment 2 Tony Chang 2012-12-05 12:43:23 PST
Comment on attachment 177810 [details]
Patch

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

> Websites/bugs.webkit.org/code-review.js:1184
> +    // FIXME: When event.clipboardData.setData supports text/html, remove all the code below.

Is there an existing webkit bug we can link to?

> Websites/bugs.webkit.org/code-review.js:1187
> +    document.body.appendChild(container);

Is this going to cause the page to flicker (possibly changing scroll position)?

> Websites/bugs.webkit.org/code-review.js:1194
> +    })

Nit: missing ;
Comment 3 Ojan Vafai 2012-12-05 15:35:13 PST
Comment on attachment 177810 [details]
Patch

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

>> Websites/bugs.webkit.org/code-review.js:1184
>> +    // FIXME: When event.clipboardData.setData supports text/html, remove all the code below.
> 
> Is there an existing webkit bug we can link to?

No. I filed bug 104179.

>> Websites/bugs.webkit.org/code-review.js:1187
>> +    document.body.appendChild(container);
> 
> Is this going to cause the page to flicker (possibly changing scroll position)?

Doesn't seem to.

>> Websites/bugs.webkit.org/code-review.js:1194
>> +    })
> 
> Nit: missing ;

Fixed.
Comment 4 Ojan Vafai 2012-12-05 15:35:42 PST
Created attachment 177848 [details]
Patch
Comment 5 Ojan Vafai 2012-12-05 15:40:37 PST
Committed r136772: <http://trac.webkit.org/changeset/136772>
Comment 6 Adam Barth 2012-12-05 18:23:06 PST
There's a small bug with this patch.  It looks like the next line after a blank line is indented by an extra space.
Comment 7 Adam Barth 2012-12-05 18:24:01 PST
I should also say: thanks for implementing this feature request!
Comment 9 noel gordon 2012-12-06 18:40:03 PST
Comment on attachment 177848 [details]
Patch

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

> Websites/bugs.webkit.org/code-review.js:1164
> +    classesToRemove.forEach(function(className) {
> +      forEachNode(fragment.querySelectorAll('.' + className), function(node) {
> +        node.remove();

Not sure what's going on, but node can be null here in Safari ...

[13:06]  <smfr> did something change with the review page? it's really whacky now on keypresses
[13:06]  <smfr> command-C nukes the selection
[13:07]  <leviw> oof, I haven't noticed that
[13:07]  <smfr> who touched it?
[13:07]  <leviw> smfr: when I use command-C it seems to blow away the selection and restore it (selection rect flashes)
[13:07]  <leviw> no clue
[13:08]  <smfr> ojan_away: you?
[13:08]  <smfr> abarth: the review page has a bunch of issues now
[13:08]  <smfr> abarth: can we revert ojan's commit?
[13:10]  <smfr> TypeError: 'undefined' is not a function (evaluating 'container.remove()')
Comment 10 Ojan Vafai 2012-12-06 18:43:10 PST
Should be fixed: https://bugs.webkit.org/show_bug.cgi?id=104331
Comment 11 noel gordon 2012-12-06 18:45:53 PST
or not defined in Safari, thx for the fix.