Created attachment 547390 [details] example of multiple selection bug In case of multiple selection on the same node, calling `range.insertNode(node)` after a `range.deleteContents()`, will collapse all the ranges below the one modified, to the start offset of the modified range. In addition, calling `range.selectNode(node)`, will corrupting the selection in the page. Reproducible: Always Steps to Reproduce: 1. Select text in the same node to have multiple selection, let's say four. 2. Get the range for the 2nd one 3. Call `range.deleteContents()` to delete the current contents, and call `range.insertNode(node)` to add a new node (where `node` is a DOM node) 4. Call `range.selectNode(node)` Actual Result: The 2nd range will shows the new content, however the 2nd, 3rd and 4th selection are disappeared: the 3rd and the 4th are collapsed to the start offset of the 2nd selection. In addition, the selection is now corrupted, showing everything selected from the start of the container node to the start of the 2nd selection offset. Expected Result: The text in the all four selection should be still selected, and the `range` of 2nd, 3rd and 4th selections should be updated with new start / end offset values, in accordance with the new content of 2nd range. The 1st range should be remain the same. For more details, see the "example of multiple selection bug" attachment for the actual result; and the "example of multiple selection workaround" for the expected result.
(In reply to comment #0) > Actual Result: > The 2nd range will shows the new content, however the 2nd, 3rd and 4th > selection are disappeared: the 3rd and the 4th are collapsed to the start > offset of the 2nd selection. This is what DOM 2 Range spec specifies. > In addition, the selection is now corrupted, showing everything selected > from the start of the container node to the start of the 2nd selection > offset. This looks indeed like a bug.
(In reply to comment #2) > > Actual Result: > > The 2nd range will shows the new content, however the 2nd, 3rd and 4th > > selection are disappeared: the 3rd and the 4th are collapsed to the start > > offset of the 2nd selection. > This is what DOM 2 Range spec specifies. Could you please give to me a link about that? I was looking to http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html but I wasn't able to find the description of this behavior. Maybe I missed it.
I asked because I'm working on Addon SDK selection module, and we're thinking to have a different behavior in this scenario. Any reference about this specific behavior in DOM 2 Range specs could be useful. Thanks!
2.12 says that changes to Text nodes can be thought as splitText() and normalize(). So in this case the text node is split and a new one is created which contains the data after the 2nd range. The other ranges don't automatically move to that new text node. Note, there is a new draft for range which seems to change the behavior of deleteContents, but I'm not sure that the draft is correct yet, or compatible with implementations.  http://html5.org/specs/dom-range.html#dom-range-deletecontents
Hmm, looking at the code, looks like our implementation should do what the new draft says... investigating.
Oh, yes, the behavior is correct even based on the new draft. You call insertNode which ends up splitting the text node, and the ranges after the split point will have their container pointing to the original text node.
Still more... The new draft is incompatible with the old spec when handling insertNode with text nodes. Investigating some more.
(In reply to comment #5) > The other ranges don't automatically move to that new text node. Hmm, why not exactly? 2.12 defines two general principles, one is "all Ranges will select the same portion of the document after any mutation operation", our current behavior seems to violate this principle.
Well, the question is what is "same portion". Adding a new text node isn't the same portion as before. DOM 2 Range doesn't specify this case too well. The new draft does, but I'm not sure it is correct.
It's not the identical nodes, no, but it's still the "same portion" as I see it. I think the intent of the second principle is clear and that is that we should update ranges that refers to text nodes that are removed as a result of the normalize() step so that they cover the same portion of the text afterwards.
When the spec describes how to handle mutations to Document, it is somewhat clear that only changes to offsets are expected, not changes to start/endContainer.
I've read 2.12 again, carefully, and I'm even more convinced this is a bug. The operations in the attached testcase is actually covered by the examples in the spec. Let's look at each part of replaceRange(): 1. range.deleteContents() If I comment out the range.insertNode/selectNode parts, the end result is that the other ranges select the same text they did before (ie. their start/end offsets were updated) -- I think we can agree that this is correct per 2.12.2 2. the range is now collapsed at its former start index -- correct per 2.12.2 3. range.insertNode() we're inserting into a text node at a position that isn't part of any other range. A range after the insertion position is covered by example 1 in 2.11.1 -- the example range ("Y blah i") has been adjusted so that it covers the same text as before the insertion. I think this makes it clear that this is what should happen for "genera" and "tall" in our testcase. Bug 191864 seems to be about the same thing.
Hacking nsGenericDOMDataNode::SplitData to propagate the new text node in the CharacterDataChangeInfo struct, and adjusting the start/end node in nsRange::CharacterDataChanged fixes this bug. So fixing the splitText() case looks relatively easy, at first glance.
Yeah, we could do that (although I do hate special casing range handling). Seems like Webkit and IE9 do update start/end node, Opera works like Gecko.
I don't understand (In reply to comment #14) > 3. range.insertNode() > we're inserting into a text node at a position that isn't part of > any other range. A range after the insertion position is covered by > example 1 in 2.11.1 -- the example range ("Y blah i") has been adjusted > so that it covers the same text as before the insertion. > I think this makes it clear that this is what should happen for > "genera" and "tall" in our testcase. I don't read that example the same way. If the text is inserted to existing Text node using insertData or similar, then the range after it get updated, per 2.12.1. But if you split the node and add something between the two nodes, then you have created two new nodes in the document tree.
Mats, the patches in that bug fixes also the `selectNode` behavior said in the description?
Yes. However, part 2 is unlikely to land so there may still be some painting errors in your testcase without that. Part 1 maintains the correct ranges though so it should be easy to do some workaround for the repaint problem. Please try a Nightly build once it lands.
Good! Thanks Mats, I will try with a Nightly once it lands. We discussed about text node insertion, I just wonder in case of adding a fragment what is the expected behavior. I added a test case for it. Probably in the Addon SDK will not follow the specs for the selection module, in this specific case, but it's good to know.
Mats, I tested with the Nightly and it works exactly as you said. The ranges are correctly maintained, but they aren't painted except for the first one. Any plans to fix the repaint problem as well? You said that the part 2 is unlikely to land, but I hope will manage this issue soon or later.
All the testcases seems to work for me in: Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111025 Firefox/10.0a1
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #22) > We discussed about text node insertion, I just wonder in case of adding a > fragment what is the expected behavior. I added a test case for it. Your document fragment test works as expected. It has the same behavior as the first test where you inserted a text node - the only reason that the inserted text is selected in the first example is the explicit range.selectNode(node) call, which the fragment test doesn't have. AFAICT, IE9 has compatible behavior (if you workaround the missing Range.createContextualFragment method) http://stackoverflow.com/questions/5375616/extjs4-ie9-object-doesnt-support-property-or-method-createcontextualfragme
All issues should be fixed by bug 619273 / bug 698237 in the latest Nightly builds: http://nightly.mozilla.org/ Please file a new bug and CC me if you still see some error. Thanks for your report!