Bug 25822 – DOM normalize does not remove empty text node between element nodes

• Component: DOM
• Status: RESOLVED FIXED

[Kai Brüning]

The DOM node normalize() function does not remove empty text nodes which are not adjacent to other text nodes nor the only child node.
That is, empty text nodes between element nodes or at the start or end of the child list are not removed.
This is clearly in contradiction to the DOM level 2 core specification.
Note also that it works in FireFox (tested with 3.0.10).
A test case is attached, a patch forthcoming.

[Kai Brüning]

Created attachment 30384 [details]
Test case for this bug

[Kai Brüning]

Created attachment 30390 [details]
Patch to fix this bug

I do not have a Tiger machine to run layout tests, but I made sure that the set of failed layout tests under Leopard did not change by the patch.

[Darin Adler]

Comment on attachment 30390 [details]
Patch to fix this bug

> -    for (; node; node = node->traverseNextNodePostOrder()) {
> +    for (; node; ) {

Should be changed a while, then.

> +        } else {
> +            node = node->traverseNextNodePostOrder();

WebKit style doesn't use braces around single lines. Typically we'd use "early continue" in a case like this:

    if (type != TEXT_NODE) {
        node = node->traverseNextNodePostOrder();
        continue;
    }

I'd really like to see a more thorough set of test cases. Adding a test for only the single case that shows the current bug seems too minimal.

r=me as is -- none of my requested changes are mandatory

[Kai Brüning]

Created attachment 30400 [details]
Path with suggested code changes

I integrated the code-level suggestions of Darin.
Adding more test will take a little longer; guess I expected tests for the other cases to already exist.
Should this be one file per tested case or is it ok to combine several cases into one test file?

[Kai Brüning]

One additional remark: I am a little puzzled by the fact that the original code carefully and specifically handled the "single empty text node child" case, but ignored the mixed empty text node/elements case. I just would feel better knowing the rationale behind that code.

[Darin Adler]

(In reply to comment #4)
> Should this be one file per tested case or is it ok to combine several cases
> into one test file?

A single test file is better. The key to making a good test is making sure you can understand what its testing and see what's failing and what's working.

[Darin Adler]

(In reply to comment #5)
> One additional remark: I am a little puzzled by the fact that the original code
> carefully and specifically handled the "single empty text node child" case, but
> ignored the mixed empty text node/elements case. I just would feel better
> knowing the rationale behind that code.

You can look at the ChangeLog to see if there are any comments that make it clear. I doubt there was any rationale -- it was probably a simple mistake. I trust test cases more than I trust code.

[Kai Brüning]

Created attachment 30421 [details]
Patch with more thorough set of test cases

I extended the test case to (hopefully) test the full range of the normalize() functionality. To express this, I renamed it to "normalize.html". The test lives inside "dom", therefore I consider the term "normalize" well defined. Please advice if a longer name is necessary.

Having these tests and after learning that document->textNodesMerged() does adjust DOMRanges to the changed text nodes and nothing more, I changed the code of Node::normalize() a little more to make it more concise and less historic.

Concerning rationale of the old code, I checked the ChangeLogs and found the entry about the addition of the "single empty text node child" case. It is indeed rather inexpressively, just stating the addition of this case.

[Darin Adler]

Comment on attachment 30421 [details]
Patch with more thorough set of test cases

> +        // Merge non-empty text nodes.
> +        while (1) {
> +            Node* nextSibling = node->nextSibling();
> +            if (!nextSibling || !nextSibling->isTextNode()) {
> +                node = node->traverseNextNodePostOrder();
> +                break;
> +            }

I think it would be clearer to have the node = node->traverseNextNodePostOrder() line outside the if statement. I also would prefer to have a while condition instead of just while (1).

Here's how I'd write it:

    while (Node* nextSibling = node->nextSibling()) {
        if (!nextSiblng->isTextNode())
            break;
        ...
    }

    node = node->traverseNextNodePostOrder();

Patch looks good, r=me even without that change.

[Darin Adler]

I'll take care of landing this. I think I'll tweak the test a bit.

[Kai Brüning]

> I'll take care of landing this. I think I'll tweak the test a bit.

Great. Please tweak the code, too, as you suggested. I agree that both changes make it clearer.

[Darin Adler]

http://trac.webkit.org/changeset/43809