Bug 30836 – Creating a link when selecting multiple nodes creates multiple links

• Component: HTML Editing
• Status: RESOLVED FIXED

[Marcos Almeida]

1. Go to http://www.mozilla.org/editor/midasdemo/
2. type "abc"
3. select "b" and press the italic button on the second toolbar
4. press ctrl-a to select all
5. press the create link button (the globe icon with a chain link, on the first toolbar)
6. type abc in the dialog that pops up and press ok
7. click the "view html source" checkbox at the bottom to see the generated html

Expected:
<a href="abc">a<i>b</i>c</a>

Actual:
<a href="abc">a</a><i><a href="abc">b</a></i><a href="abc">c</a>


This affects Google properties.

[Julie Parent]

Standalone demo: http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cdiv%20id%3D'foo'%20contentEditable%3Efoo%3Cb%3Ebar%3C%2Fb%3Ebaz%3C%2Fdiv%3E&ohh=1&ohj=1&jt=gebi('foo').focus()%3B%0Adocument.execCommand(%22SelectAll%22)%3B%0Adocument.execCommand(%22createLink%22%2C%20false%2C%20%22http%3A%2F%2Fwww.google.com%22)%3B%0A%0A%2F%2F%20Should%20have%20only%20one%20anchor%20tag%0Aif%20(foo.getElementsByTagName('A').length%20%3D%3D%201)%20%7B%0A%20%20alert(%22PASS%22)%3B%0A%7D%20else%20%7B%0A%20%20alert(%22FAIL%22)%3B%0A%7D&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1, will alert PASS if one link is created, FAIL otherwise

[Ryosuke Niwa]

Created attachment 65356 [details]
initial patch

[Tony Chang]

Comment on attachment 65356 [details]
initial patch

> Index: WebCore/editing/ApplyStyleCommand.cpp
> +    bool operator==(const StyleChange& other)
> +    {
> +        return m_cssStyle == other.m_cssStyle

I am worried that this will regress someday when someone adds a new member, but I don't have a good suggestion on how to avoid this.

> -            while (sibling && sibling != pastEndNode && (!sibling->isElementNode() || sibling->hasTagName(brTag)) && !isBlock(sibling)) {
> +            StyleChange startChange(style, Position(node, 0));
> +            while (sibling && sibling != pastEndNode && !sibling->contains(pastEndNode)) {
> +                if (isBlock(sibling) && !sibling->hasTagName(brTag))
> +                    break;
> +                if (StyleChange(style, Position(sibling, 0)) != startChange)
> +                    break;

Was there a reason to move conditions into separate if statements?  Seems like these could be merged with the while condition.

> +        RefPtr<StyledElement> styledElement;
> +        if (current->isStyledElement() && m_styledInlineElement && current->hasTagName(m_styledInlineElement->tagQName()))
> +            styledElement = static_cast<StyledElement*>(current);

Does |styledElement| need to be a RefPtr?


> Index: WebCore/editing/ApplyStyleCommand.h
> -    void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start, Node* end);
> +    void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start, Node* end, bool shouldAddStyledElement = true);

Can we use an enum instead of a bool?


Can you also add a test for anchors with inline styles.

[Ryosuke Niwa]

Created attachment 65469 [details]
fixed per Tony's comment

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 65356 [details])
> > Index: WebCore/editing/ApplyStyleCommand.cpp
> > +    bool operator==(const StyleChange& other)
> > +    {
> > +        return m_cssStyle == other.m_cssStyle
> 
> I am worried that this will regress someday when someone adds a new member, but I don't have a good suggestion on how to avoid this.

I know.  I spent some time thinking about it but couldn't come up with a better way either.

> > -            while (sibling && sibling != pastEndNode && (!sibling->isElementNode() || sibling->hasTagName(brTag)) && !isBlock(sibling)) {
> > +            StyleChange startChange(style, Position(node, 0));
> > +            while (sibling && sibling != pastEndNode && !sibling->contains(pastEndNode)) {
> > +                if (isBlock(sibling) && !sibling->hasTagName(brTag))
> > +                    break;
> > +                if (StyleChange(style, Position(sibling, 0)) != startChange)
> > +                    break;
> 
> Was there a reason to move conditions into separate if statements?  Seems like these could be merged with the while condition.

Merged.

> > +        RefPtr<StyledElement> styledElement;
> > +        if (current->isStyledElement() && m_styledInlineElement && current->hasTagName(m_styledInlineElement->tagQName()))
> > +            styledElement = static_cast<StyledElement*>(current);
> 
> Does |styledElement| need to be a RefPtr?

Yes, because extractInlineStyleToPushDown may remove "current" in which case it may be destroyed if we didn't kept a pointer.

> > Index: WebCore/editing/ApplyStyleCommand.h
> > -    void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start, Node* end);
> > +    void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start, Node* end, bool shouldAddStyledElement = true);
> 
> Can we use an enum instead of a bool?

Fixed.

> Can you also add a test for anchors with inline styles.

Good thing we talked about this.  There were a couple of bugs revealed by new test cases.

[Ryosuke Niwa]

Committed r66040: <http://trac.webkit.org/changeset/66040>