Bug 27660 - createMarkup does not handle CSS properly
: createMarkup does not handle CSS properly
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: PC All
: P3 Normal
Assigned To: Ryosuke Niwa
Depends on:
  Show dependency treegraph
Reported: 2009-07-24 11:59 PDT by Ryosuke Niwa
Modified: 2009-12-14 10:25 PST (History)
4 users (show)

See Also:

Demonstrates the bug using InsertOrderedList (638 bytes, text/html)
2009-07-24 11:59 PDT, Ryosuke Niwa
no flags Details
fixes the bug but may worsen the bug 26483 (20.31 KB, patch)
2009-07-24 21:05 PDT, Ryosuke Niwa
justin.garcia: 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 2009-07-24 11:59:57 PDT
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.
Comment 1 Eric Seidel 2009-07-24 12:21:38 PDT
This is also not a P1.  Please see http://webkit.org/quality/bugpriorities.html.
Comment 2 Ryosuke Niwa 2009-07-24 17:40:24 PDT
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?
Comment 3 Ryosuke Niwa 2009-07-24 17:41:32 PDT
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.
Comment 4 Ryosuke Niwa 2009-07-24 21:05:34 PDT
Created attachment 33483 [details]
fixes the bug but may worsen the bug 26483
Comment 5 Eric Seidel 2009-07-26 23:59:34 PDT
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?
Comment 6 Ryosuke Niwa 2009-07-27 01:16:09 PDT
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.
Comment 7 Justin Garcia 2009-07-27 13:40:38 PDT
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.

Comment 8 Ryosuke Niwa 2009-07-27 17:57:10 PDT
Landed in http://trac.webkit.org/changeset/46446
Comment 9 John Sullivan 2009-12-14 10:25:51 PST
The assertion added in this patch is firing in a case involving copying link text -- https://bugs.webkit.org/show_bug.cgi?id=32525.