Bug 30083 – Data loss occurs when unbolding nested bold tags.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Michael Thomas]

Go to the midas demo:

http://mozilla.org/editor/midasdemo/

Edit the html source.

Enter the following html snippet:

<b id="foo">1<b>2</b></b>

Switch out of html edit mode.

Highlight the "12" and hit unbold.

Note that the "2" is gone.

[Annie Sullivan]

Automated test case: http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cdiv%20id%3D%22editor%22%20contenteditable%3Dtrue%3E%3Cb%20id%3Dfoo%3E1%3Cb%3E2%3C%2Fb%3E%3C%2Fb%3E%3C%2Fdiv%3E%0A&ohh=1&ohj=1&jt=var%20div%20%3D%20document.getElementById('editor')%3B%0A%0A%2F%2F%20Change%20this%20to%20select%20a%20range%0AselectRange(document.getElementById('foo')%2C%200%2C%0A%20%20%20%20%20%20%20%20%20%20%20%20document.getElementById('foo')%2C%202)%3B%0A%0A%2F%2F%20Change%20this%20to%20run%20execCommand%0Adocument.execCommand('bold'%2C%20false%2C%20null)%3B%0A%0Afunction%20selectRange(startNode%2C%20startOffset%2C%20endNode%2C%20endOffset)%20%7B%0A%20%20var%20sel%20%3D%20window.getSelection()%3B%0A%20%20var%20range%20%3D%20null%3B%0A%20%20try%20%7B%0A%20%20%20%20range%20%3D%20sel.getRangeAt(0)%3B%0A%20%20%7D%20catch(ex)%20%7B%0A%20%20%20%20range%20%3D%20document.createRange()%3B%0A%20%20%7D%0A%20%20range.setStart(startNode%2C%20startOffset)%3B%0A%20%20range.setEnd(endNode%2C%20endOffset)%3B%0A%20%20sel.removeAllRanges()%3B%0A%20%20sel.addRange(range)%3B%0A%7D&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

[Ryosuke Niwa]

Mn... this seems to be caused by ApplyStyleCommand.  Do you think this is a regression?

[Annie Sullivan]

My test case works fine in Safari 3.2/Win, so I think it's a regression, but maybe not a recent one.

[Tancred Lindholm]

Note that the bug reproduces with *any* attribute in the outer tag, and with other inline style tags, such as at least <i>. Sample cases:

<i id=foo>1<i>2</i></i>
<b class=foo>1<b>2</b></b>
<i lang=en>1<i>2</i></i>
<b lang=en id=1>1<b>2</b></b>
<b title="Bold text">1<b>2</b></b>

[Joseph Holsten]

What needs to be done to reduce this test case? Seems pretty well reduced to me.

I'm a bit new to this testing bit, sorry.

[Ryosuke Niwa]

Created attachment 41368 [details]
demonstrates the bug

The attached file will hit ASSERT on [e-mail].

ASSERT(s.node()->inDocument());

in ApplyStyleCommand::removeInlineStyle(PassRefPtr<CSSMutableStyleDeclaration> style, const Position &start, const Position &end).

[Ryosuke Niwa]

It turned out that replaceWithSpanOrRemoveIfWithoutAttributes is the one deleting <b>2</b> when called on the outer b element.

[Ryosuke Niwa]

Created attachment 41369 [details]
Fixes the loop in swapInNodePreservingAttributesAndChildren

It turned out that the bug resided in swapInNodePreservingAttributesAndChildren:

https://trac.webkit.org/browser/trunk/WebCore/editing/ReplaceNodeWithSpanCommand.cpp#L60

60	    for (Node* child = nodeToReplace->firstChild(); child; child = child->nextSibling()) {
61	        newNode->appendChild(child, ec);
62	        ASSERT(!ec);
63	    }

As soon as we appendChild to newNode in L61, nextSibling() is changed to 0.
In my patch, I introduced Node* nextChild to temporarily save the pointer.

[Eric Seidel (no email)]

Comment on attachment 41369 [details]
Fixes the loop in swapInNodePreservingAttributesAndChildren

This would be much better as a test which showed PASS or FAIL.  script-test style tests could do that.

I might have written this a while loop instead.  But the current for seems fine.  You might want a comment there:

 62         nextChild = child->nextSibling(); // Save nextSibling() as appendChild(child) will change child->nextSibling().

r+, but consider making this a script-test and adding a comment.

[Eric Seidel (no email)]

Comment on attachment 41369 [details]
Fixes the loop in swapInNodePreservingAttributesAndChildren

Marking cq- since I've asked Ryosuke to consider further changes.

[Ryosuke Niwa]

While converting the test to a js test, I found another bug.  WebKit fails to unbold <b><b>hello</b>world</b> because in this case font-weight: 800 and won't match 'bold'.  I'm thinking to modify the change so that we check whether the font-weight is normal or not but this might have some weird side-effect.

We could fix in a separate patch but I much rather fix it in this patch than having to rebaseline the test later.  Eric & Justin, any opinion on this?

[Ryosuke Niwa]

(In reply to comment #11)
> modify the change so that we check whether the

I mean to say modify the check.  i.e. reverse the order of "normal" and "bold" passed to executeToggleBold.

[Ryosuke Niwa]

Talked with Eric on IRC.  I committed this patch with new js test that demonstrates the bug I mentioned (WebKit can't unbold <b><b>hello</b></b>), see Bug 30784 for the detail.

[Ryosuke Niwa]

Landed as https://trac.webkit.org/changeset/50090.