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
Created attachment 85948 [details] cleanup
Comment on attachment 85948 [details] cleanup ok.
Comment on attachment 85948 [details] 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.
Comment on attachment 85948 [details] 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.
Your call.
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;
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; } }
But put the return statements on separate lines. There’s no need to do it all in one line.
Created attachment 85965 [details] use switch statement instead of ifs
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.
(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.
(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!
(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().
(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!
Committed r81277: <http://trac.webkit.org/changeset/81277>