WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r92695
: <
http://trac.webkit.org/changeset/92695
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug