editing commands shouldn't run when there's no body

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

WebKit currently allows editing commands to run without a body element when the design mode is turned on, and this behavior led to crashes such as that of the bug 56642.  We should avoid executing (most of) editing commands when there is no body element because appending a node to a body-less document isn't possible from scripts and allowing such an operation via editing commands creates an unnecessary loop-hole to break assumptions in WebCore.

As far as I checked, IE doesn't support execCommand under designMode and FF ignores execCommands when there are no body element.

[Eric Seidel (no email)]

This sounds like a good (and easily testable) idea, assuming we confirm that the test passes in other browsers.

[Ryosuke Niwa]

Created attachment 86393 [details]
fixes the bug

[Eric Seidel (no email)]

Comment on attachment 86393 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=86393&action=review

Have you validated that FF and IE pass these tests the same way we do?

> Source/WebCore/editing/Editor.cpp:213
> -    return m_frame->selection()->isContentEditable();
> +    return m_frame->selection()->rootEditableElement();

Do we need to check both?  It's not clear to me how this is a superset of the previous check.

[Ryosuke Niwa]

Comment on attachment 86393 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=86393&action=review

>> Source/WebCore/editing/Editor.cpp:213
>> +    return m_frame->selection()->rootEditableElement();
> 
> Do we need to check both?  It's not clear to me how this is a superset of the previous check.

If selection start has a root editable element, then it must also be editable.  To verify this, rootEditableElement calls editableRootForPosition in htmlediting.cpp, which calls Node::rootEditableElement() but rootEditableElement() only traverses through nodes that are content editable.

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 86393 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86393&action=review
> 
> Have you validated that FF and IE pass these tests the same way we do?

In FF, yes. In IE, we can't since IE doesn't support design mode properly. For StyleWithCSS, we deviate from FF because FF ignores StyleWithCSS when there's no body (as far as I checked).  But this is odd as ap pointed on IRC because StyleWithCSS is a document properly.

[Eric Seidel (no email)]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 86393 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=86393&action=review
> > 
> > Have you validated that FF and IE pass these tests the same way we do?
> 
> In FF, yes. In IE, we can't since IE doesn't support design mode properly. For StyleWithCSS, we deviate from FF because FF ignores StyleWithCSS when there's no body (as far as I checked).  But this is odd as ap pointed on IRC because StyleWithCSS is a document properly.

The FF difference sounds like something we should test.  We could also file a but against FF with bugs.mozilla.org.

But overall sounds great.  Thanks.  You might add the comment about having tested in FF and IE to your ChangeLog.

[Eric Seidel (no email)]

Comment on attachment 86393 [details]
fixes the bug

LGTM.  Please consider adding the testing information to the ChangeLog and considering testing our behavior re: StyleWithCSS and how it differs from FF.  That can of course be a separate bug.

[Ryosuke Niwa]

Here's new WebCore change log entry.  Does it look good to you, Eric?

The bug was caused by WebKit's not checking the existence of root editable element
in enabled* functions. Although isContentEditable returns true whenever we're in design mode,
we should not run editing commands in a document without a body element editable because
doing so results in appending a non-body element to the document node.

Fixed the bug by modifying various enabled* functions to ensure we have a root editable element.
New behavior tries to match that of Firefox except StyleWithCSS, which Firefox seems to ignore
when there are no body element. Since StyleWithCSS is a document's state or property, we allow
execCommand('StyleWithCSS') even in a document without a body element.

WebKit's and Firefox's behaviors also deviate in insert-image-with-selecting-document.html.
Whereas WebKit respects selection set by script and ignores execCommand, Firefox modifies
the selection when document.write("x") is ran and successfully inserts image.

Thus, empty-document-delete.html and empty-document-justify-right.html both pass on Firefox
while empty-document-stylewithcss.html and insert-image-with-selecting-document.html both fail.

Since Internet Explorer does not allow execCommand to run under design mode properly, we could
not test its behavior.

[Ryosuke Niwa]

Committed r82791: <http://trac.webkit.org/changeset/82791>