Bug 65824 – Repeated copy and paste result in nested font elements

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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]

Created attachment 103172 [details]
fixes the bug

[Ryosuke Niwa]

Created attachment 103288 [details]
Updated rebaselines after r92620

[WebKit Review Bot]

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]

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

[Tony Chang]

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 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]

Created attachment 103320 [details]
Updated per review

[Tony Chang]

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

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

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

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

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 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]

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]

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]

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

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