| Summary: | createMarkup does not handle CSS properly | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
| Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | eric, jparent, justin.garcia, sullivan | ||||||
| Priority: | P3 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
This is also not a P1. Please see http://webkit.org/quality/bugpriorities.html. 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? 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. Created attachment 33483 [details] fixes the bug but may worsen the bug 26483 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? 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 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 Landed in http://trac.webkit.org/changeset/46446 The assertion added in this patch is firing in a case involving copying link text -- https://bugs.webkit.org/show_bug.cgi?id=32525. |
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.