Bug 27660 - createMarkup does not handle CSS properly
: createMarkup does not handle CSS properly
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC All
: P3 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-07-24 11:59 PST by
Modified: 2009-12-14 10:25 PST (History)


Attachments
Demonstrates the bug using InsertOrderedList (638 bytes, text/html)
2009-07-24 11:59 PST, Ryosuke Niwa
no flags Details
fixes the bug but may worsen the bug 26483 (20.31 KB, patch)
2009-07-24 21:05 PST, Ryosuke Niwa
justin.garcia: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-24 11:59:57 PST
Created an attachment (id=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 From 2009-07-24 12:21:38 PST -------
This is also not a P1.  Please see http://webkit.org/quality/bugpriorities.html.
------- Comment #2 From 2009-07-24 17:40:24 PST -------
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 From 2009-07-24 17:41:32 PST -------
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 From 2009-07-24 21:05:34 PST -------
Created an attachment (id=33483) [details]
fixes the bug but may worsen the bug 26483
------- Comment #5 From 2009-07-26 23:59:34 PST -------
(From update of attachment 33483 [details])
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 From 2009-07-27 01:16:09 PST -------
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 From 2009-07-27 13:40:38 PST -------
(From update of attachment 33483 [details])
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
------- Comment #8 From 2009-07-27 17:57:10 PST -------
Landed in http://trac.webkit.org/changeset/46446
------- Comment #9 From 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.