Bug 30655 - Only plain text should be copied to clipboard for TextArea.
Summary: Only plain text should be copied to clipboard for TextArea.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-21 18:30 PDT by Jian Li
Modified: 2009-10-29 22:07 PDT (History)
5 users (show)

See Also:


Attachments
Proposed Patch (1.46 KB, patch)
2009-10-22 17:50 PDT, Jian Li
darin: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (2.84 KB, patch)
2009-10-28 18:43 PDT, Jian Li
darin: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-10-21 18:30:34 PDT
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.
Comment 1 Jian Li 2009-10-22 17:50:43 PDT
Created attachment 41705 [details]
Proposed Patch
Comment 2 Eric Seidel (no email) 2009-10-23 12:15:07 PDT
Wasn't there just a discussion about this sort of thing on webkit-dev?  I guess <textarea> only has plain text data...
Comment 3 Peter Kasting 2009-10-23 12:19:06 PDT
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.
Comment 4 Jian Li 2009-10-23 12:50:30 PDT
(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.
Comment 5 Jian Li 2009-10-23 12:53:16 PDT
(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.
Comment 6 Darin Adler 2009-10-25 15:37:44 PDT
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.
Comment 7 Jian Li 2009-10-26 10:06:24 PDT
(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.
Comment 8 Darin Adler 2009-10-28 18:00:23 PDT
(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.
Comment 9 Jian Li 2009-10-28 18:43:26 PDT
Created attachment 42078 [details]
Proposed Patch

Thanks a lot. I fixed all issues.
Comment 10 Jian Li 2009-10-29 10:19:29 PDT
Committed as http://trac.webkit.org/changeset/50279.
Comment 11 Sam Weinig 2009-10-29 11:37:46 PDT
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.
Comment 12 Jian Li 2009-10-29 21:53:18 PDT
(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".
Comment 13 Sam Weinig 2009-10-29 22:07:02 PDT
Indeed, I filed bug 30945 to do that.