345666 - Node.normalize() doesn't remove empty Text nodes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jeff Walden [:Waldo]]

Normalization of an element containing only an empty text node doesn't remove that text node.  According to the DOM Level 3 specification, "there are neither adjacent Text nodes nor empty Text nodes" in a node which has been normalized.

http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-normalize

The DOM Level 1 spec does not include empty Text nodes in its description of Node.normalize(), and that addition was made for DOM Level 2.  This wouldn't strictly be a backwards-compatible change, but I suspect no one relies on that behavior, and implementations such as Sun's Java 1.5 obey the newer spec.

Comment 1

[Jeff Walden [:Waldo]]

Taking, because this looks fairly easy to fix going by a cursory glance at the code...

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch

Also, for what it's worth, Safari and Opera both remove empty Text nodes during normalization.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 230678 [details] [diff] [review]
Patch

         case nsIDOMNode::TEXT_NODE:
 
+          // ensure that if the text node is empty, it is removed
+          {
+            nsAutoString text;
+            result = node->GetNodeValue(text);
+            if (NS_FAILED(result)) {
+              return result;
+            }
+
+            if (text.IsEmpty()) {

This should just QI to nsIContent and check if TextLength() returns null or not, that'd be *much* cheaper than copying the string data out of every text node while walking the tree.

r/sr- based on that, but other than that this looks good.

Comment 4

[Johnny Stenback (:jst)]

s/null/0/

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Patch, v2

(In reply to comment #3)
> that'd be *much* cheaper than copying the string data out of every text
> node while walking the tree.

Mm, internal APIs...thanks for pointing out the ridiculously more efficient method that uses them.  :-)

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 230690 [details] [diff] [review]
Patch, v2

r+sr=jst

Comment 7

[Jeff Walden [:Waldo]]

Patch checked in, marking FIXED.