Bug 57079 – DeleteSelectionCommand::removeNode tries to insert block placeholder in non-editable table cell positions

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin (Ben) Kalman]

DeleteSelectionCommand::removeNode tries to insert block placeholder in non-editable table cell positions

[Benjamin (Ben) Kalman]

Created attachment 86881 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 86881 [details]
Patch

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

> Source/WebCore/editing/DeleteSelectionCommand.cpp:346
> +        Position firstEditablePositionInChild = firstEditablePositionInNode(child);
> +        if (firstEditablePositionInChild.isNotNull())
> +            return firstEditablePositionInChild;
> +        child = child->nextSibling();

I don't think this function needs be recursive.  Can't you just traverseNextNode(node)?

[Benjamin (Ben) Kalman]

Created attachment 86884 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 86884 [details]
Patch

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

> LayoutTests/editing/execCommand/delete-table-with-empty-contents-expected.txt:1
> +PASS

It'll be nice if you explain what this test is testing.

It'll be even nicer if you added some label next to each PASS/FAIL so that when a test fail, we can easily tell which one failed without having to count.
You can probably add a title attribute to each div and extract & print that in runTestForContainer.

> LayoutTests/editing/execCommand/delete-table-with-empty-contents.html:61
> +  var console = document.getElementsByTagName('pre')[0];

No need to indent these.

> LayoutTests/editing/execCommand/delete-table-with-empty-contents.html:75
> +  Array.prototype.slice.call(document.getElementsByTagName('div')).forEach(runTestForContainer);

Ah, this is really neat.

[Benjamin (Ben) Kalman]

Created attachment 86891 [details]
Patch

[Benjamin (Ben) Kalman]

Comment I was writing initially, but collided with your review :-)

---

As I've commented in the changelog, this bug is only observable in debug builds since it causes an ASSERT fail.

In any case, it's caused by the deletion of a non-editable table, when a cell's contents has no height.  This includes
 - no content
 - children amounting to no content (e.g. empty span(s))
 - editable children such that their contents is deleted, but due to one of the nodes being an editing root, not being deleted

An example of the latter case is:

<div contenteditable>
  <table contenteditable=false><tr><td>
    <span contenteditable=true>Deleting the whole table will crash a debug build</span>
  </td></tr></table>
</div>

Currently, removeNode inserts a placeholder when a table cell has no height after deletion.  However, it should only try to insert a placeholder if there actually is somewhere to insert one, and in which case, actually in that editable node rather than on the table cell itself.

[Ryosuke Niwa]

Comment on attachment 86891 [details]
Patch

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

> LayoutTests/editing/execCommand/delete-table-with-empty-contents-expected.txt:1
> +Tests that tables with inevitably empty content are deleted correctly.

I'm not sure if "inevitably empty content" is self-explanatory.  I wouldn't mind it be a little more verbose.

> LayoutTests/editing/execCommand/delete-table-with-empty-contents.html:2
> +<div contenteditable tests='empty'>

I'm not sure if it's really desirable to have custom "tests" attribute.  Is it allowed in HTML5 spec?  I mean what if in the future we decided to add tests attribute for whatever purpose in HTMLx.  Wouldn't this collide with that?  Ideally, we wouldn't need to worry about this test when that happens.

> LayoutTests/editing/execCommand/delete-table-with-empty-contents.html:73
> +      'FAIL (' + testDescription + '), should be empty but was ' + container.innerText;

Not necessarily "empty", right?  How about 'should contain exactly one LF but was' or 'should be "\n" but was'

> Source/WebCore/editing/DeleteSelectionCommand.cpp:344
> +    Node* next = node;
> +    while (next) {
> +        if (next->isContentEditable())
> +            return firstPositionInNode(next);
> +        next = next->traverseNextNode(node);
> +    }

First off, it's wrong to always return firstPositionInNode because next could be img element with width & height 0px. r- because of this.

I would make it a for loop as in:
for (Node* next = node; next && !next->isContentEditable(); next = next->traverseNextNode(node)) {}
return firstPositionInOrBeforeNode(next);

> Source/WebCore/editing/DeleteSelectionCommand.cpp:389
> +            Position firstEditablePosition = firstEditablePositionInNode(node.get());

Presumably firstEditablePosition is visually coinciding with firstPositionInOrBeforeNode(node).  Can we assert that by e.g. comparing that the canonicalized visible positions created by those two positions are identical?

[WebKit Review Bot]

Attachment 86891 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8266503

[Benjamin (Ben) Kalman]

Comment on attachment 86891 [details]
Patch

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

>> LayoutTests/editing/execCommand/delete-table-with-empty-contents.html:2
>> +<div contenteditable tests='empty'>
> 
> I'm not sure if it's really desirable to have custom "tests" attribute.  Is it allowed in HTML5 spec?  I mean what if in the future we decided to add tests attribute for whatever purpose in HTMLx.  Wouldn't this collide with that?  Ideally, we wouldn't need to worry about this test when that happens.

I used "tests" to match junit terminology (for lack of any other), though it should actually be "test".  As in "testWithEmptySpan", "testWithEditableNonEmptySpan", etc.  It really makes no difference to me either way what is used, though I highly doubt that HTML5 will add a test or tests attribute.  And that if it did, it would collide.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:344
>> +    }
> 
> First off, it's wrong to always return firstPositionInNode because next could be img element with width & height 0px. r- because of this.
> 
> I would make it a for loop as in:
> for (Node* next = node; next && !next->isContentEditable(); next = next->traverseNextNode(node)) {}
> return firstPositionInOrBeforeNode(next);

Images can be contenteditable?  Really?  Wow.

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:389
>> +            Position firstEditablePosition = firstEditablePositionInNode(node.get());
> 
> Presumably firstEditablePosition is visually coinciding with firstPositionInOrBeforeNode(node).  Can we assert that by e.g. comparing that the canonicalized visible positions created by those two positions are identical?

Yes and no.  Yes, they should be visually coinciding.  No, it can't be asserted that they're canonicalised the same, since editability affects canonicalisation.  The following positions
<td>|<span contenteditable></span></td>
and
<td><span contenteditable>|</span></td>
are... kind of visually coinciding, but certainly not canonicalised identically.  And this is one of the cases that has a bug, fixed by this patch.

[Benjamin (Ben) Kalman]

Created attachment 87104 [details]
Patch

[WebKit Review Bot]

Attachment 87104 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8266953

[Early Warning System Bot]

Attachment 87104 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8270710

[Build Bot]

Attachment 87104 [details] did not build on win:
Build output: http://queues.webkit.org/results/8267864

[Benjamin (Ben) Kalman]

Erm, am I missing something?  Why are all these bots saying

DeleteSelectionCommand.cpp:342: error: ‘class WebCore::Node’ has no member named ‘isContentEditable’

Since (a) Node definitely does have isContentEditable, and (b) that line doesn't appear on line 342 of the patch, and (c) it compiles locally.

[Adam Barth]

http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.h#L322

#if PLATFORM(MAC)
    // Objective-C extensions
    bool isContentEditable() const { return rendererIsEditable(Editable); }
#endif

Those bots do not build with PLATFORM(MAC) because they do not run Mac OS.

[Benjamin (Ben) Kalman]

Ah, I didn't realise that isContentEditable had been renamed recently to renderIsEditable.

[Benjamin (Ben) Kalman]

Created attachment 87257 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 87257 [details]
Patch

Clearing flags on attachment: 87257

Committed r82550: <http://trac.webkit.org/changeset/82550>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82550 might have broken GTK Linux 32-bit Debug