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.
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
Mn... this seems to be caused by ApplyStyleCommand. Do you think this is a regression?
My test case works fine in Safari 3.2/Win, so I think it's a regression, but maybe not a recent one.
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>
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.
Created attachment 41368 [details] demonstrates the bug The attached file will hit ASSERT on L1466@ApplyStyleCommand.cpp. ASSERT(s.node()->inDocument()); in ApplyStyleCommand::removeInlineStyle(PassRefPtr<CSSMutableStyleDeclaration> style, const Position &start, const Position &end).
It turned out that replaceWithSpanOrRemoveIfWithoutAttributes is the one deleting <b>2</b> when called on the outer b element.
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.
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 on attachment 41369 [details] Fixes the loop in swapInNodePreservingAttributesAndChildren Marking cq- since I've asked Ryosuke to consider further changes.
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?
(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.
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.
Landed as https://trac.webkit.org/changeset/50090.