Bug 104155 – Sanitize content on copy in the code review tool

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

Sanitize content on copy in the code review tool

[Ojan Vafai]

Created attachment 177810 [details]
Patch

[Tony Chang]

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 ;

[Ojan Vafai]

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.

[Ojan Vafai]

Created attachment 177848 [details]
Patch

[Ojan Vafai]

Committed r136772: <http://trac.webkit.org/changeset/136772>

[Adam Barth]

There's a small bug with this patch.  It looks like the next line after a blank line is indented by an extra space.

[Adam Barth]

I should also say: thanks for implementing this feature request!

[Ojan Vafai]

(In reply to comment #6)
> There's a small bug with this patch.  It looks like the next line after a blank line is indented by an extra space.

Interesting. Looks like it might actually be a webkit bug with white-space:pre-wrap. :(

http://plexode.com/u/#-zle2q%3A2k%3D2Zspa2Yine2X%3B2W%2FMVMxtarea6ARstyz2P%3C2O%222N%3E-Mte2K%0AbJ!div8LY6LHrHYContaYrGNrGO!Zn8MxtOr9PWZn!Wdib86classkO26%20Q!NKP~http://pzxode.com/eval3/#ht=PRNKZn6%7BKwhiM-Zceq6pre-wrapXK%7DKPWR!div6conMntEditabzJSezct6all6%2B6copy6here9vJPbrN9vJThen6pasM6in6the6Vbelow9v!Wdiv!VRkOwidthq100%25X6heightq6400pxON&jt=6666layoutFzxIMms(*m_orderIMrator%2C6lYConMxts)XKK66666LayoutUnit6oldClientAfMrEdge6k6clientLogicalBottom()X

[noel gordon]

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()')

[Ojan Vafai]

Should be fixed: https://bugs.webkit.org/show_bug.cgi?id=104331

[noel gordon]

or not defined in Safari, thx for the fix.