56472 2011-03-16 11:14:45 -0700 Node::isContentEditable should always call parentNode() instead of parentOrHostNode() 2011-03-16 14:09:10 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) All All RESOLVED FIXED P2 Enhancement --- 1 rniwa rniwa darin dglazkov justin.garcia oldest_to_newest 368403 0 rniwa 2011-03-16 11:14:45 -0700 Right now, Node::isContentEditable calls parentOrHostNode() when the node is not an HTML element or document. However, this is unintentional. The original patch that added HTMLElement::isContentEditable was: http://trac.webkit.org/changeset/15643 and at this revision, parentNode() was identical to parent(): http://trac.webkit.org/browser/trunk/WebCore/dom/Node.h?rev=15643#L90 However, the behavior of parent() was changed in http://trac.webkit.org/changeset/73618, which resulted in unfortunate deviation of code paths. While Internet Explorer lets user edit readonly input element's content when the element is inside a contenteditable region, it's not true that we always like to make shadow DOM elements editable when the host is editable. So we should probably special-treat readonly elements instead if we want to match that behavior and not by calling parentOrHostNode() in isContentEditable(). Related change: http://trac.webkit.org/changeset/10945 http://trac.webkit.org/changeset/15643 http://trac.webkit.org/changeset/73618 368425 1 85948 rniwa 2011-03-16 11:36:00 -0700 Created attachment 85948 cleanup 368428 2 85948 dglazkov 2011-03-16 11:37:49 -0700 Comment on attachment 85948 cleanup ok. 368484 3 85948 darin 2011-03-16 13:20:14 -0700 Comment on attachment 85948 cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=85948&action=review > Source/WebCore/dom/Node.cpp:724 > + if ((node->isHTMLElement() || node->isDocumentNode()) && node->renderer()) { I’d use continue for this instead of nesting the body of the loop inside an if. Think of it as “early return” style. 368490 4 85948 rniwa 2011-03-16 13:29:18 -0700 Comment on attachment 85948 cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=85948&action=review >> Source/WebCore/dom/Node.cpp:724 >> + if ((node->isHTMLElement() || node->isDocumentNode()) && node->renderer()) { > > I’d use continue for this instead of nesting the body of the loop inside an if. Think of it as “early return” style. I'm not sure if that's clearer. for (const Node* node = this; node; node = node->parentNode()) { if ((node->isHTMLElement() || node->isDocumentNode()) && node->renderer()) { if (editableLevel == RichlyEditable) return node->renderer()->style()->userModify() == READ_WRITE; EUserModify userModify = node->renderer()->style()->userModify(); return userModify == READ_WRITE || userModify == READ_WRITE_PLAINTEXT_ONLY; } } for (const Node* node = this; node; node = node->parentNode()) { if ((!node->isHTMLElement() && !node->isDocumentNode()) || !node->renderer()) continue; if (editableLevel == RichlyEditable) return node->renderer()->style()->userModify() == READ_WRITE; EUserModify userModify = node->renderer()->style()->userModify(); return userModify == READ_WRITE || userModify == READ_WRITE_PLAINTEXT_ONLY; } The latter seems confusing to me because the normal flow results in return. 368492 5 darin 2011-03-16 13:30:40 -0700 Your call. 368493 6 rniwa 2011-03-16 13:32:34 -0700 Maybe a switch statement works better here: switch (node->renderer()->style()->userModify()) { case READ_ONLY: return false; case READ_WRITE: return true; case READ_WRITE_PLAINTEXT_ONLY: return editableLevel != RichlyEditable; } ASSERT_NOT_REACHED(); return false; 368494 7 rniwa 2011-03-16 13:34:25 -0700 This looks slightly better than other variants. And there's a slight performance improvement. for (const Node* node = this; node; node = node->parentNode()) { if ((node->isHTMLElement() || node->isDocumentNode()) && node->renderer()) { switch (node->renderer()->style()->userModify()) { case READ_ONLY: return false; case READ_WRITE: return true; case READ_WRITE_PLAINTEXT_ONLY: return editableLevel != RichlyEditable; } ASSERT_NOT_REACHED(); return false; } } 368499 8 darin 2011-03-16 13:39:16 -0700 But put the return statements on separate lines. There’s no need to do it all in one line. 368503 9 85965 rniwa 2011-03-16 13:42:22 -0700 Created attachment 85965 use switch statement instead of ifs 368505 10 darin 2011-03-16 13:43:23 -0700 Another thought: This function can, at some point, be moved out of the Node class and into a free function in some source file int he editing directory. It doesn’t need to be a virtual function, and it doesn’t need access to anything private. So those reasons for being a member function don’t apply. The only reason I can think of is “I like the syntax”. And the function’s purpose is about editing, not the fundamental properties of DOM nodes, so there is a modularity reason to not have it be a member of the Node class. 368508 11 rniwa 2011-03-16 13:46:57 -0700 (In reply to comment #10) > This function can, at some point, be moved out of the Node class and into a free function in some source file int he editing directory. > > It doesn’t need to be a virtual function, and it doesn’t need access to anything private. So those reasons for being a member function don’t apply. The only reason I can think of is “I like the syntax”. > > And the function’s purpose is about editing, not the fundamental properties of DOM nodes, so there is a modularity reason to not have it be a member of the Node class. Yeah. But I don't like the fact we already have so many global editing functions (e.g. ones in htmlediting.cpp) without much organizations. At some point, we need to find a way to bundle those functions together into a namespace, class, etc... because it's needlessly polluting the entire WebCore namespace as of now. 368509 12 darin 2011-03-16 13:47:47 -0700 (In reply to comment #11) > But I don't like the fact we already have so many global editing functions (e.g. ones in htmlediting.cpp) without much organizations. At some point, we need to find a way to bundle those functions together into a namespace, class, etc... because it's needlessly polluting the entire WebCore namespace as of now. Organizing the editing functions is a good idea. But mixing editing in to the basic DOM is a far worse problem! 368511 13 rniwa 2011-03-16 13:52:43 -0700 (In reply to comment #12) > Organizing the editing functions is a good idea. But mixing editing in to the basic DOM is a far worse problem! But "contenteditable" is a DOM property though (i.e. we have HTMLElement::contentEditable() and HTMLElement::setContentEditable) although I admit that Node::isContentEditable is about whether or not a node is editable and slightly different from HTMLElement::contentEditable(). 368515 14 darin 2011-03-16 13:55:09 -0700 (In reply to comment #13) > But "contenteditable" is a DOM property though (i.e. we have HTMLElement::contentEditable() and HTMLElement::setContentEditable) although I admit that Node::isContentEditable is about whether or not a node is editable and slightly different from HTMLElement::contentEditable(). This is exactly my point! 368527 15 rniwa 2011-03-16 14:09:10 -0700 Committed r81277: <http://trac.webkit.org/changeset/81277> 85948 2011-03-16 11:36:00 -0700 2011-03-16 13:42:19 -0700 cleanup bug-56472-20110316113559.patch text/plain 2405 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDgxMjYxKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTggQEAKKzIwMTEtMDMtMTYgIFJ5b3N1a2Ug Tml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgTm9kZTo6aXNDb250ZW50RWRpdGFibGUgc2hvdWxkIGFsd2F5cyBj YWxsIHBhcmVudE5vZGUoKSBpbnN0ZWFkIG9mIHBhcmVudE9ySG9zdE5vZGUoKQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTY0NzIKKworICAgICAgICBS ZXBsYWNlZCB0aGUgY2FsbCB0byBwYXJlbnRPckhvc3ROb2RlKCkgaW4gTm9kZTo6aXNDb250ZW50 RWRpdGFibGUgYnkgYSBjYWxsIHRvIHBhcmVudE5vZGUoKS4KKyAgICAgICAgTm9kZTo6aXNDb250 ZW50RWRpdGFibGUgbm93IGNhbGxzIHBhcmVudE5vZGUoKSBvbiBhbGwgbm9kZXMuCisKKyAgICAg ICAgTm8gdGVzdHMgYXJlIGFkZGVkIHNpbmNlIHRoaXMgYmVoYXZpb3IgY2hhbmdlIGlzIG5vdCB2 aXNpYmxlIHRvIHNjcmlwdHMgYXQgdGhlIG1vbWVudC4KKworICAgICAgICAqIGRvbS9Ob2RlLmNw cDoKKyAgICAgICAgKFdlYkNvcmU6Ok5vZGU6OmlzQ29udGVudEVkaXRhYmxlKToKKwogMjAxMS0w My0xNSAgUGF2ZWwgRmVsZG1hbiAgPHBmZWxkbWFuQGNocm9taXVtLm9yZz4KIAogICAgICAgICBS ZXZpZXdlZCBieSBZdXJ5IFNlbWlraGF0c2t5LgpJbmRleDogU291cmNlL1dlYkNvcmUvZG9tL05v ZGUuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2RvbS9Ob2RlLmNwcAkocmV2aXNp b24gODEyMjApCisrKyBTb3VyY2UvV2ViQ29yZS9kb20vTm9kZS5jcHAJKHdvcmtpbmcgY29weSkK QEAgLTcyMCwxOSArNzIwLDEzIEBAIGJvb2wgTm9kZTo6aXNDb250ZW50RWRpdGFibGUoRWRpdGFi bGVMZXYKICAgICAvLyBDb250YWluZXJOb2RlOjpzZXRGb2N1cygpIGNhbGxzIHNldE5lZWRzU3R5 bGVSZWNhbGMoKSwgc28gdGhlIGFzc2VydGlvbgogICAgIC8vIHdvdWxkIGZpcmUgaW4gdGhlIG1p ZGRsZSBvZiBEb2N1bWVudDo6c2V0Rm9jdXNlZE5vZGUoKS4KIAotICAgIGZvciAoY29uc3QgTm9k ZSogbm9kZSA9IHRoaXM7IG5vZGU7ICkgewotICAgICAgICBpZiAobm9kZS0+aXNIVE1MRWxlbWVu dCgpIHx8IG5vZGUtPmlzRG9jdW1lbnROb2RlKCkpIHsKLSAgICAgICAgICAgIGlmIChub2RlLT5y ZW5kZXJlcigpKSB7Ci0gICAgICAgICAgICAgICAgaWYgKGVkaXRhYmxlTGV2ZWwgPT0gUmljaGx5 RWRpdGFibGUpCi0gICAgICAgICAgICAgICAgICAgIHJldHVybiBub2RlLT5yZW5kZXJlcigpLT5z dHlsZSgpLT51c2VyTW9kaWZ5KCkgPT0gUkVBRF9XUklURTsKKyAgICBmb3IgKGNvbnN0IE5vZGUq IG5vZGUgPSB0aGlzOyBub2RlOyBub2RlID0gbm9kZS0+cGFyZW50Tm9kZSgpKSB7CisgICAgICAg IGlmICgobm9kZS0+aXNIVE1MRWxlbWVudCgpIHx8IG5vZGUtPmlzRG9jdW1lbnROb2RlKCkpICYm IG5vZGUtPnJlbmRlcmVyKCkpIHsKKyAgICAgICAgICAgIGlmIChlZGl0YWJsZUxldmVsID09IFJp Y2hseUVkaXRhYmxlKQorICAgICAgICAgICAgICAgIHJldHVybiBub2RlLT5yZW5kZXJlcigpLT5z dHlsZSgpLT51c2VyTW9kaWZ5KCkgPT0gUkVBRF9XUklURTsKIAotICAgICAgICAgICAgICAgIEVV c2VyTW9kaWZ5IHVzZXJNb2RpZnkgPSBub2RlLT5yZW5kZXJlcigpLT5zdHlsZSgpLT51c2VyTW9k aWZ5KCk7Ci0gICAgICAgICAgICAgICAgcmV0dXJuIHVzZXJNb2RpZnkgPT0gUkVBRF9XUklURSB8 fCB1c2VyTW9kaWZ5ID09IFJFQURfV1JJVEVfUExBSU5URVhUX09OTFk7Ci0gICAgICAgICAgICB9 Ci0gICAgICAgICAgICBub2RlID0gbm9kZS0+cGFyZW50Tm9kZSgpOwotICAgICAgICB9IGVsc2Ug ewotICAgICAgICAgICAgLy8gRklYTUU6IFNob3VsZCB0aGlzIGJlIHBhcmVudE5vZGUoKSBpbnN0 ZWFkPwotICAgICAgICAgICAgbm9kZSA9IG5vZGUtPnBhcmVudE9ySG9zdE5vZGUoKTsKKyAgICAg ICAgICAgIEVVc2VyTW9kaWZ5IHVzZXJNb2RpZnkgPSBub2RlLT5yZW5kZXJlcigpLT5zdHlsZSgp LT51c2VyTW9kaWZ5KCk7CisgICAgICAgICAgICByZXR1cm4gdXNlck1vZGlmeSA9PSBSRUFEX1dS SVRFIHx8IHVzZXJNb2RpZnkgPT0gUkVBRF9XUklURV9QTEFJTlRFWFRfT05MWTsKICAgICAgICAg fQogICAgIH0KIAo= 85965 2011-03-16 13:42:22 -0700 2011-03-16 13:43:48 -0700 use switch statement instead of ifs bug-56472-20110316134221.patch text/plain 2455 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDgxMjY1KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTggQEAKKzIwMTEtMDMtMTYgIFJ5b3N1a2Ug Tml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgTm9kZTo6aXNDb250ZW50RWRpdGFibGUgc2hvdWxkIGFsd2F5cyBj YWxsIHBhcmVudE5vZGUoKSBpbnN0ZWFkIG9mIHBhcmVudE9ySG9zdE5vZGUoKQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTY0NzIKKworICAgICAgICBS ZXBsYWNlZCB0aGUgY2FsbCB0byBwYXJlbnRPckhvc3ROb2RlKCkgaW4gTm9kZTo6aXNDb250ZW50 RWRpdGFibGUgYnkgYSBjYWxsIHRvIHBhcmVudE5vZGUoKS4KKyAgICAgICAgTm9kZTo6aXNDb250 ZW50RWRpdGFibGUgbm93IGNhbGxzIHBhcmVudE5vZGUoKSBvbiBhbGwgbm9kZXMuCisKKyAgICAg ICAgTm8gdGVzdHMgYXJlIGFkZGVkIHNpbmNlIHRoaXMgYmVoYXZpb3IgY2hhbmdlIGlzIG5vdCB2 aXNpYmxlIHRvIHNjcmlwdHMgYXQgdGhlIG1vbWVudC4KKworICAgICAgICAqIGRvbS9Ob2RlLmNw cDoKKyAgICAgICAgKFdlYkNvcmU6Ok5vZGU6OmlzQ29udGVudEVkaXRhYmxlKToKKwogMjAxMS0w My0xNiAgTWlraGFpbCBOYWdhbm92ICA8bW5hZ2Fub3ZAY2hyb21pdW0ub3JnPgogCiAgICAgICAg IFJldmlld2VkIGJ5IFBhdmVsIEZlbGRtYW4uCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9kb20vTm9k ZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvZG9tL05vZGUuY3BwCShyZXZpc2lv biA4MTI2NSkKKysrIFNvdXJjZS9XZWJDb3JlL2RvbS9Ob2RlLmNwcAkod29ya2luZyBjb3B5KQpA QCAtNzIwLDE5ICs3MjAsMTggQEAgYm9vbCBOb2RlOjppc0NvbnRlbnRFZGl0YWJsZShFZGl0YWJs ZUxldgogICAgIC8vIENvbnRhaW5lck5vZGU6OnNldEZvY3VzKCkgY2FsbHMgc2V0TmVlZHNTdHls ZVJlY2FsYygpLCBzbyB0aGUgYXNzZXJ0aW9uCiAgICAgLy8gd291bGQgZmlyZSBpbiB0aGUgbWlk ZGxlIG9mIERvY3VtZW50OjpzZXRGb2N1c2VkTm9kZSgpLgogCi0gICAgZm9yIChjb25zdCBOb2Rl KiBub2RlID0gdGhpczsgbm9kZTsgKSB7Ci0gICAgICAgIGlmIChub2RlLT5pc0hUTUxFbGVtZW50 KCkgfHwgbm9kZS0+aXNEb2N1bWVudE5vZGUoKSkgewotICAgICAgICAgICAgaWYgKG5vZGUtPnJl bmRlcmVyKCkpIHsKLSAgICAgICAgICAgICAgICBpZiAoZWRpdGFibGVMZXZlbCA9PSBSaWNobHlF ZGl0YWJsZSkKLSAgICAgICAgICAgICAgICAgICAgcmV0dXJuIG5vZGUtPnJlbmRlcmVyKCktPnN0 eWxlKCktPnVzZXJNb2RpZnkoKSA9PSBSRUFEX1dSSVRFOwotCi0gICAgICAgICAgICAgICAgRVVz ZXJNb2RpZnkgdXNlck1vZGlmeSA9IG5vZGUtPnJlbmRlcmVyKCktPnN0eWxlKCktPnVzZXJNb2Rp ZnkoKTsKLSAgICAgICAgICAgICAgICByZXR1cm4gdXNlck1vZGlmeSA9PSBSRUFEX1dSSVRFIHx8 IHVzZXJNb2RpZnkgPT0gUkVBRF9XUklURV9QTEFJTlRFWFRfT05MWTsKKyAgICBmb3IgKGNvbnN0 IE5vZGUqIG5vZGUgPSB0aGlzOyBub2RlOyBub2RlID0gbm9kZS0+cGFyZW50Tm9kZSgpKSB7Cisg ICAgICAgIGlmICgobm9kZS0+aXNIVE1MRWxlbWVudCgpIHx8IG5vZGUtPmlzRG9jdW1lbnROb2Rl KCkpICYmIG5vZGUtPnJlbmRlcmVyKCkpIHsKKyAgICAgICAgICAgIHN3aXRjaCAobm9kZS0+cmVu ZGVyZXIoKS0+c3R5bGUoKS0+dXNlck1vZGlmeSgpKSB7CisgICAgICAgICAgICBjYXNlIFJFQURf T05MWToKKyAgICAgICAgICAgICAgICByZXR1cm4gZmFsc2U7CisgICAgICAgICAgICBjYXNlIFJF QURfV1JJVEU6CisgICAgICAgICAgICAgICAgcmV0dXJuIHRydWU7CisgICAgICAgICAgICBjYXNl IFJFQURfV1JJVEVfUExBSU5URVhUX09OTFk6CisgICAgICAgICAgICAgICAgcmV0dXJuIGVkaXRh YmxlTGV2ZWwgIT0gUmljaGx5RWRpdGFibGU7CiAgICAgICAgICAgICB9Ci0gICAgICAgICAgICBu b2RlID0gbm9kZS0+cGFyZW50Tm9kZSgpOwotICAgICAgICB9IGVsc2UgewotICAgICAgICAgICAg Ly8gRklYTUU6IFNob3VsZCB0aGlzIGJlIHBhcmVudE5vZGUoKSBpbnN0ZWFkPwotICAgICAgICAg ICAgbm9kZSA9IG5vZGUtPnBhcmVudE9ySG9zdE5vZGUoKTsKKyAgICAgICAgICAgIEFTU0VSVF9O T1RfUkVBQ0hFRCgpOworICAgICAgICAgICAgcmV0dXJuIGZhbHNlOwogICAgICAgICB9CiAgICAg fQogCg==