30267 2009-10-09 17:11:21 -0700 editing\pasteboard\paste-xml.xhtml should not be skipped for windows 2009-10-14 10:36:58 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 1 victorw webkit-unassigned adele darin enrica oldest_to_newest 153814 0 victorw 2009-10-09 17:11:21 -0700 Hi Darin, I am investigating why "editing\pasteboard\paste-xml.xhtml" fails and is skipped on windows. It looks to me the window's result is correct while the mac version is different because of a workaround. This layout test is skipped because of the patch that creates a fast path for ReplaceSelectionCommand: Here is the patch: http://trac.webkit.org/changeset/42549 see comments in LayoutTests/platform/win/Skipped. The patch only applies to simple text pastes, so pasting xml is handled differently in mac and windows code. Looking into the _documentFragmentFromPasteboard method in WebKit/WebView/WebHTMLView.mm, it appears this is because mac uses a workaround when pasting xml. It checks whether a doc is HTMLDocument if paste type is RTF, and creates a DocumentFragment from text if it is an XML doc. Per comments in the code, _hasHTMLDocument clause is a workaround for a bug in NSAttributedString: Radar 5052369. There is a "FIXME" on removing the check once bug 5052369 is fixed. Since you creates the original test, I would like to check with you before I make any changes. -. Should the mac version be changed to not pasting as simple text in this case? any update on Radar 5052369? My understanding is I can't see the bug details from outside of Apple. -. I think the windows result is correct, we should remove it from skipped list regardless whether or not to keep the mac workaround. Let me know your thoughts and whether I miss anything. Thanks, Victor 153818 1 darin 2009-10-09 17:23:41 -0700 Radar bug 5052369 is a problem on Mac OS X 10.4 Tiger and it is fixed in Mac OS X 10.5 Leopard and newer. So we could consider testing on Leopard or newer without the workaround. But we’d still have to leave it in and test it on Tiger. I can’t see the Windows result that you think is correct, so I can’t give an opinion about whether it’s correct or not. Maybe you could attach a diff to show me what’s different? Ideally, the workaround in WebHTMLView.mm should not cause any observable difference in the result, so if there is a difference we could possibly fix the workaround code so there is no difference. The expected result shows something that is not crashing, and shows the text "foo barbar baz", which seems correct, so I have to assume that the difference in output is something else, not a simple markup difference. 153830 2 40979 victorw 2009-10-09 19:20:23 -0700 Created attachment 40979 Mac and Win Result Diff 153831 3 victorw 2009-10-09 19:21:46 -0700 Thanks Darin for your clarification. I attached the text diff between mac and win. Basically, there are three differences, which I think all are expected: 1. Event message: Mac calls shouldInsertText while win calls shouldInsertNode 2. shouldChangeSelectedDOMRange mac: range from 7 of #text ... win: range from 4 of #text ... 3. text run outputs Diff 1 is because of the work around. Diff 2 & 3 is because of the workaround and the patch for simple text paste: http://trac.webkit.org/changeset/42549 The actual visual results "foo barbar baz" on both win and mac are correct. Since the workaround is still needed for tiger, we probably have to keep different baselines for mac and win. I can fix the win skipped list and baseline once you confirm its result looks ok. Thanks, Victor 154086 4 darin 2009-10-12 09:25:02 -0700 I think the best way to fix this would be to add the same fast case for pasting plain text to the more general pasting code. The more general case should detect that this can be done without creating a new text node. Then aspects (2) and (3) of the test results would be same cross-platform. Item (1) might still require different results per-platform. 154106 5 victorw 2009-10-12 11:06:48 -0700 Darin, Any suggestions on how to detect this can be done without creating new text node? It seems to me that the mac version will also skip the fast path if the _hasHTMLDocument workaround is removed. I am still new to the copy-paste code. It would be great if you can share more details of your thoughts. Thanks, Victor 154118 6 darin 2009-10-12 11:41:58 -0700 (In reply to comment #5) > Any suggestions on how to detect this can be done without creating new text > node? We need to figure out why performTrivialReplace returns in the Windows code path. Once we understand that, we can figure out if we can safely expand performTrivialReplace so it will handle this case. 154146 7 victorw 2009-10-12 13:13:10 -0700 That's because the fragment child is not a text node: if (!fragment.firstChild() || fragment.firstChild() != fragment.lastChild() || !fragment.firstChild()->isTextNode()) Mac version pastes this as text due to the workaround while Windows pasting this as HTML fragment. 154153 8 darin 2009-10-12 13:47:10 -0700 (In reply to comment #7) > That's because the fragment child is not a text node: > if (!fragment.firstChild() || fragment.firstChild() != fragment.lastChild() || > !fragment.firstChild()->isTextNode()) > > Mac version pastes this as text due to the workaround while Windows pasting > this as HTML fragment. The fragment does contain more than just a single text node. But I also think it could qualify for this simple case, because what it contains is simple enough. Probably a single text node inside a simple <div> wrapper — not sure of the details. If we figure out how to make the simple case work when the fragment contains a bit more than just a text node, but nothing that affects the pasting, then we'd have both working the same. On the other hand, we can probably just land results for Windows that expect this different behavior for now. It's a shame to have this using the slow case for no good reason though. 154177 9 victorw 2009-10-12 15:22:19 -0700 Here is the data in Win clipboard: <span class="Apple-style-span" style="background-color: rgba(0, 0, 0, 0); border-collapse: separate; color: rgb(0, 0, 0); font-f amily: 'times new roman'; font-style: normal; font-variant: norm al; font-weight: normal; letter-spacing: normal; line-height: no rmal; orphans: 2; text-align: auto; text-indent: 0px; text-trans form: none; white-space: normal; widows: 2; word-spacing: 0px; - webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-s pacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-t ext-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size : medium; "><span class="Apple-style-span" style="font-size: 24p x; ">bar</span></span> Looks to me the current fast path (performTrivialReplace) simply replaces the string in the text node. The clipboard data has style info, are we able to do simple text paste in this case? The fast path works for this layout test on mac because the fragment and the test node that holds the selection have the same styles (the texts to paste is selected from texts that they will paste to.). My understanding is the style info will be lost on mac for more general cases due to the workaround. Let me know if I misunderstood or miss anything. Thanks, Victor 154182 10 darin 2009-10-12 15:29:25 -0700 (In reply to comment #9) > Looks to me the current fast path (performTrivialReplace) simply > replaces the string in the text node. The clipboard > data has style info, are we able to do simple text paste in this case? This may be too difficult to do, but the idea would be to test that the style is exactly the same as the surrounding text. If it is, then take the fast patch. Given the markup you showed it seems that's going to be a challenge. It would be easy to detect that this is just some text inside some nested span elements. But I can't see how we could efficiently check that none of the style is needed. We'd have to somehow parse the style attributes and check that they match the computed style. It might be possible, but I don't know exactly how you'd have to code it. It also seems like a problem that we have two spans on the clipboard in this case. That's arguably a flaw in the copy code. Anyway, lets land expected results for Windows -- too bad we can't easily fix this to work the same way on all platforms, and can't make this simple case use a simple fast case. 154183 11 darin 2009-10-12 15:30:14 -0700 Enrica, Adele, thought you might be interested in this. 154429 12 41126 victorw 2009-10-13 14:15:25 -0700 Created attachment 41126 Proposed Patch 154463 13 victorw 2009-10-13 15:24:26 -0700 Accidentally changed the status. The patch has not been landed yet. 154604 14 victorw 2009-10-14 10:36:58 -0700 Patch landed: http://trac.webkit.org/changeset/49569. Close the bug. 40979 2009-10-09 19:20:23 -0700 2009-10-09 19:20:23 -0700 Mac and Win Result Diff paste-xml.diff application/octet-stream 2552 victorw IEVESVRJTkcgREVMRUdBVEU6IHdlYlZpZXdEaWRDaGFuZ2VTZWxlY3Rpb246V2ViVmlld0RpZENo YW5nZVNlbGVjdGlvbk5vdGlmaWNhdGlvbgogRURJVElORyBERUxFR0FURTogd2ViVmlld0RpZENo YW5nZVNlbGVjdGlvbjpXZWJWaWV3RGlkQ2hhbmdlU2VsZWN0aW9uTm90aWZpY2F0aW9uCiBFRElU SU5HIERFTEVHQVRFOiB3ZWJWaWV3RGlkQ2hhbmdlU2VsZWN0aW9uOldlYlZpZXdEaWRDaGFuZ2VT ZWxlY3Rpb25Ob3RpZmljYXRpb24KLUVESVRJTkcgREVMRUdBVEU6IHNob3VsZEluc2VydFRleHQ6 YmFyIHJlcGxhY2luZ0RPTVJhbmdlOnJhbmdlIGZyb20gNyBvZiAjdGV4dCA+IHNwYW4gPiBkaXYg PiBib2R5ID4gaHRtbCA+ICNkb2N1bWVudCB0byA3IG9mICN0ZXh0ID4gc3BhbiA+IGRpdiA+IGJv ZHkgPiBodG1sID4gI2RvY3VtZW50IGdpdmVuQWN0aW9uOldlYlZpZXdJbnNlcnRBY3Rpb25QYXN0 ZWQKLUVESVRJTkcgREVMRUdBVEU6IHNob3VsZENoYW5nZVNlbGVjdGVkRE9NUmFuZ2U6cmFuZ2Ug ZnJvbSA3IG9mICN0ZXh0ID4gc3BhbiA+IGRpdiA+IGJvZHkgPiBodG1sID4gI2RvY3VtZW50IHRv IDcgb2YgI3RleHQgPiBzcGFuID4gZGl2ID4gYm9keSA+IGh0bWwgPiAjZG9jdW1lbnQgdG9ET01S YW5nZTpyYW5nZSBmcm9tIDEwIG9mICN0ZXh0ID4gc3BhbiA+IGRpdiA+IGJvZHkgPiBodG1sID4g I2RvY3VtZW50IHRvIDEwIG9mICN0ZXh0ID4gc3BhbiA+IGRpdiA+IGJvZHkgPiBodG1sID4gI2Rv Y3VtZW50IGFmZmluaXR5Ok5TU2VsZWN0aW9uQWZmaW5pdHlEb3duc3RyZWFtIHN0aWxsU2VsZWN0 aW5nOkZBTFNFCitFRElUSU5HIERFTEVHQVRFOiBzaG91bGRJbnNlcnROb2RlOiNkb2N1bWVudC1m cmFnbWVudCByZXBsYWNpbmdET01SYW5nZTpyYW5nZSBmcm9tIDcgb2YgI3RleHQgPiBzcGFuID4g ZGl2ID4gYm9keSA+IGh0bWwgPiAjZG9jdW1lbnQgdG8gNyBvZiAjdGV4dCA+IHNwYW4gPiBkaXYg PiBib2R5ID4gaHRtbCA+ICNkb2N1bWVudCBnaXZlbkFjdGlvbjpXZWJWaWV3SW5zZXJ0QWN0aW9u UGFzdGVkCitFRElUSU5HIERFTEVHQVRFOiBzaG91bGRDaGFuZ2VTZWxlY3RlZERPTVJhbmdlOnJh bmdlIGZyb20gNCBvZiAjdGV4dCA+IHNwYW4gPiBkaXYgPiBib2R5ID4gaHRtbCA+ICNkb2N1bWVu dCB0byA0IG9mICN0ZXh0ID4gc3BhbiA+IGRpdiA+IGJvZHkgPiBodG1sID4gI2RvY3VtZW50IHRv RE9NUmFuZ2U6cmFuZ2UgZnJvbSAzIG9mICN0ZXh0ID4gc3BhbiA+IGRpdiA+IGJvZHkgPiBodG1s ID4gI2RvY3VtZW50IHRvIDMgb2YgI3RleHQgPiBzcGFuID4gZGl2ID4gYm9keSA+IGh0bWwgPiAj ZG9jdW1lbnQgYWZmaW5pdHk6TlNTZWxlY3Rpb25BZmZpbml0eURvd25zdHJlYW0gc3RpbGxTZWxl Y3Rpbmc6RkFMU0UKIEVESVRJTkcgREVMRUdBVEU6IHdlYlZpZXdEaWRDaGFuZ2VTZWxlY3Rpb246 V2ViVmlld0RpZENoYW5nZVNlbGVjdGlvbk5vdGlmaWNhdGlvbgogRURJVElORyBERUxFR0FURTog d2ViVmlld0RpZENoYW5nZTpXZWJWaWV3RGlkQ2hhbmdlTm90aWZpY2F0aW9uCiBsYXllciBhdCAo MCwwKSBzaXplIDgwMHg2MDAKQEAgLTE5LDggKzE5LDEyIEBACiAgIFJlbmRlckJsb2NrIHtodG1s fSBhdCAoMCwwKSBzaXplIDgwMHg3MgogICAgIFJlbmRlckJvZHkge2JvZHl9IGF0ICg4LDgpIHNp emUgNzg0eDU2CiAgICAgICBSZW5kZXJCbG9jayB7ZGl2fSBhdCAoMCwwKSBzaXplIDc4NHg1NiBb Ym9yZGVyOiAoMnB4IHNvbGlkICNGRjAwMDApXQotICAgICAgICBSZW5kZXJJbmxpbmUge3NwYW59 IGF0ICgwLDApIHNpemUgMTQweDI4Ci0gICAgICAgICAgUmVuZGVyVGV4dCB7I3RleHR9IGF0ICgx NCwxNCkgc2l6ZSAxNDB4MjgKLSAgICAgICAgICAgIHRleHQgcnVuIGF0ICgxNCwxNCkgd2lkdGgg MTQwOiAiZm9vIGJhcmJhciBiYXoiCisgICAgICAgIFJlbmRlcklubGluZSB7c3Bhbn0gYXQgKDAs MCkgc2l6ZSAxMzl4MjcKKyAgICAgICAgICBSZW5kZXJUZXh0IHsjdGV4dH0gYXQgKDE0LDE0KSBz aXplIDY5eDI3CisgICAgICAgICAgICB0ZXh0IHJ1biBhdCAoMTQsMTQpIHdpZHRoIDY5OiAiZm9v IGJhciIKKyAgICAgICAgICBSZW5kZXJUZXh0IHsjdGV4dH0gYXQgKDgzLDE0KSBzaXplIDMxeDI3 CisgICAgICAgICAgICB0ZXh0IHJ1biBhdCAoODMsMTQpIHdpZHRoIDMxOiAiYmFyIgorICAgICAg ICAgIFJlbmRlclRleHQgeyN0ZXh0fSBhdCAoMTE0LDE0KSBzaXplIDM5eDI3CisgICAgICAgICAg ICB0ZXh0IHJ1biBhdCAoMTE0LDE0KSB3aWR0aCAzOTogIiBiYXoiCiAgICAgICAgIFJlbmRlclRl eHQgeyN0ZXh0fSBhdCAoMCwwKSBzaXplIDB4MAotY2FyZXQ6IHBvc2l0aW9uIDEwIG9mIGNoaWxk IDAgeyN0ZXh0fSBvZiBjaGlsZCAxIHtzcGFufSBvZiBjaGlsZCAxIHtkaXZ9IG9mIGNoaWxkIDMg e2JvZHl9IG9mIGNoaWxkIDEge2h0bWx9IG9mIGRvY3VtZW50CitjYXJldDogcG9zaXRpb24gMyBv ZiBjaGlsZCAxIHsjdGV4dH0gb2YgY2hpbGQgMSB7c3Bhbn0gb2YgY2hpbGQgMSB7ZGl2fSBvZiBj aGlsZCAzIHtib2R5fSBvZiBjaGlsZCAxIHtodG1sfSBvZiBkb2N1bWVudAo= 41126 2009-10-13 14:15:25 -0700 2009-10-13 14:22:36 -0700 Proposed Patch paste-xml.patch text/plain 2821 victorw SW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9D aGFuZ2VMb2cJKHJldmlzaW9uIDQ5NTA4KQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTggQEAKKzIwMDktMTAtMTMgIFZpY3RvciBXYW5nICA8dmlj dG9yd0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgQWRkIHBhc3RlLXhtbCB0ZXN0IGJhY2sgZm9yIFdpbi4KKyAgICAgICAgVGhl IHJlc3VsdCBkaWZmZXJlbmNlIGJldHdlZW4gTWFjIGFuZCBXaW4gaXMgZHVlIHRvIHRoZQorICAg ICAgICBwbGF0Zm9ybSBkZXBlbmRlbnQgaW1wbGVtZW50YXRpb24uIFRoZSBkaWZmIGlzIGV4cGVj dGVkCisgICAgICAgIHdpdGggY3VycmVudCBpbXBsZW1lbnRhdGlvbi4KKyAgICAgICAgU2VlIGJ1 ZyBmb3IgbW9yZSBkZWFpbHM6CisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTMwMjY3CisKKyAgICAgICAgKiBwbGF0Zm9ybS93aW4vU2tpcHBlZDoKKyAg ICAgICAgKiBwbGF0Zm9ybS93aW4vZWRpdGluZy9wYXN0ZWJvYXJkL3Bhc3RlLXhtbC1leHBlY3Rl ZC50eHQ6IEFkZGVkLgorCiAyMDA5LTEwLTEzICBNaWNoZWxhbmdlbG8gRGUgU2ltb25lICA8bWlj ZGVzaW1AZ21haWwuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYW0gQmFydGguCkluZGV4 OiBMYXlvdXRUZXN0cy9wbGF0Zm9ybS93aW4vU2tpcHBlZAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRU ZXN0cy9wbGF0Zm9ybS93aW4vU2tpcHBlZAkocmV2aXNpb24gNDk1MDUpCisrKyBMYXlvdXRUZXN0 cy9wbGF0Zm9ybS93aW4vU2tpcHBlZAkod29ya2luZyBjb3B5KQpAQCAtOTUsOSArOTUsNiBAQCBz dmcvaGl4aWUvdmlld2JveC9wcmVzZXJ2ZUFzcGVjdFJhdGlvLzAwCiAjIEFTU0VSVCBpbiBDbGlw Ym9hcmRXaW46OnNldERhdGEgPHJkYXI6Ly9wcm9ibGVtLzUwMTU5NDE+CiBlZGl0aW5nL2V4ZWND b21tYW5kL2NvcHktd2l0aG91dC1zZWxlY3Rpb24uaHRtbAogCi0jIDxyZGFyOi8vcHJvYmxlbS82 Nzk0ODgwPiBlZGl0aW5nL3Bhc3RlYm9hcmQvcGFzdGUteG1sLnhodG1sIHJlc3VsdHMgZGlkbid0 IGNoYW5nZSBvbiBXaW5kb3dzIGFmdGVyIHI0MjU0OQotZWRpdGluZy9wYXN0ZWJvYXJkL3Bhc3Rl LXhtbC54aHRtbAotCiAjIERyYWcgJiBEcm9wIGRvZXNuJ3Qgd29yayBjb3JyZWN0bHkgaW4gRFJU IDxyZGFyOi8vNTYyMTI0ND4KIGVkaXRpbmcvcGFzdGVib2FyZC9kcmFnLWltYWdlLWluLWFib3V0 LWJsYW5rLWZyYW1lLmh0bWwKIGVkaXRpbmcvcGFzdGVib2FyZC9nZXQtZGF0YS10ZXh0LXBsYWlu LWRyb3AuaHRtbApJbmRleDogTGF5b3V0VGVzdHMvcGxhdGZvcm0vd2luL2VkaXRpbmcvcGFzdGVi b2FyZC9wYXN0ZS14bWwtZXhwZWN0ZWQudHh0Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL3Bs YXRmb3JtL3dpbi9lZGl0aW5nL3Bhc3RlYm9hcmQvcGFzdGUteG1sLWV4cGVjdGVkLnR4dAkocmV2 aXNpb24gMCkKKysrIExheW91dFRlc3RzL3BsYXRmb3JtL3dpbi9lZGl0aW5nL3Bhc3RlYm9hcmQv cGFzdGUteG1sLWV4cGVjdGVkLnR4dAkocmV2aXNpb24gMCkKQEAgLTAsMCArMSwxNSBAQAorbGF5 ZXIgYXQgKDAsMCkgc2l6ZSA4MDB4NjAwCisgIFJlbmRlclZpZXcgYXQgKDAsMCkgc2l6ZSA4MDB4 NjAwCitsYXllciBhdCAoMCwwKSBzaXplIDgwMHg3MgorICBSZW5kZXJCbG9jayB7aHRtbH0gYXQg KDAsMCkgc2l6ZSA4MDB4NzIKKyAgICBSZW5kZXJCb2R5IHtib2R5fSBhdCAoOCw4KSBzaXplIDc4 NHg1NgorICAgICAgUmVuZGVyQmxvY2sge2Rpdn0gYXQgKDAsMCkgc2l6ZSA3ODR4NTYgW2JvcmRl cjogKDJweCBzb2xpZCAjRkYwMDAwKV0KKyAgICAgICAgUmVuZGVySW5saW5lIHtzcGFufSBhdCAo MCwwKSBzaXplIDE0MHgyOAorICAgICAgICAgIFJlbmRlclRleHQgeyN0ZXh0fSBhdCAoMTQsMTQp IHNpemUgNjl4MjgKKyAgICAgICAgICAgIHRleHQgcnVuIGF0ICgxNCwxNCkgd2lkdGggNjk6ICJm b28gYmFyIgorICAgICAgICAgIFJlbmRlclRleHQgeyN0ZXh0fSBhdCAoODMsMTQpIHNpemUgMzF4 MjgKKyAgICAgICAgICAgIHRleHQgcnVuIGF0ICg4MywxNCkgd2lkdGggMzE6ICJiYXIiCisgICAg ICAgICAgUmVuZGVyVGV4dCB7I3RleHR9IGF0ICgxMTQsMTQpIHNpemUgNDB4MjgKKyAgICAgICAg ICAgIHRleHQgcnVuIGF0ICgxMTQsMTQpIHdpZHRoIDQwOiAiIGJheiIKKyAgICAgICAgUmVuZGVy VGV4dCB7I3RleHR9IGF0ICgwLDApIHNpemUgMHgwCitjYXJldDogcG9zaXRpb24gMyBvZiBjaGls ZCAxIHsjdGV4dH0gb2YgY2hpbGQgMSB7c3Bhbn0gb2YgY2hpbGQgMSB7ZGl2fSBvZiBjaGlsZCAz IHtib2R5fSBvZiBjaGlsZCAxIHtodG1sfSBvZiBkb2N1bWVudAoKUHJvcGVydHkgY2hhbmdlcyBv bjogTGF5b3V0VGVzdHMvcGxhdGZvcm0vd2luL2VkaXRpbmcvcGFzdGVib2FyZC9wYXN0ZS14bWwt ZXhwZWN0ZWQudHh0Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX18KTmFtZTogc3ZuOmV4ZWN1dGFibGUKICAgKyAqCk5hbWU6 IHN2bjplb2wtc3R5bGUKICAgKyBuYXRpdmUKCg==