DeleteSelectionCommand::removeNode tries to insert block placeholder in non-editable table cell positions
Created attachment 86881 [details] Patch
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)?
Created attachment 86884 [details] Patch
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.
Created attachment 86891 [details] Patch
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.
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?
Attachment 86891 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8266503
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.
Created attachment 87104 [details] Patch
Attachment 87104 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8266953
Attachment 87104 [details] did not build on qt: Build output: http://queues.webkit.org/results/8270710
Attachment 87104 [details] did not build on win: Build output: http://queues.webkit.org/results/8267864
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.
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.
Ah, I didn't realise that isContentEditable had been renamed recently to renderIsEditable.
Created attachment 87257 [details] Patch
Comment on attachment 87257 [details] Patch Clearing flags on attachment: 87257 Committed r82550: <http://trac.webkit.org/changeset/82550>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/82550 might have broken GTK Linux 32-bit Debug