673108 - Modifying a `range` in the selection, having multiple selection in the same node, ends up to corrupting selection

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: 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.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: example of multiple selection workaround

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

(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.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

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!

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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[1] which seems to change the behavior of
deleteContents, but I'm not sure that the draft is correct yet, or compatible with
implementations.

[1] http://html5.org/specs/dom-range.html#dom-range-deletecontents

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Hmm, looking at the code, looks like our implementation should do what the
new draft says... investigating.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test

This seems to work as expected.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Still more...
The new draft is incompatible with the old spec when handling insertNode
with text nodes.
Investigating some more.

Comment 10

[Mats Palmgren (inactive)]

(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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Mats Palmgren (inactive)]

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.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Mats Palmgren (inactive)]

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.

Comment 15

[Mats Palmgren (inactive)]

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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Mats Palmgren (inactive)]

The patches in bug 191864 fixes this bug.

Comment 19

[Matteo Ferretti [:zer0] [:matteo]]

Mats, the patches in that bug fixes also the `selectNode` behavior said in the description?

Comment 20

[Mats Palmgren (inactive)]

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.

Comment 21

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: example of multiple selection with fragment

Comment 22

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 23

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 24

[Mats Palmgren (inactive)]

All the testcases seems to work for me in:
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111025 Firefox/10.0a1

Comment 25

[Mats Palmgren (inactive)]

Bug 619273 will fix the remaining repaint problem.

Comment 26

[Mats Palmgren (inactive)]

(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

Comment 27

[Mats Palmgren (inactive)]

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!