Bug 56472 – Node::isContentEditable should always call parentNode() instead of parentOrHostNode()

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 85948 [details]
cleanup

[Dimitri Glazkov (Google)]

Comment on attachment 85948 [details]
cleanup

ok.

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Darin Adler]

Your call.

[Ryosuke Niwa]

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;

[Ryosuke Niwa]

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;
    }
}

[Darin Adler]

But put the return statements on separate lines. There’s no need to do it all in one line.

[Ryosuke Niwa]

Created attachment 85965 [details]
use switch statement instead of ifs

[Darin Adler]

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.

[Ryosuke Niwa]

(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.

[Darin Adler]

(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!

[Ryosuke Niwa]

(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().

[Darin Adler]

(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!

[Ryosuke Niwa]

Committed r81277: <http://trac.webkit.org/changeset/81277>