Sanitize content on copy in the code review tool
Created attachment 177810 [details] Patch
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 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.
Created attachment 177848 [details] Patch
Committed r136772: <http://trac.webkit.org/changeset/136772>
There's a small bug with this patch. It looks like the next line after a blank line is indented by an extra space.
I should also say: thanks for implementing this feature request!
(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
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()')
Should be fixed: https://bugs.webkit.org/show_bug.cgi?id=104331
or not defined in Safari, thx for the fix.