30655 2009-10-21 18:30:34 -0700 Only plain text should be copied to clipboard for TextArea. 2009-10-29 22:07:02 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 jianli jianli darin eric ojan pkasting sam oldest_to_newest 156746 0 jianli 2009-10-21 18:30:34 -0700 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. 157068 1 41705 jianli 2009-10-22 17:50:43 -0700 Created attachment 41705 Proposed Patch 157249 2 eric 2009-10-23 12:15:07 -0700 Wasn't there just a discussion about this sort of thing on webkit-dev? I guess <textarea> only has plain text data... 157250 3 pkasting 2009-10-23 12:19:06 -0700 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. 157261 4 jianli 2009-10-23 12:50:30 -0700 (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. 157262 5 jianli 2009-10-23 12:53:16 -0700 (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. 157628 6 41705 darin 2009-10-25 15:37:44 -0700 Comment on attachment 41705 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. 157806 7 jianli 2009-10-26 10:06:24 -0700 (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. 158798 8 darin 2009-10-28 18:00:23 -0700 (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. 158814 9 42078 jianli 2009-10-28 18:43:26 -0700 Created attachment 42078 Proposed Patch Thanks a lot. I fixed all issues. 158937 10 jianli 2009-10-29 10:19:29 -0700 Committed as http://trac.webkit.org/changeset/50279. 158986 11 sam 2009-10-29 11:37:46 -0700 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. 159220 12 jianli 2009-10-29 21:53:18 -0700 (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". 159223 13 sam 2009-10-29 22:07:02 -0700 Indeed, I filed bug 30945 to do that. 41705 2009-10-22 17:50:43 -0700 2009-10-28 18:43:26 -0700 Proposed Patch 30655 text/plain 1492 jianli ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZGY2M2NmNS4uNWFiZWIzMyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxMyBAQAorMjAwOS0xMC0yMiAgSmlhbiBMaSAgPGpp YW5saUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgQnVnIDMwNjU1IC0gT25seSBwbGFpbiB0ZXh0IHNob3VsZCBiZSBjb3BpZWQg dG8gY2xpcGJvYXJkIGZvciBUZXh0QXJlYS4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTMwNjU1CisKKyAgICAgICAgKiBlZGl0aW5nL0VkaXRvci5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpFZGl0b3I6OmNvcHkpOgorCiAyMDA5LTEwLTIxICBEdW1pdHJ1 IERhbmlsaXVjICA8ZHVtaUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRGlt aXRyaSBHbGF6a292LgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9lZGl0aW5nL0VkaXRvci5jcHAgYi9X ZWJDb3JlL2VkaXRpbmcvRWRpdG9yLmNwcAppbmRleCAzZjNmNzM2Li4xZDI5NGI4IDEwMDY0NAot LS0gYS9XZWJDb3JlL2VkaXRpbmcvRWRpdG9yLmNwcAorKysgYi9XZWJDb3JlL2VkaXRpbmcvRWRp dG9yLmNwcApAQCAtMTAyNyw3ICsxMDI3LDE3IEBAIHZvaWQgRWRpdG9yOjpjb3B5KCkKICAgICAg ICAgc3lzdGVtQmVlcCgpOwogICAgICAgICByZXR1cm47CiAgICAgfQotICAgIAorCisgICAgLy8g SWYgdGhlIHRhcmdldCBpcyB0aGUgY2hhcmFjdGVycyBpbnNpZGUgYSB0ZXh0IGNvbnRyb2wsIGNv cHkgYXMgcGxhaW4gdGV4dCBvbmx5LgorICAgIE5vZGUqIHRhcmdldCA9IG1fZnJhbWUtPnNlbGVj dGlvbigpLT5zdGFydCgpLm5vZGUoKTsKKyAgICBpZiAodGFyZ2V0ICYmIHRhcmdldC0+aXNDaGFy YWN0ZXJEYXRhTm9kZSgpKSB7CisgICAgICAgIHRhcmdldCA9IHRhcmdldC0+c2hhZG93QW5jZXN0 b3JOb2RlKCk7CisgICAgICAgIGlmICh0YXJnZXQtPmlzRWxlbWVudE5vZGUoKSAmJiBzdGF0aWNf Y2FzdDxFbGVtZW50Kj4odGFyZ2V0KS0+aXNUZXh0Rm9ybUNvbnRyb2woKSkgeworICAgICAgICAg ICAgUGFzdGVib2FyZDo6Z2VuZXJhbFBhc3RlYm9hcmQoKS0+d3JpdGVQbGFpblRleHQobV9mcmFt ZS0+c2VsZWN0ZWRUZXh0KCkpOworICAgICAgICAgICAgcmV0dXJuOworICAgICAgICB9CisgICAg fQorCiAgICAgRG9jdW1lbnQqIGRvY3VtZW50ID0gbV9mcmFtZS0+ZG9jdW1lbnQoKTsKICAgICBp ZiAoSFRNTEltYWdlRWxlbWVudCogaW1hZ2VFbGVtZW50ID0gaW1hZ2VFbGVtZW50RnJvbUltYWdl RG9jdW1lbnQoZG9jdW1lbnQpKQogICAgICAgICBQYXN0ZWJvYXJkOjpnZW5lcmFsUGFzdGVib2Fy ZCgpLT53cml0ZUltYWdlKGltYWdlRWxlbWVudCwgZG9jdW1lbnQtPnVybCgpLCBkb2N1bWVudC0+ dGl0bGUoKSk7Cg== 42078 2009-10-28 18:43:26 -0700 2009-10-28 20:20:41 -0700 Proposed Patch 30655_2 text/plain 2909 jianli ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg ZGY2M2NmNS4uNmIxM2QyNiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNSBAQAorMjAwOS0xMC0yMiAgSmlhbiBMaSAgPGpp YW5saUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgQnVnIDMwNjU1IC0gT25seSBwbGFpbiB0ZXh0IHNob3VsZCBiZSBjb3BpZWQg dG8gY2xpcGJvYXJkIGZvciBUZXh0QXJlYS4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTMwNjU1CisKKyAgICAgICAgKiBlZGl0aW5nL0VkaXRvci5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpub2RlSXNJblRleHRGb3JtQ29udHJvbCk6CisgICAgICAgIChX ZWJDb3JlOjpFZGl0b3I6OmN1dCk6CisgICAgICAgIChXZWJDb3JlOjpFZGl0b3I6OmNvcHkpOgor CiAyMDA5LTEwLTIxICBEdW1pdHJ1IERhbmlsaXVjICA8ZHVtaUBjaHJvbWl1bS5vcmc+CiAKICAg ICAgICAgUmV2aWV3ZWQgYnkgRGltaXRyaSBHbGF6a292LgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9l ZGl0aW5nL0VkaXRvci5jcHAgYi9XZWJDb3JlL2VkaXRpbmcvRWRpdG9yLmNwcAppbmRleCAzZjNm NzM2Li4yMmNiYTAxIDEwMDY0NAotLS0gYS9XZWJDb3JlL2VkaXRpbmcvRWRpdG9yLmNwcAorKysg Yi9XZWJDb3JlL2VkaXRpbmcvRWRpdG9yLmNwcApAQCAtMTAwMyw2ICsxMDAzLDE2IEBAIGJvb2wg RWRpdG9yOjppbnNlcnRQYXJhZ3JhcGhTZXBhcmF0b3IoKQogICAgIHJldHVybiB0cnVlOwogfQog CitzdGF0aWMgYm9vbCBub2RlSXNJblRleHRGb3JtQ29udHJvbChOb2RlKiBub2RlKQoreworICAg IGlmICghbm9kZSkKKyAgICAgICAgcmV0dXJuIGZhbHNlOworICAgIE5vZGUqIGFuY2VzdG9yID0g bm9kZS0+c2hhZG93QW5jZXN0b3JOb2RlKCk7CisgICAgaWYgKGFuY2VzdG9yID09IG5vZGUpCisg ICAgICAgIHJldHVybiBmYWxzZTsKKyAgICByZXR1cm4gYW5jZXN0b3ItPmlzRWxlbWVudE5vZGUo KSAmJiBzdGF0aWNfY2FzdDxFbGVtZW50Kj4oYW5jZXN0b3IpLT5pc1RleHRGb3JtQ29udHJvbCgp OworfQorCiB2b2lkIEVkaXRvcjo6Y3V0KCkKIHsKICAgICBpZiAodHJ5REhUTUxDdXQoKSkKQEAg LTEwMTMsNyArMTAyMywxMCBAQCB2b2lkIEVkaXRvcjo6Y3V0KCkKICAgICB9CiAgICAgUmVmUHRy PFJhbmdlPiBzZWxlY3Rpb24gPSBzZWxlY3RlZFJhbmdlKCk7CiAgICAgaWYgKHNob3VsZERlbGV0 ZVJhbmdlKHNlbGVjdGlvbi5nZXQoKSkpIHsKLSAgICAgICAgUGFzdGVib2FyZDo6Z2VuZXJhbFBh c3RlYm9hcmQoKS0+d3JpdGVTZWxlY3Rpb24oc2VsZWN0aW9uLmdldCgpLCBjYW5TbWFydENvcHlP ckRlbGV0ZSgpLCBtX2ZyYW1lKTsKKyAgICAgICAgaWYgKG5vZGVJc0luVGV4dEZvcm1Db250cm9s KG1fZnJhbWUtPnNlbGVjdGlvbigpLT5zdGFydCgpLm5vZGUoKSkpCisgICAgICAgICAgICBQYXN0 ZWJvYXJkOjpnZW5lcmFsUGFzdGVib2FyZCgpLT53cml0ZVBsYWluVGV4dChtX2ZyYW1lLT5zZWxl Y3RlZFRleHQoKSk7CisgICAgICAgIGVsc2UKKyAgICAgICAgICAgIFBhc3RlYm9hcmQ6OmdlbmVy YWxQYXN0ZWJvYXJkKCktPndyaXRlU2VsZWN0aW9uKHNlbGVjdGlvbi5nZXQoKSwgY2FuU21hcnRD b3B5T3JEZWxldGUoKSwgbV9mcmFtZSk7CiAgICAgICAgIGRpZFdyaXRlU2VsZWN0aW9uVG9QYXN0 ZWJvYXJkKCk7CiAgICAgICAgIGRlbGV0ZVNlbGVjdGlvbldpdGhTbWFydERlbGV0ZShjYW5TbWFy dENvcHlPckRlbGV0ZSgpKTsKICAgICB9CkBAIC0xMDI3LDEzICsxMDQwLDE3IEBAIHZvaWQgRWRp dG9yOjpjb3B5KCkKICAgICAgICAgc3lzdGVtQmVlcCgpOwogICAgICAgICByZXR1cm47CiAgICAg fQotICAgIAotICAgIERvY3VtZW50KiBkb2N1bWVudCA9IG1fZnJhbWUtPmRvY3VtZW50KCk7Ci0g ICAgaWYgKEhUTUxJbWFnZUVsZW1lbnQqIGltYWdlRWxlbWVudCA9IGltYWdlRWxlbWVudEZyb21J bWFnZURvY3VtZW50KGRvY3VtZW50KSkKLSAgICAgICAgUGFzdGVib2FyZDo6Z2VuZXJhbFBhc3Rl Ym9hcmQoKS0+d3JpdGVJbWFnZShpbWFnZUVsZW1lbnQsIGRvY3VtZW50LT51cmwoKSwgZG9jdW1l bnQtPnRpdGxlKCkpOwotICAgIGVsc2UKLSAgICAgICAgUGFzdGVib2FyZDo6Z2VuZXJhbFBhc3Rl Ym9hcmQoKS0+d3JpdGVTZWxlY3Rpb24oc2VsZWN0ZWRSYW5nZSgpLmdldCgpLCBjYW5TbWFydENv cHlPckRlbGV0ZSgpLCBtX2ZyYW1lKTsKLSAgICAKKworICAgIGlmIChub2RlSXNJblRleHRGb3Jt Q29udHJvbChtX2ZyYW1lLT5zZWxlY3Rpb24oKS0+c3RhcnQoKS5ub2RlKCkpKQorICAgICAgICBQ YXN0ZWJvYXJkOjpnZW5lcmFsUGFzdGVib2FyZCgpLT53cml0ZVBsYWluVGV4dChtX2ZyYW1lLT5z ZWxlY3RlZFRleHQoKSk7CisgICAgZWxzZSB7CisgICAgICAgIERvY3VtZW50KiBkb2N1bWVudCA9 IG1fZnJhbWUtPmRvY3VtZW50KCk7CisgICAgICAgIGlmIChIVE1MSW1hZ2VFbGVtZW50KiBpbWFn ZUVsZW1lbnQgPSBpbWFnZUVsZW1lbnRGcm9tSW1hZ2VEb2N1bWVudChkb2N1bWVudCkpCisgICAg ICAgICAgICBQYXN0ZWJvYXJkOjpnZW5lcmFsUGFzdGVib2FyZCgpLT53cml0ZUltYWdlKGltYWdl RWxlbWVudCwgZG9jdW1lbnQtPnVybCgpLCBkb2N1bWVudC0+dGl0bGUoKSk7CisgICAgICAgIGVs c2UKKyAgICAgICAgICAgIFBhc3RlYm9hcmQ6OmdlbmVyYWxQYXN0ZWJvYXJkKCktPndyaXRlU2Vs ZWN0aW9uKHNlbGVjdGVkUmFuZ2UoKS5nZXQoKSwgY2FuU21hcnRDb3B5T3JEZWxldGUoKSwgbV9m cmFtZSk7CisgICAgfQorCiAgICAgZGlkV3JpdGVTZWxlY3Rpb25Ub1Bhc3RlYm9hcmQoKTsKIH0K IAo=