Bug 27660 – createMarkup does not handle CSS properly

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Created attachment 33458 [details]
Demonstrates the bug using InsertOrderedList

createMarkup adds span with style, which is directly taken from CSSMutableDeclaration class.  But this causes the following problems:

1. -webkit-text-decorations-in-effect hasn't been converted to text-decoration.
2. It produces CSS regardless of whether StyleWithCSS is on or not.

[Eric Seidel (no email)]

This is also not a P1.  Please see http://webkit.org/quality/bugpriorities.html.

[Ryosuke Niwa]

Changing specialCommonAncestor turned out to be rather challenging because of /LayoutTests/editing/deleting/pruning-after-merge-2.html.

In this test, we have
<div id="test" contenteditable="true"><div>foo</div><div style="border: 1px solid blue; padding: 5px"><b><div style="border: 1px solid red; padding: 5px">bar</div></b></div></div>

and we try to delete both div's that surrounds "bar".  But allowing specialCommonAncestor to be any presentational node (u, s, strike, i, em, b, & strong) causes this div be included in the fragment.

However, this is malformed HTML to begin with since we are allowing block element inside an inline element.  Should we delete the inner div?  Or should we delete b between div's?

[Ryosuke Niwa]

editing/deleting/delete-3857753-fix.html and editing/pasteboard/display-block-on-spans.html will also require rebaseline but that will be nothing but improvement.

[Ryosuke Niwa]

Created attachment 33483 [details]
fixes the bug but may worsen the bug 26483

[Eric Seidel (no email)]

Comment on attachment 33483 [details]
fixes the bug but may worsen the bug 26483

Isn't this patch doing two separate things?

It looks like the addStyleMarkup change is just code-cleanup, and is very easy to r+.  The isElementPresentational usage change seems to be a functional change.  Can we separate these two changes into separate patches, or am I missing something?

[Ryosuke Niwa]

As you mentioned, I'm making two changes:
1. Addition of addStyleMarkup
2. Modification of isElementPresentational (=elementHasTextDecorationProperty)

But they are inseparable in that fixing the bug requires me changing isElementPresentational which means I need to add the first ASSERT of addStyleMarkup in three places to signify that -webkit-text-decorations-in-effect isn't passed to the style.

On the other hand, if I were to make addStyleMarkup first, I need to ensure that -webkit-text-decorations-in-effect isn't passed to the style because that's what the function must assume. But this means I need to fix elementHasTextDecorationProperty beforehand. But I could make this change first if you really want me to separate the patch.

[Justin Garcia]

Comment on attachment 33483 [details]
fixes the bug but may worsen the bug 26483

No need to separate the patch, it's rather small.

+    // All text-decoration-related elements should have been treated as special ancestor

Small typo.  Should read "as special ancestors".

+    DEFINE_STATIC_LOCAL(const String, divCloseTag, ("</div>"));
+    DEFINE_STATIC_LOCAL(const String, styleSpanOpen, ("<span class=\"" AppleStyleSpanClass "\" style=\""));
+    DEFINE_STATIC_LOCAL(const String, styleSpanClose, ("</span>"));

I'd name it "divClose" for consistency.

r=me

[Ryosuke Niwa]

Landed in http://trac.webkit.org/changeset/46446

[John Sullivan]

The assertion added in this patch is firing in a case involving copying link text -- https://bugs.webkit.org/show_bug.cgi?id=32525.