Summary: | unlink removes inline style of anchor elements | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, enrica, jparent, mjs, ojan, tony | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 47428 | ||||||||
Bug Blocks: | 47710, 43017 | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2010-10-08 11:06:29 PDT
Weird. Now it seems like the only Firefox does this (Internet Explorer does not preserve style). Firefox also removes all other attributes such as lang, title, and class. Created attachment 70382 [details]
work in progress
I'm stuck with this bug because preserving style may introduce multiple nodes into DOM and we currently don't have a good way of dealing with this situation if styles were to contain non-inline styles. If we're removing a as in <a href="/" style="font-weight:bold;"><div id="div1">hello</div>world</a> Then we must replace a with a block element or push the style down into child nodes. Or we either get <div style="font-weight:bold;"><div id="div1">hello</div>world</div> or <div id="div1" style="font-weight:bold;">hello</div>world However, we if we had <a href="/" style="font-weight:bold;"><i>hello</i>world</a>WebKit we must get: <span style="font-weight:bold;"><i>hello</i>world</span>WebKit or <b><i>hello</i>world</b>WebKit On the other hand, if we had <a href="/" style="font-weight:bold;"><div>hello</div>world</a>WebKit we can't replace a with anything. we ought to get: <div style="font-weight: bold;">hello</div><b>world</b> or some variant of this. Created attachment 70826 [details]
fixes the bug
Comment on attachment 70826 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=70826&action=review > WebCore/editing/ApplyStyleCommand.cpp:1229 > if (m_styledInlineElement && element->hasTagName(m_styledInlineElement->tagQName())) { > - if (mode != RemoveNone) { > - if (extractedStyle && element->inlineStyleDecl()) > - extractedStyle->merge(element->inlineStyleDecl()); > - removeNodePreservingChildren(element); > - } > + if (mode == RemoveNone) > + return true; > + ASSERT(extractedStyle); > + if (element->inlineStyleDecl()) > + extractedStyle->merge(element->inlineStyleDecl()); > + removeNodePreservingChildren(element); > return true; > } This seems to be entirely refactoring and not part of the bug fix. Might be nice to land it separately. > WebCore/editing/ApplyStyleCommand.cpp:1592 > + Node* childNode = 0; It seems dangerous that this is a raw pointer. Can’t removeInlineStyleFromElement result in some kind of mutation? > WebCore/editing/ApplyStyleCommand.cpp:1598 > + removeInlineStyleFromElement(style.get(), elem, RemoveIfNeeded, styleToPushDown.get()); Two spaces here after one of the commas. > WebCore/editing/ApplyStyleCommand.cpp:1616 > + for (;childNode; childNode = childNode->nextSibling()) Please put a space after that first semicolon. > WebCore/editing/ApplyStyleCommand.cpp:1880 > + Position positionToComputeStyle; Not quite a grammatical name. Maybe positionForStyleComputation instead? Thanks for reviewing all my patches today! And I'm always impressed with how thorough your reviews are. (In reply to comment #5) > (From update of attachment 70826 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70826&action=review > > > WebCore/editing/ApplyStyleCommand.cpp:1229 > > if (m_styledInlineElement && element->hasTagName(m_styledInlineElement->tagQName())) { > > - if (mode != RemoveNone) { > > - if (extractedStyle && element->inlineStyleDecl()) > > - extractedStyle->merge(element->inlineStyleDecl()); > > - removeNodePreservingChildren(element); > > - } > > + if (mode == RemoveNone) > > + return true; > > + ASSERT(extractedStyle); > > + if (element->inlineStyleDecl()) > > + extractedStyle->merge(element->inlineStyleDecl()); > > + removeNodePreservingChildren(element); > > return true; > > } > > This seems to be entirely refactoring and not part of the bug fix. Might be nice to land it separately. Not really. This change makes sure that extractedStyle is always specified when mode isn't RemoveNone to prevent making the same mistakes. We should never be calling removeInlineStyleFromElement when mode is removeNone and element is a styled inline element because that result in a loss of inline style declaration. I added a comment to WebCore/ChangeLog. > > WebCore/editing/ApplyStyleCommand.cpp:1592 > > + Node* childNode = 0; > > It seems dangerous that this is a raw pointer. Can’t removeInlineStyleFromElement result in some kind of mutation? Good point. Changed node, next, prev, next, and childNode to RefPtr<Node>. > > WebCore/editing/ApplyStyleCommand.cpp:1598 > > + removeInlineStyleFromElement(style.get(), elem, RemoveIfNeeded, styleToPushDown.get()); > > Two spaces here after one of the commas. Fixed. > > WebCore/editing/ApplyStyleCommand.cpp:1616 > > + for (;childNode; childNode = childNode->nextSibling()) > > Please put a space after that first semicolon. Fixed. > > WebCore/editing/ApplyStyleCommand.cpp:1880 > > + Position positionToComputeStyle; > > Not quite a grammatical name. Maybe positionForStyleComputation instead? Renamed to positionForStyle"Comparison" because the position is used to take the diff of the style at that position and the style we're applying. Committed r69910: <http://trac.webkit.org/changeset/69910> |