Bug 26937 is related in that the pasted html is invalid (with spans around divs), but even if we fix bug 26937 at paste time, we shouldn't be putting invalid HTML in the clipboard. Comments 5+ on bug 26937 describe the problem in more detail. Comment 7 has an example that will generate invalid HTML on the clipboard.
This bug seems to be still causing a lot of problems in pasting side. I have a plan to fix this issue in several iterations: 1. Isolate the code to find the special ancestors into a function, and call this function before calling serializeNodes. 2. Modify ReplaceSelectionCommand so that it'll remove duplicated style from all top-level nodes in the fragment. 3. Add an ability for serializeNodes to annotate document + editing style on each top-level nodes in the fragment. We'll use this only if there is no special common ancestor. 4. Stop wrapping styles in createMarkup
Created attachment 94727 [details] work in progress
Can you give an example of how styles will get applied to top-level nodes? For example, if I have the following HTML: <ul> <li>Line One</li> <li>Line Two</li> </ul> And the text should be red, would it look like this: <ul> <li><font color="ff0000">Line One</font></li> <li><font color="ff0000">Line Two</font></li> </ul> Or like this: <ul style="color:red"> <li>Line One</li> <li>Line Two</li> </ul> Or something else? Do I understand correctly that we currently wrap the <font> or <span> tag around the entire <ul>?
The latter. We'd have to include all computed style in inline declaration when we copy and remove them all when we paste.
(In reply to comment #4) > The latter. We'd have to include all computed style in inline declaration when we copy and remove them all when we paste. The latter is complicated for a few reasons: 1. Some styles apply differently to block elements than to inline ones. For example, setting background-color on the ul will look very different than wrapping the list items in background-color styled spans. 2. The former behavior is how WebKit currently behaves if you select the list in a contenteditable region and use execCommand to apply a style. It's confusing for web developers to have the two different ways styles can be applied (of course, the current span-wrapping is worse).
(In reply to comment #5) > 1. Some styles apply differently to block elements than to inline ones. For example, setting background-color on the ul will look very different than wrapping the list items in background-color styled spans. Sure but the style is coming from the ancestors, and we apply the style on children when we paste anyways. > 2. The former behavior is how WebKit currently behaves if you select the list in a contenteditable region and use execCommand to apply a style. It's confusing for web developers to have the two different ways styles can be applied (of course, the current span-wrapping is worse). This is irrelevant because we'll strip all redundant styles in paste side. And it's impossible to get the same behavior between serialization code and ApplyStyleCommand because we can't modify DOM when we serialize DOM. New approach will only be visible to websites if they manually interrupt paste event and grab the clipboard content directly but then they need to deal with different kinds of markup anyways now.
Created attachment 95063 [details] work in progress 2 Smart replace is broken and there's something wrong with XML copy & paste right now but this patch is a good representative of where I'm heading.
This patch requires the following tests be rebaselined. I'll covert them to text-based tests first to make the correctness clear. editing/pasteboard/4700297.html editing/pasteboard/5065605.html editing/pasteboard/5089327.html editing/pasteboard/5483567.html editing/pasteboard/emacs-ctrl-k-y-001.html editing/pasteboard/interchange-newline-2.html editing/pasteboard/merge-end-3.html editing/pasteboard/merge-end-5.html editing/pasteboard/merge-end-blockquote.html editing/pasteboard/merge-end-borders.html editing/pasteboard/merge-end-list.html editing/pasteboard/merge-end-table.html editing/pasteboard/paste-line-endings-007.html editing/pasteboard/paste-line-endings-008.html editing/pasteboard/paste-line-endings-009.html editing/pasteboard/paste-text-004.html editing/pasteboard/paste-text-005.html editing/pasteboard/paste-text-008.html editing/pasteboard/paste-text-009.html editing/pasteboard/paste-text-010.html editing/pasteboard/smart-paste-001.html editing/pasteboard/smart-paste-003.html editing/pasteboard/smart-paste-006.html editing/pasteboard/smart-paste-007.html editing/pasteboard/smart-paste-008.html
More tests to convert: editing/pasteboard/5027857.html editing/pasteboard/interchange-newline-1.html editing/pasteboard/merge-end-1.html editing/pasteboard/merge-end-2.html editing/pasteboard/paste-blockquote-3.html editing/pasteboard/paste-noscript-xhtml.xhtml editing/pasteboard/paste-noscript.html editing/pasteboard/paste-pre-001.html editing/pasteboard/paste-pre-002.html editing/pasteboard/paste-table-001.html editing/pasteboard/paste-text-001.html editing/pasteboard/paste-text-002.html editing/pasteboard/paste-text-003.html editing/pasteboard/paste-unrendered-select.html editing/pasteboard/smart-paste-002.html editing/pasteboard/smart-paste-004.html editing/pasteboard/smart-paste-005.html These are actually more important because the correctness of rebaselines for these tests is less obvious.
Created attachment 95214 [details] work in progress 3 This approach is flawed as it turns out. I need to remove UA default styles when we serialize markup instead of removing them in the paste side.
Created attachment 95235 [details] work in progress 4 Okay, now only 34 tests fail and 2 tests crash. I'll have to split changes and see which change is causing which failure now. Also, we'd have to be more clever about avoiding presentational elements when we're pasting contents. e.g. when we copy & paste <b><i>hello</i></b> at the same place, we'll end up nesting them like <b><i><b style="font-weight: normal; font-style: normal;"><i>hello</i></b></i></b>. Maybe I'll file a blocker and fix it separately.
Created attachment 95857 [details] work in progress 5 There are 3-4 regressions and 8 layout tests fail but there are no crashes so it's getting there. I'm hoping to get this ready for a review by the end of next week.
Created attachment 95991 [details] alpha version This patch works in the sense that there are no obvious regressions. I'll try cleaning up the code and submit for a review on Monday.
Attachment 95991 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/editing/deleting/deleting-line..." exit_code: 1 Source/WebCore/editing/EditingStyle.h:126: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 96010 [details] beta version Cleaned up the patch. This is ready for a review once I write the change log entries.
Created attachment 96013 [details] Patch
Can someone review my patch?
Comment on attachment 96013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96013&action=review > LayoutTests/editing/execCommand/insert-list-with-noneditable-content-expected.txt:7 > -| style="background-color: LightGray;" > +| style="background-color: rgb(211, 211, 211); " Is this intentional? > LayoutTests/editing/pasteboard/5065605-expected.txt:47 > | <font> > | class="Apple-style-span" > | color="#ff0000" > -| <span> > +| <font> > | class="Apple-style-span" > +| color="#ff0000" > +| "This text should be red." > +| <div> The fonts are now nested? Yet neither of them provides any font information? > LayoutTests/editing/pasteboard/display-block-on-spans-expected.txt:33 > | <b> > -| "This<#selection-caret>" > -| <b> > +| <b> > +| "This<#selection-caret>" The bolds are now nested? > LayoutTests/editing/pasteboard/paste-pre-001-expected.txt:10 > -execPasteCommand: <div id="test" class="editing"> <pre><span class="Apple-style-span" style="font-family: Times; white-space: normal; "><pre>foo bar</pre></span></pre> </div> > +execPasteCommand: <div id="test" class="editing"> <pre><pre>foo bar</pre></pre> </div> Is this change intentional?
Comment on attachment 96013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96013&action=review >> LayoutTests/editing/execCommand/insert-list-with-noneditable-content-expected.txt:7 >> +| style="background-color: rgb(211, 211, 211); " > > Is this intentional? As I mentioned in the change log, this is visually identical. It's just that we're preserving background color in different code path. >> LayoutTests/editing/pasteboard/5065605-expected.txt:47 >> +| <div> > > The fonts are now nested? Yet neither of them provides any font information? Not quite. We used to nest font, span, font, div, font, div font. Now we only have font, font, div, font. >> LayoutTests/editing/pasteboard/display-block-on-spans-expected.txt:33 >> +| "This<#selection-caret>" > > The bolds are now nested? Yeah, sad but we should be able to improve it in follow ups. >> LayoutTests/editing/pasteboard/paste-pre-001-expected.txt:10 >> +execPasteCommand: <div id="test" class="editing"> <pre><pre>foo bar</pre></pre> </div> > > Is this change intentional? Yes, this span wasn't doing anything useful because pre overrode both font-family and white-space.
Ping reviewers.
Any reviewers?
Ping reviewers again.
Comment on attachment 96013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96013&action=review I would like to review this patch, but it is too large for me to comprehend. Is it possible to break it into smaller patches? Here's one suggestion on how to do that: a) a patch for adding inline style declarations to top level elements (we would still wrap in an apple style span) b) a patch for wraping top level text nodes with a style span (maybe in addition to the apple style span) c) a patch to remove apple style span since it is now redundant I admit that it would make the resulting html worse temporarily (since we're duplicating styles), but hopefully that wouldn't be more than a few weeks. > LayoutTests/editing/pasteboard/paste-text-with-style-expected.txt:40 > +| <div> > +| style="font-style: normal; font-weight: normal; " > +| <i> > +| "hello italic" In this test, why did the div end up inside the <i> further up the DOM? This test seems to have gone from valid html to invalid html.
I talked with Tony about this and I'm going to split this patch into two pieces: 1. Add style removal logic to RelaceSelectionCommand. We can even add tests to ensure redundant styles are not added here. 2. Update serialization code not to use wrapping span. I'll first update the existing patch for ToT.
It appears that this patch is now broken due to various changes in ReplaceSelectionCommand.
Apparently, this was caused by r92330 and I found various regressions caused by r92330 while investigating the issue. It turned out that this bug really blocks the bug 26483.
Created attachment 103144 [details] updated
Comment on attachment 103144 [details] updated Attachment 103144 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9328055 New failing tests: fast/events/ondrop-text-html.html editing/pasteboard/paste-pre-002.html editing/pasteboard/paste-code-in-pre.html editing/pasteboard/onpaste-text-html.html editing/pasteboard/paste-text-011.html editing/pasteboard/paste-pre-001.html editing/pasteboard/data-transfer-items.html
Created attachment 103171 [details] rebaselined more tests for chromium
Comment on attachment 103171 [details] rebaselined more tests for chromium Attachment 103171 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9329470 New failing tests: fast/events/ondrop-text-html.html editing/pasteboard/paste-code-in-pre.html
Created attachment 103299 [details] updated after r92620
Comment on attachment 103299 [details] updated after r92620 Most of rebaselines in this patch are due to the bug 65824 so the final patch should be even smaller than this.
Comment on attachment 103299 [details] updated after r92620 Attachment 103299 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9333178 New failing tests: fast/events/ondrop-text-html.html editing/pasteboard/paste-code-in-pre.html
Created attachment 103412 [details] updated after r92695
Comment on attachment 103412 [details] updated after r92695 Oops, I included 2 bad rebaselines. Uploading a new patch in a minute.
Created attachment 103414 [details] reverted 2 bad rebaselines
I've decided to keep code that deals with style spans in ReplaceSelectionCommand to paste contents copied by old versions of WebKit. We can probably kill it in the future once this version become dominant.
Comment on attachment 103412 [details] updated after r92695 View in context: https://bugs.webkit.org/attachment.cgi?id=103412&action=review Sorry, comments on the previous patch, but I think they still apply. > Source/WebCore/editing/markup.cpp:139 > + Node* traverseNodesForSerialization(Node* startNode, Node* pastEnd, bool shouldEmit); It would be nice if shouldEmit was an enum. > Source/WebCore/editing/markup.cpp:223 > + // <rdar://problem/5371536> Nit: Maybe add FIXME: ? > Source/WebCore/editing/markup.cpp:224 > + // Make sure spans are inline style in paste side e.g. span { display: block } Nit: Period at the end of the comment. > Source/WebCore/editing/markup.cpp:226 > + // FIXME: should this be included in forceInline? Nit: Capitalize |should|. > Source/WebCore/editing/markup.cpp:339 > + Node* lastClosed = traverseNodesForSerialization(startNode, pastEnd, false); It looks like we traverse the nodes for serialization twice now (once before) so we can determine the highest node to be serialized. Do you think that will have a perf impact?
Comment on attachment 103412 [details] updated after r92695 View in context: https://bugs.webkit.org/attachment.cgi?id=103412&action=review >> Source/WebCore/editing/markup.cpp:139 >> + Node* traverseNodesForSerialization(Node* startNode, Node* pastEnd, bool shouldEmit); > > It would be nice if shouldEmit was an enum. That's a good idea. >> Source/WebCore/editing/markup.cpp:223 >> + // <rdar://problem/5371536> > > Nit: Maybe add FIXME: ? Yeah, it should be // FIXME: <rdar://problem/5371536> Style rules that match pasted content can change it's appearance instead >> Source/WebCore/editing/markup.cpp:224 > > Nit: Period at the end of the comment. Oops, will fix. >> Source/WebCore/editing/markup.cpp:226 >> + // FIXME: should this be included in forceInline? > > Nit: Capitalize |should|. Will fix. >> Source/WebCore/editing/markup.cpp:339 >> + Node* lastClosed = traverseNodesForSerialization(startNode, pastEnd, false); > > It looks like we traverse the nodes for serialization twice now (once before) so we can determine the highest node to be serialized. Do you think that will have a perf impact? I don't think it will. Sure it'll double the traversal time but I suspect that we spend much more time generating string and concatenating them.
Created attachment 103567 [details] Updated per comment #38
Created attachment 103568 [details] Renamed enum values
Comment on attachment 103568 [details] Renamed enum values View in context: https://bugs.webkit.org/attachment.cgi?id=103568&action=review > Source/WebCore/ChangeLog:34 > + with shouldEmit set false first to compute the highest node to be serialized. The second call to Nit: Use enum name. > Source/WebCore/ChangeLog:37 > + Outputs string only if shouldEmit is true. Nit: Ditto. > Source/WebCore/editing/markup.cpp:221 > + bool parentIsTextarea = text->parentElement() && text->parentElement()->tagQName() == textareaTag; > + bool wrappingSpan = shouldApplyWrappingStyle(text) && !parentIsTextarea; FWIW, the usage of const bool for local variables seems kind of arbitrary. I don't think there's a style guide rule one way or another (and I don't have a strong preference, I find them not that useful but not that noisy either), but it would be nice if we were at least consistent within a file. E.g., you could make these const. > Source/WebCore/editing/markup.cpp:291 > + bool shouldAnnotateOrForceInline = element->isHTMLElement() && (shouldAnnotate() || addDisplayInline); > + bool shouldOverrideStyleAttr = shouldAnnotateOrForceInline || shouldApplyWrappingStyle(element); ...these could be const too.
Thanks for the review! I'm very excited to land this patch. (In reply to comment #42) > > Source/WebCore/ChangeLog:34 > > + with shouldEmit set false first to compute the highest node to be serialized. The second call to > > Nit: Use enum name. Fixed. > > Source/WebCore/ChangeLog:37 > > + Outputs string only if shouldEmit is true. > > Nit: Ditto. Fixed. > > Source/WebCore/editing/markup.cpp:221 > > + bool parentIsTextarea = text->parentElement() && text->parentElement()->tagQName() == textareaTag; > > + bool wrappingSpan = shouldApplyWrappingStyle(text) && !parentIsTextarea; > > FWIW, the usage of const bool for local variables seems kind of arbitrary. I don't think there's a style guide rule one way or another (and I don't have a strong preference, I find them not that useful but not that noisy either), but it would be nice if we were at least consistent within a file. E.g., you could make these const. Fixed.
Committed r92823: <http://trac.webkit.org/changeset/92823>
(In reply to comment #44) > Committed r92823: <http://trac.webkit.org/changeset/92823> This change caused bug 75330.