Bug 47424 – unlink removes inline style of anchor elements

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

[Ryosuke Niwa]

WebKit removes all inline style declaration from anchor elements on unlink.

Reproduction steps:
1. Goto http://www.mozilla.org/editor/midasdemo/
2. Type in the following HTML in HTML source mode.
hello
<a href="/" style="font-weight: bold;display:block;">world</a>
WebKit
3. Select all
4. Unlink

Expected result (Firefox and Internet Explorer get this result):
hello
<span style="style-weight: bold;display:block;">world
WebKit

Actual result:
hello world WebKit.

Both .

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 70382 [details]
work in progress

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 70826 [details]
fixes the bug

[Darin Adler]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r69910: <http://trac.webkit.org/changeset/69910>