Bug 47424 - unlink removes inline style of anchor elements
Summary: unlink removes inline style of anchor elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 47428
Blocks: 47710 43017
  Show dependency treegraph
 
Reported: 2010-10-08 11:06 PDT by Ryosuke Niwa
Modified: 2010-10-15 21:10 PDT (History)
6 users (show)

See Also:


Attachments
work in progress (9.01 KB, patch)
2010-10-09 17:47 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixes the bug (15.66 KB, patch)
2010-10-14 20:21 PDT, Ryosuke Niwa
darin: 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 2010-10-08 11:06:29 PDT
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 .
Comment 1 Ryosuke Niwa 2010-10-08 11:48:32 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.
Comment 2 Ryosuke Niwa 2010-10-09 17:47:45 PDT
Created attachment 70382 [details]
work in progress
Comment 3 Ryosuke Niwa 2010-10-12 11:33:00 PDT
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.
Comment 4 Ryosuke Niwa 2010-10-14 20:21:27 PDT
Created attachment 70826 [details]
fixes the bug
Comment 5 Darin Adler 2010-10-15 11:11:07 PDT
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?
Comment 6 Ryosuke Niwa 2010-10-15 14:12:15 PDT
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.
Comment 7 Ryosuke Niwa 2010-10-15 21:10:43 PDT
Committed r69910: <http://trac.webkit.org/changeset/69910>