RESOLVED FIXED 65824
Repeated copy and paste result in nested font elements
https://bugs.webkit.org/show_bug.cgi?id=65824
Summary Repeated copy and paste result in nested font elements
Ryosuke Niwa
Reported 2011-08-06 21:44:01 PDT
While r81887 and subsequent change sets avoided nesting spans, it still allows b, em, font, and other styling elements to nest themselves. We should the extend the idea introduced in r81887 and remove other inline style elements.
Attachments
fixes the bug (34.45 KB, patch)
2011-08-06 22:57 PDT, Ryosuke Niwa
no flags
Updated rebaselines after r92620 (32.75 KB, patch)
2011-08-08 13:41 PDT, Ryosuke Niwa
no flags
Removed chromium-specific result for paste-text-011 (35.62 KB, patch)
2011-08-08 14:51 PDT, Ryosuke Niwa
no flags
Updated per review (35.67 KB, patch)
2011-08-08 17:38 PDT, Ryosuke Niwa
tony: review+
Ryosuke Niwa
Comment 1 2011-08-06 21:44:57 PDT
This separates some changes from my patch for the bug 34564, and extends it further to reduce the verbosity of the pasted markup further.
Ryosuke Niwa
Comment 2 2011-08-06 22:57:17 PDT
Created attachment 103172 [details] fixes the bug
Ryosuke Niwa
Comment 3 2011-08-08 13:41:35 PDT
Created attachment 103288 [details] Updated rebaselines after r92620
WebKit Review Bot
Comment 4 2011-08-08 14:33:31 PDT
Comment on attachment 103288 [details] Updated rebaselines after r92620 Attachment 103288 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9335107 New failing tests: editing/pasteboard/paste-text-011.html
Ryosuke Niwa
Comment 5 2011-08-08 14:51:01 PDT
Created attachment 103296 [details] Removed chromium-specific result for paste-text-011
Tony Chang
Comment 6 2011-08-08 17:06:14 PDT
Comment on attachment 103296 [details] Removed chromium-specific result for paste-text-011 View in context: https://bugs.webkit.org/attachment.cgi?id=103296&action=review Some small style nits and questions about some of the changed results. > Source/WebCore/editing/EditingStyle.cpp:640 > + DEFINE_STATIC_LOCAL(Vector<OwnPtr<HTMLElementEquivalent> >, HTMLElementEquivalents, ()); What's the benefit of OwnPtr here? Don't we want to just leak it on exit? > Source/WebCore/editing/EditingStyle.cpp:759 > + elementIsSpanOrElementEquivalent = i < HTMLElementEquivalents.size(); Nit: Maybe just set elementIsSpanOrElementEquivalent = true; before you break (could then also put the declaration of |i| in the for statement)? > Source/WebCore/editing/EditingStyle.cpp:786 > + if (editingInheritableProperties[i] == it->id()) > + matched = true; Should we break here? > Source/WebCore/editing/ReplaceSelectionCommand.cpp:960 > + if (RefPtr<Node> nodeToSplitTo = highestEnclosingNodeOfType(insertionPos, isInlineNodeWithStyle, CannotCrossEditingBoundary, Is it possible to write a test to verify that this editability check is correct? > LayoutTests/editing/pasteboard/paste-after-inline-style-element-expected.txt:8 > +| "line 2<#selection-caret>" > +| " " Is this expected? It looks like it was bold but no longer is. > LayoutTests/editing/pasteboard/paste-text-with-style-2-expected.txt:21 > +| <font> > +| class="Apple-style-span" > +| color="#ff0000" Why did font move to the outsize here? > LayoutTests/editing/pasteboard/paste-with-redundant-style-expected.txt:12 > +| <em> > +| "rocks<#selection-caret>" Why isn't this bold? It looks like it's bold before.
Ryosuke Niwa
Comment 7 2011-08-08 17:14:10 PDT
Comment on attachment 103296 [details] Removed chromium-specific result for paste-text-011 View in context: https://bugs.webkit.org/attachment.cgi?id=103296&action=review >> Source/WebCore/editing/EditingStyle.cpp:640 >> + DEFINE_STATIC_LOCAL(Vector<OwnPtr<HTMLElementEquivalent> >, HTMLElementEquivalents, ()); > > What's the benefit of OwnPtr here? Don't we want to just leak it on exit? I don't think we want to use that pattern anymore. Also this patches the code in htmlAttributeEquivalents. >> Source/WebCore/editing/EditingStyle.cpp:759 >> + elementIsSpanOrElementEquivalent = i < HTMLElementEquivalents.size(); > > Nit: Maybe just set elementIsSpanOrElementEquivalent = true; before you break (could then also put the declaration of |i| in the for statement)? Will do. >> Source/WebCore/editing/EditingStyle.cpp:786 >> + matched = true; > > Should we break here? Definitely. That's a good catch. >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:960 >> + if (RefPtr<Node> nodeToSplitTo = highestEnclosingNodeOfType(insertionPos, isInlineNodeWithStyle, CannotCrossEditingBoundary, > > Is it possible to write a test to verify that this editability check is correct? The existing tests verify this already (tests crash otherwise). >> LayoutTests/editing/pasteboard/paste-after-inline-style-element-expected.txt:8 >> +| " " > > Is this expected? It looks like it was bold but no longer is. Yes. This is fixing a bug. See http://trac.webkit.org/browser/trunk/LayoutTests/editing/pasteboard/paste-after-inline-style-element.html. We're inserting a new blockquote after the first one. Since b is inside the first blockquote, we should certainly not bold it. >> LayoutTests/editing/pasteboard/paste-text-with-style-2-expected.txt:21 >> +| color="#ff0000" > > Why did font move to the outsize here? Yeah, this is probably done by ApplyStyleCommand (typing style). We shouldn't be wrapping br like this to begin with. >> LayoutTests/editing/pasteboard/paste-with-redundant-style-expected.txt:12 >> +| "rocks<#selection-caret>" > > Why isn't this bold? It looks like it's bold before. See the CSS rule "em { text-decoration: underline; font-style: none; }". It was never bolded.
Ryosuke Niwa
Comment 8 2011-08-08 17:38:13 PDT
Created attachment 103320 [details] Updated per review
Tony Chang
Comment 9 2011-08-09 10:12:09 PDT
(In reply to comment #7) > (From update of attachment 103296 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103296&action=review > > >> Source/WebCore/editing/EditingStyle.cpp:640 > >> + DEFINE_STATIC_LOCAL(Vector<OwnPtr<HTMLElementEquivalent> >, HTMLElementEquivalents, ()); > > > > What's the benefit of OwnPtr here? Don't we want to just leak it on exit? > > I don't think we want to use that pattern anymore. Also this patches the code in htmlAttributeEquivalents. Why not? Using a raw pointer should be a bit faster. Using an OwnPtr doesn't provide any benefits here. > >> LayoutTests/editing/pasteboard/paste-with-redundant-style-expected.txt:12 > >> +| "rocks<#selection-caret>" > > > > Why isn't this bold? It looks like it's bold before. > > See the CSS rule "em { text-decoration: underline; font-style: none; }". It was never bolded. I don't see that CSS rule. I see <em style="font-style: none; text-decoration: underline; font-weight: bold;"> and an em with no style information.
Ryosuke Niwa
Comment 10 2011-08-09 10:28:09 PDT
(In reply to comment #9) > > >> LayoutTests/editing/pasteboard/paste-with-redundant-style-expected.txt:12 > > >> +| "rocks<#selection-caret>" > > > > > > Why isn't this bold? It looks like it's bold before. > > > > See the CSS rule "em { text-decoration: underline; font-style: none; }". It was never bolded. > > I don't see that CSS rule. I see <em style="font-style: none; text-decoration: underline; font-weight: bold;"> and an em with no style information. 1 <!DOCTYPE html> 2 <html> 3 <head> 4 <style> 5 em { text-decoration: underline; font-style: none; } 6 </style> 7 </head> 8 <body> 9 <div id="test" contenteditable><b style="font-style: italic;"><br></b></div> 10 <script src="../../resources/dump-as-markup.js"></script> 11 <script> 12 13 document.getElementById('test').focus(); 14 document.execCommand('insertHTML', false, 15 '<em style="font-style: none; text-decoration: underline; font-weight: bold;"><span style="color: black;">hello world</span></em><br>' 16 + '<span style="font-weight: bold;"><span class="Apple-style-span" style="font-weight: bold;">WebKit</span></span><br>' 17 + '<em><span style="font-style: italic;">rocks</span></em>');
Tony Chang
Comment 11 2011-08-09 10:36:18 PDT
(In reply to comment #10) > (In reply to comment #9) > > > >> LayoutTests/editing/pasteboard/paste-with-redundant-style-expected.txt:12 > > > >> +| "rocks<#selection-caret>" > > > > > > > > Why isn't this bold? It looks like it's bold before. > > > > > > See the CSS rule "em { text-decoration: underline; font-style: none; }". It was never bolded. > > > > I don't see that CSS rule. I see <em style="font-style: none; text-decoration: underline; font-weight: bold;"> and an em with no style information. > > 4 <style> > 5 em { text-decoration: underline; font-style: none; } > 6 </style> Ah, right. So why does this nullify the bolding? text-decoration and font-style don't impact font-weight do they? When I load the test, everything is bolded in ToT.
Ryosuke Niwa
Comment 12 2011-08-09 10:45:17 PDT
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > > See the CSS rule "em { text-decoration: underline; font-style: none; }". It was never bolded. > > > > > > I don't see that CSS rule. I see <em style="font-style: none; text-decoration: underline; font-weight: bold;"> and an em with no style information. > > > > 4 <style> > > 5 em { text-decoration: underline; font-style: none; } > > 6 </style> > > Ah, right. So why does this nullify the bolding? text-decoration and font-style don't impact font-weight do they? > > When I load the test, everything is bolded in ToT. Ah, you're right. But new behavior is as expected. Because we preserve all editing styles when we copy, inline elements at the insertion position should not affect the style of the pasted contents. As such, the correct behavior here is not to bold the entire content. The same behavior change is observed on editing/pasteboard/paste-after-inline-style-element.html
Ryosuke Niwa
Comment 13 2011-08-09 10:46:48 PDT
In fact, this is the bug "<rdar://problem/5371536> Style rules that match pasted content can change it's appearance" as documented in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline.
Tony Chang
Comment 14 2011-08-09 10:51:59 PDT
Comment on attachment 103320 [details] Updated per review (In reply to comment #12) > Ah, you're right. But new behavior is as expected. Because we preserve all editing styles when we copy, inline elements at the insertion position should not affect the style of the pasted contents. As such, the correct behavior here is not to bold the entire content. The same behavior change is observed on editing/pasteboard/paste-after-inline-style-element.html Please document these changes in the ChangeLog (what changed and why the new output is correct).
Ryosuke Niwa
Comment 15 2011-08-09 10:58:23 PDT
Thanks for the review! (In reply to comment #14) > > Ah, you're right. But new behavior is as expected. Because we preserve all editing styles when we copy, inline elements at the insertion position should not affect the style of the pasted contents. As such, the correct behavior here is not to bold the entire content. The same behavior change is observed on editing/pasteboard/paste-after-inline-style-element.html > > Please document these changes in the ChangeLog (what changed and why the new output is correct). Yeah. Now that I think about it, it wasn't obvious at all.
Ryosuke Niwa
Comment 16 2011-08-09 11:23:28 PDT
How about adding the following paragraph in LayoutTests/ChangeLog? Because WebKit serializes all editing inheritable styles on copy, we should be able to remove all inline styles at the insertion point on paste. And this patch removes inline style elements such as font and b more aggressively. WebKit erroneously bolded the last words in paste-after-inline-style-element.html and paste-with-redundant-style.html before this change set but this is no longer the case.
Tony Chang
Comment 17 2011-08-09 11:28:11 PDT
(In reply to comment #16) > How about adding the following paragraph in LayoutTests/ChangeLog? > > Because WebKit serializes all editing inheritable styles on copy, we should be able to remove all inline styles > at the insertion point on paste. And this patch removes inline style elements such as font and b more aggressively. > WebKit erroneously bolded the last words in paste-after-inline-style-element.html and paste-with-redundant-style.html > before this change set but this is no longer the case. LGTM
Ryosuke Niwa
Comment 18 2011-08-09 12:10:37 PDT
Note You need to log in before you can comment on or make changes to this bug.