Bug 30655 – Only plain text should be copied to clipboard for TextArea.

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

[Jian Li]

For TextArea, we should only copy plain text to the clipboard, not both plain text and HTML text. This will fix the problem we cannot copy and paste the data from the spreadsheet web application to the office application, like Excel or OpenOffice Calc.

[Jian Li]

Created attachment 41705 [details]
Proposed Patch

[Eric Seidel (no email)]

Wasn't there just a discussion about this sort of thing on webkit-dev?  I guess <textarea> only has plain text data...

[Peter Kasting]

Can this affect contentEditable at all?  I just want to make sure we don't stop copying rich text in some place where we should.

[Jian Li]

(In reply to comment #2)
> Wasn't there just a discussion about this sort of thing on webkit-dev?  I guess
> <textarea> only has plain text data...

I am not aware of such discussion. Yes, the textarea should only contain plain text.

[Jian Li]

(In reply to comment #3)
> Can this affect contentEditable at all?  I just want to make sure we don't stop
> copying rich text in some place where we should.

I do not think it will affect contentEditable enabled elements since it only checks for TextArea or text input element that only contains plain text. If you have something else in mind, could you please tell me so that I can do the check.

[Darin Adler]

Comment on attachment 41705 [details]
Proposed Patch

It's wrong to change copy, but not cut. This same situation can arise with cut. This should be in shared code used by both cut and copy.

This breaks the Mac platform because it removes the call to didWriteSelectionToPasteboard for this case. Other platforms treat that as a no-op. (For some reason they all call notImplemented, which seems unnecessary).

> +    if (target && target->isCharacterDataNode()) {

I don't think it's helpful to check isCharacterDataNode here. None of the code inside the if statement depends on the type of the node. And it's almost never helpful to check isCharacterDataNode instead of isTextNode. The nodes in this case are going to be text nodes. But there's no need to depend on this implementation detail. Later we may decide we want to use some type of element inside textarea for some purpose. I suggest you just remove that check entirely; the rest of the code should work fine.

review- because of the first two problems above.

[Jian Li]

(In reply to comment #6)
> (From update of attachment 41705 [details])
> It's wrong to change copy, but not cut. This same situation can arise with cut.
> This should be in shared code used by both cut and copy.
> 
> This breaks the Mac platform because it removes the call to
> didWriteSelectionToPasteboard for this case. Other platforms treat that as a
> no-op. (For some reason they all call notImplemented, which seems unnecessary).
> 

Good catch. I will fix both issues.

> > +    if (target && target->isCharacterDataNode()) {
> 
> I don't think it's helpful to check isCharacterDataNode here. None of the code
> inside the if statement depends on the type of the node. And it's almost never
> helpful to check isCharacterDataNode instead of isTextNode. The nodes in this
> case are going to be text nodes. But there's no need to depend on this
> implementation detail. Later we may decide we want to use some type of element
> inside textarea for some purpose. I suggest you just remove that check
> entirely; the rest of the code should work fine.
> 

Indeed I did not add the check for isCharacterDataNode in my first version. However, it broke LayoutTests/editing/pasteboard/input-field-1.html. This layout test tests the copy/paste of a text input field: it selects the whole text input element and assumes success if the whole text input element is copied as HTML.

If we do not do the check for isCharacterDataNode, we will need to change the behavior of this layout test. How do you think?

> review- because of the first two problems above.

[Darin Adler]

(In reply to comment #7)
> Indeed I did not add the check for isCharacterDataNode in my first version.
> However, it broke LayoutTests/editing/pasteboard/input-field-1.html. This
> layout test tests the copy/paste of a text input field: it selects the whole
> text input element and assumes success if the whole text input element is
> copied as HTML.
> 
> If we do not do the check for isCharacterDataNode, we will need to change the
> behavior of this layout test. How do you think?

I suggest you factor this out into a separate function. The right way to handle this case is to either:

    1) Check for cases where the node is == the shadowAncestorNode, and don't do the special handling in that case.

---
    static bool nodeIsInTextFormControl(Node* node)
    {
        if (!node)
            return false;
        Node* ancestor = node->shadowAncestorNode();
        if (ancestor == node)
            return false;
        return ancestor->isElementNode() && static_cast<Element*>(ancestor)->isTextFormControl();
    }
---

    2) Call shadowTreeRootNode, check for 0, and then call shadowParentNode, instead of calling shadowAncestorNode.

---
    static bool nodeIsInTextFormControl(Node* node)
    {
        if (!node)
            return false;
        Node* root = target->shadowTreeRootNode();
        if (!root)
            return false;
        Node* parent = root->shadowParentNode();
        return parent && parent->isElementNode() && static_cast<Element*>(parent)->isTextFormControl();
    }
---

It’s not right to special-case text nodes or character data nodes, and not needed.

[Jian Li]

Created attachment 42078 [details]
Proposed Patch

Thanks a lot. I fixed all issues.

[Jian Li]

Committed as http://trac.webkit.org/changeset/50279.

[Sam Weinig]

Is there a way this can be tested?  One way I can think of doing it is modifying DRT to dump the type/content of the pasteboard.  Perhaps there is a way to do by pasting the content that is being copied, I just don't know.

[Jian Li]

(In reply to comment #11)
> Is there a way this can be tested?  One way I can think of doing it is
> modifying DRT to dump the type/content of the pasteboard.  Perhaps there is a
> way to do by pasting the content that is being copied, I just don't know.

I've thought about this but could not find an easy way to write the test within current DRT infrastructure. How do we encapsulate the content in the platform-dependent pasteboard/clipboard? Maybe we can define a PasteboardController that could be used to dump the content of certain type, say "text" and "html".

[Sam Weinig]

Indeed, I filed bug 30945 to do that.