30083 2009-10-05 11:07:22 -0700 Data loss occurs when unbolding nested bold tags. 2009-10-26 15:33:30 -0700 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) All All RESOLVED FIXED P3 Normal --- 1 michaelthomas rniwa enrica eric joseph jparent rniwa sullivan tancred.lindholm.spammy thakis oldest_to_newest 152375 0 michaelthomas 2009-10-05 11:07:22 -0700 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. 152485 1 sullivan 2009-10-05 15:12:03 -0700 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 152502 2 rniwa 2009-10-05 15:59:31 -0700 Mn... this seems to be caused by ApplyStyleCommand. Do you think this is a regression? 152505 3 sullivan 2009-10-05 16:02:54 -0700 My test case works fine in Safari 3.2/Win, so I think it's a regression, but maybe not a recent one. 152982 4 tancred.lindholm.spammy 2009-10-07 01:11:52 -0700 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> 155433 5 joseph 2009-10-17 19:05:44 -0700 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. 155439 6 41368 rniwa 2009-10-17 20:38:29 -0700 Created attachment 41368 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). 155440 7 rniwa 2009-10-17 21:07:21 -0700 It turned out that replaceWithSpanOrRemoveIfWithoutAttributes is the one deleting <b>2</b> when called on the outer b element. 155443 8 41369 rniwa 2009-10-17 21:38:28 -0700 Created attachment 41369 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. 155910 9 41369 eric 2009-10-19 14:32:57 -0700 Comment on attachment 41369 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. 155951 10 41369 eric 2009-10-19 15:19:46 -0700 Comment on attachment 41369 Fixes the loop in swapInNodePreservingAttributesAndChildren Marking cq- since I've asked Ryosuke to consider further changes. 156645 11 rniwa 2009-10-21 14:25:07 -0700 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? 156648 12 rniwa 2009-10-21 14:29:12 -0700 (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. 157969 13 rniwa 2009-10-26 15:32:47 -0700 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. 157970 14 rniwa 2009-10-26 15:33:30 -0700 Landed as https://trac.webkit.org/changeset/50090. 41368 2009-10-17 20:38:29 -0700 2009-10-17 20:38:29 -0700 demonstrates the bug t.html text/html 825 rniwa PGh0bWw+Cjxib2R5Pgo8cD5UaGlzIHRlc3RzIHVuLWJvbGRpbmcgbmVzdGVkIGIgZWxlbWVudHMg c3VjaCB0aGF0IHRoZSBvdXRlciBiIGhhcyBpZCBhdHRyaWJ1dGUuICBXZWJLaXQgc2hvdWxkbid0 IHJlbW92ZSB0aGUgb3V0ZXIgYi48L3A+CjxkaXYgaWQ9ImUiIGNvbnRlbnRlZGl0YWJsZT0idHJ1 ZSI+PHNwYW4gaWQ9InRlc3QiPjxiIGlkPSJmb28iPjE8Yj4yPC9iPjwvYj48L3NwYW4+PC9kaXY+ Cgo8dWw+CjxsaT5CZWZvcmU6PHNwYW4gaWQ9ImMxIj48L3NwYW4+PC9saT4KPGxpPkFmdGVyOjxz cGFuIGlkPSJjMiI+PC9zcGFuPjwvbGk+CjwvdWw+Cgo8c2NyaXB0IHR5cGU9InRleHQvamF2YXNj cmlwdCI+CgppZiAod2luZG93LmxheW91dFRlc3RDb250cm9sbGVyKQogICAgbGF5b3V0VGVzdENv bnRyb2xsZXIuZHVtcEFzVGV4dCgpOwoKdmFyIGUgPSBkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgn ZScpOwpkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgnYzEnKS5hcHBlbmRDaGlsZChkb2N1bWVudC5j cmVhdGVUZXh0Tm9kZShlLmlubmVySFRNTCkpOwoKdmFyIHMgPSB3aW5kb3cuZ2V0U2VsZWN0aW9u KCk7CnZhciByID0gZG9jdW1lbnQuY3JlYXRlUmFuZ2UoKTsKci5zZWxlY3ROb2RlKGRvY3VtZW50 LmdldEVsZW1lbnRCeUlkKCd0ZXN0JykpOwpzLnJlbW92ZUFsbFJhbmdlcygpOwpzLmFkZFJhbmdl KHIpOwpkb2N1bWVudC5leGVjQ29tbWFuZCgiYm9sZCIsIGZhbHNlLCBudWxsKTsKCmRvY3VtZW50 LmdldEVsZW1lbnRCeUlkKCdjMicpLmFwcGVuZENoaWxkKGRvY3VtZW50LmNyZWF0ZVRleHROb2Rl KGUuaW5uZXJIVE1MKSk7Cgo8L3NjcmlwdD4K 41369 2009-10-17 21:38:28 -0700 2009-10-19 15:19:46 -0700 Fixes the loop in swapInNodePreservingAttributesAndChildren fix30083 text/plain 4184 rniwa SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0OTc0NykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTkgQEAKKzIwMDktMTAtMTcgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5v cmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRGF0 YSBsb3NzIG9jY3VycyB3aGVuIHVuYm9sZGluZyBuZXN0ZWQgYm9sZCB0YWdzLgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzAwODMKKworICAgICAgICBG aXhlcyB0aGUgbG9vcCBpbiBzd2FwSW5Ob2RlUHJlc2VydmluZ0F0dHJpYnV0ZXNBbmRDaGlsZHJl biBieSBzYXZpbmcgbmV4dFNpYmxpbmcoKSBvZiBjaGlsZAorICAgICAgICB0byBhIHRlbXBvcmFy eSB2YWx1YWJsZS4gIEl0IHdhcyBvcmlnaW5hbGx5IGNhbGxpbmcgbmV4dFNpYmxpbmcoKSBhZnRl ciBhcHBlbmRpbmcgdGhlIGNoaWxkCisgICAgICAgIHRvIG5ldyBwYXJlbnQsIGluIHdoaWNoIGNh c2UsIG5leHRTaWJsaW5nIGlzIGFsd2F5cyAwLgorCisgICAgICAgIFRlc3Q6IGVkaXRpbmcvc3R5 bGUvdW5ib2xkaW5nLW5lc3RlZC1iLmh0bWwKKworICAgICAgICAqIGVkaXRpbmcvUmVwbGFjZU5v ZGVXaXRoU3BhbkNvbW1hbmQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6c3dhcEluTm9kZVByZXNl cnZpbmdBdHRyaWJ1dGVzQW5kQ2hpbGRyZW4pOgorCiAyMDA5LTEwLTE3ICBOaWtvbGFzIFppbW1l cm1hbm4gIDxuemltbWVybWFubkByaW0uY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEdlb3Jn ZSBTdGFpa29zLgpJbmRleDogV2ViQ29yZS9lZGl0aW5nL1JlcGxhY2VOb2RlV2l0aFNwYW5Db21t YW5kLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2VkaXRpbmcvUmVwbGFjZU5vZGVXaXRoU3Bh bkNvbW1hbmQuY3BwCShyZXZpc2lvbiA0OTc0NykKKysrIFdlYkNvcmUvZWRpdGluZy9SZXBsYWNl Tm9kZVdpdGhTcGFuQ29tbWFuZC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTU3LDcgKzU3LDkgQEAg c3RhdGljIHZvaWQgc3dhcEluTm9kZVByZXNlcnZpbmdBdHRyaWJ1dAogICAgIHBhcmVudE5vZGUt Pmluc2VydEJlZm9yZShuZXdOb2RlLCBub2RlVG9SZXBsYWNlLCBlYyk7CiAgICAgQVNTRVJUKCFl Yyk7CiAKLSAgICBmb3IgKE5vZGUqIGNoaWxkID0gbm9kZVRvUmVwbGFjZS0+Zmlyc3RDaGlsZCgp OyBjaGlsZDsgY2hpbGQgPSBjaGlsZC0+bmV4dFNpYmxpbmcoKSkgeworICAgIE5vZGUqIG5leHRD aGlsZDsKKyAgICBmb3IgKE5vZGUqIGNoaWxkID0gbm9kZVRvUmVwbGFjZS0+Zmlyc3RDaGlsZCgp OyBjaGlsZDsgY2hpbGQgPSBuZXh0Q2hpbGQpIHsKKyAgICAgICAgbmV4dENoaWxkID0gY2hpbGQt Pm5leHRTaWJsaW5nKCk7CiAgICAgICAgIG5ld05vZGUtPmFwcGVuZENoaWxkKGNoaWxkLCBlYyk7 CiAgICAgICAgIEFTU0VSVCghZWMpOwogICAgIH0KSW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxv Zwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHJldmlzaW9uIDQ5NzQ3KQor KysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAK KzIwMDktMTAtMTcgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRGF0YSBsb3NzIG9jY3VycyB3 aGVuIHVuYm9sZGluZyBuZXN0ZWQgYm9sZCB0YWdzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzAwODMKKworICAgICAgICBBZGRzIGEgdGVzdCB0byBt YWtlIHN1cmUgV2ViS2l0IGNhbiByZW1vdmUgbmVzdGVkIGIgdGFncyBwcm9wZXJseQorICAgICAg ICB3aXRob3V0IGxvb3NpbmcgdGhlIGNvbnRlbnQgb2YgaW5uZXIgYidzLgorCisgICAgICAgICog ZWRpdGluZy9zdHlsZS91bmJvbGRpbmctbmVzdGVkLWItZXhwZWN0ZWQudHh0OiBBZGRlZC4KKyAg ICAgICAgKiBlZGl0aW5nL3N0eWxlL3VuYm9sZGluZy1uZXN0ZWQtYi5odG1sOiBBZGRlZC4KKwog MjAwOS0xMC0xNyAgUGF2ZWwgRmVsZG1hbiAgPHBmZWxkbWFuQGNocm9taXVtLm9yZz4KIAogICAg ICAgICBOb3QgcmV2aWV3ZWQ6IHJlZHVjZSBpbnNwZWN0b3IgY29uc29sZSB0ZXN0IHNpemUgLSBp dCB0aW1lcyBvdXQuCkluZGV4OiBMYXlvdXRUZXN0cy9lZGl0aW5nL3N0eWxlL3VuYm9sZGluZy1u ZXN0ZWQtYi1leHBlY3RlZC50eHQKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvZWRpdGluZy9z dHlsZS91bmJvbGRpbmctbmVzdGVkLWItZXhwZWN0ZWQudHh0CShyZXZpc2lvbiAwKQorKysgTGF5 b3V0VGVzdHMvZWRpdGluZy9zdHlsZS91bmJvbGRpbmctbmVzdGVkLWItZXhwZWN0ZWQudHh0CShy ZXZpc2lvbiAwKQpAQCAtMCwwICsxLDUgQEAKK1RoaXMgdGVzdHMgdW4tYm9sZGluZyBuZXN0ZWQg YiBlbGVtZW50cyBzdWNoIHRoYXQgdGhlIG91dGVyIGIgaGFzIGlkIGF0dHJpYnV0ZS4gV2ViS2l0 IHNob3VsZG4ndCByZW1vdmUgdGhlIG91dGVyIGIuIFNlZSBCdWcgMzAwODMgZm9yIGRldGFpbC4K KworMTIKK0JlZm9yZTo8c3BhbiBpZD0idGVzdCI+PGIgaWQ9ImZvbyI+MTxiPjI8L2I+PC9iPjwv c3Bhbj4KK0FmdGVyOjxzcGFuIGlkPSJ0ZXN0Ij48c3BhbiBpZD0iZm9vIj4xMjwvc3Bhbj48L3Nw YW4+CkluZGV4OiBMYXlvdXRUZXN0cy9lZGl0aW5nL3N0eWxlL3VuYm9sZGluZy1uZXN0ZWQtYi5o dG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2VkaXRpbmcvc3R5bGUvdW5ib2xkaW5nLW5l c3RlZC1iLmh0bWwJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0cy9lZGl0aW5nL3N0eWxlL3Vu Ym9sZGluZy1uZXN0ZWQtYi5odG1sCShyZXZpc2lvbiAwKQpAQCAtMCwwICsxLDI4IEBACis8aHRt bD4KKzxib2R5PgorPHA+VGhpcyB0ZXN0cyB1bi1ib2xkaW5nIG5lc3RlZCBiIGVsZW1lbnRzIHN1 Y2ggdGhhdCB0aGUgb3V0ZXIgYiBoYXMgaWQgYXR0cmlidXRlLiAgV2ViS2l0IHNob3VsZG4ndCBy ZW1vdmUgdGhlIG91dGVyIGIuICBTZWUgPGEgaHJlZj0iaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTMwMDgzIj5CdWcgMzAwODM8L2E+IGZvciBkZXRhaWwuPC9wPgorPGRp diBpZD0iZSIgY29udGVudGVkaXRhYmxlPSJ0cnVlIj48c3BhbiBpZD0idGVzdCI+PGIgaWQ9ImZv byI+MTxiPjI8L2I+PC9iPjwvc3Bhbj48L2Rpdj4KKworPHVsPgorPGxpPkJlZm9yZTo8c3BhbiBp ZD0iYzEiPjwvc3Bhbj48L2xpPgorPGxpPkFmdGVyOjxzcGFuIGlkPSJjMiI+PC9zcGFuPjwvbGk+ Cis8L3VsPgorCis8c2NyaXB0IHR5cGU9InRleHQvamF2YXNjcmlwdCI+CisKK2lmICh3aW5kb3cu bGF5b3V0VGVzdENvbnRyb2xsZXIpCisgICAgbGF5b3V0VGVzdENvbnRyb2xsZXIuZHVtcEFzVGV4 dCgpOworCit2YXIgZSA9IGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKCdlJyk7Citkb2N1bWVudC5n ZXRFbGVtZW50QnlJZCgnYzEnKS5hcHBlbmRDaGlsZChkb2N1bWVudC5jcmVhdGVUZXh0Tm9kZShl LmlubmVySFRNTCkpOworCit2YXIgcyA9IHdpbmRvdy5nZXRTZWxlY3Rpb24oKTsKK3ZhciByID0g ZG9jdW1lbnQuY3JlYXRlUmFuZ2UoKTsKK3Iuc2VsZWN0Tm9kZShkb2N1bWVudC5nZXRFbGVtZW50 QnlJZCgndGVzdCcpKTsKK3MucmVtb3ZlQWxsUmFuZ2VzKCk7CitzLmFkZFJhbmdlKHIpOworZG9j dW1lbnQuZXhlY0NvbW1hbmQoImJvbGQiLCBmYWxzZSwgbnVsbCk7CisKK2RvY3VtZW50LmdldEVs ZW1lbnRCeUlkKCdjMicpLmFwcGVuZENoaWxkKGRvY3VtZW50LmNyZWF0ZVRleHROb2RlKGUuaW5u ZXJIVE1MKSk7CisKKzwvc2NyaXB0Pgo=