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
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.
Created attachment 40979 [details] Mac and Win Result Diff
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
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.
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
(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.
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.
(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.
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
(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.
Enrica, Adele, thought you might be interested in this.
Created attachment 41126 [details] Proposed Patch
Accidentally changed the status. The patch has not been landed yet.
Patch landed: http://trac.webkit.org/changeset/49569. Close the bug.