Bug 30083 - Data loss occurs when unbolding nested bold tags.
Summary: Data loss occurs when unbolding nested bold tags.
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Ryosuke Niwa
Depends on:
Reported: 2009-10-05 11:07 PDT by Michael Thomas
Modified: 2009-10-26 15:33 PDT (History)
8 users (show)

See Also:

demonstrates the bug (825 bytes, text/html)
2009-10-17 20:38 PDT, Ryosuke Niwa
no flags Details
Fixes the loop in swapInNodePreservingAttributesAndChildren (4.09 KB, patch)
2009-10-17 21:38 PDT, Ryosuke Niwa
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Thomas 2009-10-05 11:07:22 PDT
Go to the midas demo:


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.
Comment 2 Ryosuke Niwa 2009-10-05 15:59:31 PDT
Mn... this seems to be caused by ApplyStyleCommand.  Do you think this is a regression?
Comment 3 Annie Sullivan 2009-10-05 16:02:54 PDT
My test case works fine in Safari 3.2/Win, so I think it's a regression, but maybe not a recent one.
Comment 4 Tancred Lindholm 2009-10-07 01:11:52 PDT
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>
Comment 5 Joseph Holsten 2009-10-17 19:05:44 PDT
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.
Comment 6 Ryosuke Niwa 2009-10-17 20:38:29 PDT
Created attachment 41368 [details]
demonstrates the bug

The attached file will hit ASSERT on L1466@ApplyStyleCommand.cpp.


in ApplyStyleCommand::removeInlineStyle(PassRefPtr<CSSMutableStyleDeclaration> style, const Position &start, const Position &end).
Comment 7 Ryosuke Niwa 2009-10-17 21:07:21 PDT
It turned out that replaceWithSpanOrRemoveIfWithoutAttributes is the one deleting <b>2</b> when called on the outer b element.
Comment 8 Ryosuke Niwa 2009-10-17 21:38:28 PDT
Created attachment 41369 [details]
Fixes the loop in swapInNodePreservingAttributesAndChildren

It turned out that the bug resided in swapInNodePreservingAttributesAndChildren:


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.
Comment 9 Eric Seidel (no email) 2009-10-19 14:32:57 PDT
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.
Comment 10 Eric Seidel (no email) 2009-10-19 15:19:46 PDT
Comment on attachment 41369 [details]
Fixes the loop in swapInNodePreservingAttributesAndChildren

Marking cq- since I've asked Ryosuke to consider further changes.
Comment 11 Ryosuke Niwa 2009-10-21 14:25:07 PDT
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?
Comment 12 Ryosuke Niwa 2009-10-21 14:29:12 PDT
(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.
Comment 13 Ryosuke Niwa 2009-10-26 15:32:47 PDT
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.
Comment 14 Ryosuke Niwa 2009-10-26 15:33:30 PDT
Landed as https://trac.webkit.org/changeset/50090.