Bug 65824

This separates some changes from my patch for the bug 34564, and extends it further to reduce the verbosity of the pasted markup further.

Created attachment 103172 [details] fixes the bug

Created attachment 103288 [details] Updated rebaselines after r92620

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

Created attachment 103296 [details] Removed chromium-specific result for paste-text-011

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.

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.

Created attachment 103320 [details] Updated per review

(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.

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

(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.

(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

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.

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).

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.

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.

(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

Committed r92695: <http://trac.webkit.org/changeset/92695>