Closed Bug 500243 Opened 11 years ago Closed 11 years ago

range.surroundContents() adds an extraneous empty text node


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

Windows XP
Not set





(Reporter: marcosalmeida, Assigned: smaug)




(Keywords: regression, testcase)


(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/530.5 (KHTML, like Gecko) Chrome/ Safari/530.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5

When a range surrounds a whole text node that is the only child of an element, and then you call surroundContents on it, the text node is placed inside the new element, and empty text nodes are added on either side of the new node. I've seen this in FF 3.0 and WebKit, so it seems like an acceptable thing. However, with FF 3.5 I see two empty text nodes coming after the new element. I also tried having the range surrounding only a part of the text inside the original text node, and the resulting dom has the extraneous empty text node immediately following the new element, followed by a text node with the remaining text that didn't get surrounded.

Reproducible: Always

Steps to Reproduce:
1. go to{%0A%20%20result%20%2B%3D%20innerDiv.childNodes[i]%0A%20%20%20%20%20%20%20%20%20%20%20%20%2B%20%27%28%27%20%2B%20innerDiv.childNodes[i].textContent%20%2B%20%27%29%27%3B%0A}%0Aresult%3B
2. click on eval once
3. see result displayed
4. change range ofssets from 0,5 to 1,4
5. click on eval once
6. see the result displayed
Actual Results:  
In both cases there are two text nodes following the code element. The first of those is always empty

Expected Results:  
There should only be one text node following the code element. In the first case it should be empty, in the second case it should contain "o".

This affects Google properties.
Regression range appears to be:
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
I'll make it blocking 332148 for now, guessing that this is the cause.
Blocks: 332148
Component: General → DOM
Ever confirmed: true
Keywords: regression, testcase
QA Contact: general → general
It seems to me that all the empty textnodes created here are a bad idea. Even if webkit does the same.
Flags: wanted1.9.2?
Assignee: nobody → Olli.Pettay
I want to take a look at the test in DOM Inspector, both before and after the surroundContents call.  I'm thinking about what nodes are there before, and what nodes are there after.
Attached patch patch (obsolete) — Splinter Review
FYI, cloning a text node should be fast, even if it contains lots of data.
Attachment #385252 - Flags: review?(ajvincent)
Hmm, the patch breaks some unit tests
Attachment #385252 - Flags: review?(ajvincent)
Comment on attachment 385252 [details] [diff] [review]

I need to figure out what is causing the unit test failure.
I can't do a formal review on this until Saturday evening, but one thing I can point out now while you're fixing tests is this:

-            rv = SplitDataNode(charData, startOffset, endOffset,
-                               getter_AddRefs(cutNode),
-                               getter_AddRefs(endNode));

IIRC, that was the only caller for which we required the fifth argument in SplitDataNode.  The other callers passed in null.  I'd suggest removing the argument, and possibly endOffset if it's not relevant either.
Attached patch v2 (obsolete) — Splinter Review
Fixes unit tests.
It is not very clear when to merge text nodes. There is just this
"Note that if deletion of a Range leaves adjacent Text nodes, they are not automatically merged, and empty Text nodes are not automatically removed. Two Text nodes should be joined only if each is the container of one of the boundary-points of a Range whose contents are deleted. To merge adjacent Text nodes, or remove empty text nodes, the normalize() method on the Node interface should be used."
So the patch makes us to merge text nodes, if they were originally
the same text node which contained both boundary points and after deletion
each contain one boundary point. (In practice the patch skips the whole split/merge).

I wish there were some cross browser tests.
(our unit tests should be converted to something which could be run in any browser - to mochitest)

I'll write some more tests later.
Attachment #385252 - Attachment is obsolete: true
Attachment #385280 - Flags: review?(ajvincent)
Attachment #385280 - Attachment is obsolete: true
Attachment #385280 - Flags: review?(ajvincent)
Attached patch v3Splinter Review
Attachment #385283 - Flags: superreview?(jonas)
Attachment #385283 - Flags: review?(ajvincent)
As far as I see, our current behavior is also ok per the spec.
As it is said, normalize() could be called to merge adjacent text nodes.

But anyway, the patch should bring back our old behavior.
Attachment #385283 - Flags: superreview?(jonas) → superreview+
Flags: wanted1.9.1.x?
Attachment #385283 - Flags: review?(ajvincent) → review+
Comment on attachment 385283 [details] [diff] [review]

>+              clone->SetNodeValue(cutValue);

I feel uncomfortable about not checking for an error code returned here.  Right now it looks like it can't fail (per nsGenericDOMDataNode::SetNodeValue and nsGenericDOMDataNode::SetTextInternal) as the paths to errors shouldn't be possible here.  Future changes to any of this code, though, could make that no longer the case.  I recommend putting in a NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv)) for debug builds, at least, preferably a NS_ENSURE_SUCCESS(rv, rv), even though it may not be necessary now.

r+ with or without that change.
Closed: 11 years ago
Resolution: --- → FIXED
Flags: wanted1.9.1.x?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.