editing/deleting/5156801.html doesn't test what it claims to test

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ryosuke Niwa]

The explanation and code in
LayoutTests\editing\deleting\5156801.html
does not match.

In fact, the test seems to be missing some code.

The following is the original code:
<p>This tests for a bug where delete would crash if a node just after the selection to delete is removed in a DOMNodeRemoved event handler for the last node in the selection to delete.</p> 
<div contenteditable="true">foo<span id="foo">x</span><span id="removeme">y</span></div> 
 
<script> 
function removeHandler() {
    var removeme = document.getElementById("removeme");
    removeme.parentNode.removeChild(removeme);
}
 
document.getElementById("foo").addEventListener("DOMNodeRemoved", removeHandler);
</script>

[Ryosuke Niwa]

My guess is that the test should look something like this:

<p>This tests for a bug where delete would crash if a node just after the selection to delete is removed in a DOMNodeRemoved event handler for the last node in the selection to delete.</p>
<div id="div" contenteditable="true">foo<span id="foo">x</span><span id="removeme">y</span></div>
<p id="console"></p>

<script>

if (window.layoutTestController)
    window.layoutTestController.dumpAsText();

function log(message) {
    var console = document.getElementById("console");
    var text = document.createTextNode(message);
    console.appendChild(text);
}

function removeHandler() {
    var removeme = document.getElementById("removeme");
    removeme.parentNode.removeChild(removeme);
}

document.getElementById("foo").addEventListener("DOMNodeRemoved", removeHandler);
var sel = window.getSelection();
sel.setBaseAndExtent( document.getElementById('foo'), 0, document.getElementById('foo'), 1 );
document.execCommand("Delete");

log(document.getElementById('div');

</script>

[Justin Garcia]

> My guess is that the test should look something like this:

Looks right.

[Ryosuke Niwa]

Thanks, Justin.  I'll make a patch for it since I've been already rewriting a lot of test cases in editing\deleting.

[Ryosuke Niwa]

Created attachment 31428 [details]
This patch fixes the bug, and converts the text from a pixel test to a dumpAsText test.

[Ryosuke Niwa]

Created attachment 31442 [details]
fixes the bug.  The previous patch had wrong relative path with "WebKit5/" at the beginning

Sorry Justin, I didn't know that I had to svn-create-patch on src/.  I had "WebKit5/" all over the patch.

[Maciej Stachowiak]

Comment on attachment 31442 [details]
fixes the bug.  The previous patch had wrong relative path with "WebKit5/" at the beginning

r=me

This might be a good candidate for a dumpAsMarkup() text if we add that mode.

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 31442 [details] [review])
> r=me
> 
> This might be a good candidate for a dumpAsMarkup() text if we add that mode.
> 

I agree.

[Brent Fulgham]

Assign for landing.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/44882.