Bug 30267 – editing\pasteboard\paste-xml.xhtml should not be skipped for windows

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Victor Wang]

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

[Darin Adler]

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.

[Victor Wang]

Created attachment 40979 [details]
Mac and Win Result Diff

[Victor Wang]

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

[Darin Adler]

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.

[Victor Wang]

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

[Darin Adler]

(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.

[Victor Wang]

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.

[Darin Adler]

(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.

[Victor Wang]

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

[Darin Adler]

(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.

[Darin Adler]

Enrica, Adele, thought you might be interested in this.

[Victor Wang]

Created attachment 41126 [details]
Proposed Patch

[Victor Wang]

Accidentally changed the status. The patch has not been landed yet.

[Victor Wang]

Patch landed: http://trac.webkit.org/changeset/49569.
Close the bug.