Bug 65824 - Repeated copy and paste result in nested font elements
Summary: Repeated copy and paste result in nested font elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 34564
  Show dependency treegraph
 
Reported: 2011-08-06 21:44 PDT by Ryosuke Niwa
Modified: 2011-08-09 12:10 PDT (History)
8 users (show)

See Also:


Attachments
fixes the bug (34.45 KB, patch)
2011-08-06 22:57 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated rebaselines after r92620 (32.75 KB, patch)
2011-08-08 13:41 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Removed chromium-specific result for paste-text-011 (35.62 KB, patch)
2011-08-08 14:51 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated per review (35.67 KB, patch)
2011-08-08 17:38 PDT, Ryosuke Niwa
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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.
Comment 1 Ryosuke Niwa 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.
Comment 2 Ryosuke Niwa 2011-08-06 22:57:17 PDT
Created attachment 103172 [details]
fixes the bug
Comment 3 Ryosuke Niwa 2011-08-08 13:41:35 PDT
Created attachment 103288 [details]
Updated rebaselines after r92620
Comment 4 WebKit Review Bot 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
Comment 5 Ryosuke Niwa 2011-08-08 14:51:01 PDT
Created attachment 103296 [details]
Removed chromium-specific result for paste-text-011
Comment 6 Tony Chang 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.
Comment 7 Ryosuke Niwa 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.
Comment 8 Ryosuke Niwa 2011-08-08 17:38:13 PDT
Created attachment 103320 [details]
Updated per review
Comment 9 Tony Chang 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.
Comment 10 Ryosuke Niwa 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>');
Comment 11 Tony Chang 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.
Comment 12 Ryosuke Niwa 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
Comment 13 Ryosuke Niwa 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.
Comment 14 Tony Chang 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).
Comment 15 Ryosuke Niwa 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.
Comment 16 Ryosuke Niwa 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.
Comment 17 Tony Chang 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
Comment 18 Ryosuke Niwa 2011-08-09 12:10:37 PDT
Committed r92695: <http://trac.webkit.org/changeset/92695>