415860 - Range that selects textContent's last character will select everything when textContent changes

Status: RESOLVED FIXED

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

Description

[Ed Lee :Mardak]

If a range currently selects the last character of a #text node, changing the text content causes everything in the new text to be selected (doesn't matter if the new string is shorter, same length, longer).

<bz_gone> Note that when content is inserted at a boundary-point, it is ambiguous as to where the boundary-point should be repositioned if its relative position is to be maintained. 
<bz_gone> There are two possibilities: at the start or at the end of the newly inserted content. We have chosen that in this case neither the container nor offset of the boundary-point is changed. As a result, the boundary-point will be positioned at the start of the newly inserted content.
<bz_gone> Sez the range spec
<bz_gone> So certainly the range start should be at the beginning of the new text

<Mardak> and the end? 
<bz_gone> That one is tougher
<bz_gone> I think what we're doing is "not quite right" per spec
<bz_gone> It should also be ending up at the beginning of the text...
<bz_gone> That is, as I read the range spec the call:
<bz_gone> foo.textContent = "bar";
<bz_gone> and the calls:
<bz_gone> foo.textContent = ""; foo.textContent = "bar";
<bz_gone> should in fact result in the same output
<bz_gone> we got this right in 1.8
<bz_gone> probably a regression from sicking's updates to the range code...

This seems to be causing problems for bug 407944 (see attachment 301083 [details])

Comment 1

[Ed Lee :Mardak]

Attachment: testcase

Comment 2

[Ed Lee :Mardak]

Testcase passes in firefox 2

Comment 3

[Boris Zbarsky [:bzbarsky]]

The point being that if we treat the replace as a removal followed by an insertion (per the spec), then the right behavior in the case when the range start point is in the removed text and the range endpoint is right after the removed text is to end up with the range collapsed at the beginning of the reinserted text.

Jonas, do you agree?  If so, this is pretty easy to fix; I just want to make sure that the behavior change you made here in bug 358106 was in fact an accident...

Comment 4

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

Btw, the testcase works quite differently in FF/Opera/Safari

FF2: all succeeds
FF3: last one fails
Opera9.25: first one fails
Safari3.0.4: Only first one succeeds.

Comment 5

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

Opera seems to have bug in getSelection(),
for some reason it doesn't take account the first range in the testcase.
In Safari range isn't collapsed when setting .textContent.
FF3 doesn't collapse the text in the last testcase, Opera does.

Comment 6

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

(What Safari does is wrong.)

Should nsRange just update boundary points in CharacterDataWillChange, and then
re-updating them in CharacterDataChanged would have the right result.

Comment 7

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

Attachment: possible patch

Am I missing something here?
Have to write still some tests.

Comment 8

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

The patch seems to bring back the 1.8 behavior, which though isn't
the same as what Opera has for DOM Ranges.
I think Opera just has a bug:
if a range in textnode is set to start at offset 0, then when setting textContent
the whole text gets selected, but if offset is set to any other valid value, 
range gets collapsed.

Comment 9

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

There is still one difference comparing to Opera.
If textnode.replaceData is used and the end offset is within the replaced data, 
Opera moves end offset to be after the inserted data.
FF2 doesn't pass my test. FF3, with or without the patch, moves the offset before inserting new data (which is correct, because of DOMRange remove/insert 
behavior), so end offset is right before inserted data.
Safari doesn't move offset at all. Safari's range implementation seems to miss quite many cases.

Comment 10

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

Attachment: mochitests

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand why this is needed. The spec is clear that the start and end boundary points use the exact same algorithm. Need to look more at the code.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Jonas, the issue is not the algorithm (which is fine in our impl).  The issue is that we're not modeling the textContent set as a removal and an insertion like the spec says to do....

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So I think our impl is fine actually. Setting .textContent is only defined as remove-and-insert-new when it comes to children. Here we are changing the .textContent of a textnode, which doesn't have children.

The spec is actually very vague on what happens for textnodes, but it does indicate that .textContent maps to .nodeValue for textnodes, so I'd read it as setting .textContent is the same as setting .nodeValue.

Trickier though is how setting .nodeValue changes a range. We treat it as calling .replaceText(0, node.length, newValue) which I also think makes a lot of sense.

So then the question is, how should .replaceText behave. Like removing the old contents and then inserting the new, or by treating it as a single operation. I.e. given the following textnode, and range

"hello |little| kitty"

Where '|' denotes the start and end of the range. What should a call to .replaceNode(6, 6, "cute") result in. The resulting text would be "hello cute kitty", but where would the start and end of the range be.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 301684 [details] [diff] [review]
possible patch

In any case, this patch is wrong. The algorithm should definitely be the same for both endpoints. If we do believe that the above example should collapse the range then we should use <= for both endpoints.

But I honestly don't know what the correct behavior is.

Comment 15

[Boris Zbarsky [:bzbarsky]]

So I can think of two ways to treat textnodes:

1)  Treat a replaceText as a remove and then insert (of text).
2)  Treat it as an atomic operation and try to figure out what the "keep selecting
    the same content" thing would mean in that case, within the constraint that
    both endpoints get adjusted in the same way.

#1 gives us more predictable behavior, I think, and is what Gecko 1.8 did...

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm tempted to say that we should do 2 since that almost seems like what the purpose of replaceData is.

But I guess the safe thing to do is to do what 1.8 did until we can get a clarification from W3C.

Comment 17

[Boris Zbarsky [:bzbarsky]]

To be honest, if we keep the "both endpoints modified the same way" constraint, there is no sane way to do #2, as far as I can tell.  Throwing away that constraint would let us do better, and in any case that constraint relies on the remove/insert model of mutations in the spec...

Comment 18

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

"Note that when content is inserted at a boundary-point...the boundary-point will be positioned at the start of the newly inserted content."

That is what the patch does. If new text is inserted at the end point, the 
inserted text goes after that point. The same happens with the start point.
So in that sense the boundary points are handled similarly.

And .textContent doesn't matter here. One could use .nodeValue or .data etc.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The first question to ask is if replaceData is considered a single operation, or an delete and insert, or even an insert and delete.

Unfortunately the only paragraph that resonably talks about this is an informative one:

"Any mutation of the document tree which affect Ranges can be considered to be a combination of basic deletion and insertion operations. In fact, it can be convenient to think of those operations as being accomplished using the deleteContents() and insertNode() Range methods and, in the case of Text mutations, the splitText() and normalize() methods."

The first sentence seem to indicate that everything is built up of inserts and deletes (though it doesn't specify order). However the last few words would seem to indicate that bug 191864 is indeed a bug which means that normalize and splitText are atomic and not separate insert and delete.

Comment 20

[Boris Zbarsky [:bzbarsky]]

If we posit that splitText and normalize are the primitives to use for text, then this mutation can in fact be expressed in terms of these primitives and remove/insert.  It would look like so, with <> being textnodes and [] being the range endpoints:

  <Hello[!]>

splitText to split out the part that's changing from the part that's not:

  <><Hello[!]><>

remove the part that's changing:

  <>[]<>

Insert the new text:

  <>[]<Hello!><>

Normalize:

  <[]Hello!>

At least that's the only thing I can assume that paragraph means.

It really is too bad that it's informative and not normative.  If it were the latter, everything would be so much clearer...  It would also help if the range spec defined splitText and normalize behavior like it defines remove/insert, of course...  ;)

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, that would make sense to me. So using <= for both boundaries should fix that I think. If nothing else it seems to preserve compat with 1.8 so at least we're not breaking anyone unnecessarily.

Comment 22

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

1.8 doesn't have the "<= in start boundary" behavior. Nor does Opera.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Won't that mean that if you have a range like:

hello[]

And then set .textContent to "hello" you'll get a range like:

]hello[

I.e. with the end being before the start.

Comment 24

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

It becomes []hello

Comment 25

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

I mean on 1.8

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But that's not what you'd get with the attached patch, right?

Comment 27

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

The attached patch does the wrong ]hello[
If <= is used when checking startOffset, result is []hello.

With patched 1.9 (when using <= comparison with startOffset too) the difference 
to 1.8 is:
Range: Hel[lo!]
replaceData(1, 2, "MID") -> textnode is now HMIDlo!
1.8 result range is [lo!]
1.9 result range is [MIDlo!]
Which behavior do we want in this case? Current trunk gives [lo!], so does Opera.

Comment 28

[Boris Zbarsky [:bzbarsky]]

The 1.8 behavior in that case is correct.

Basically, the approach in that patch is wrong, imo.  If we want to treat the replace as a remove/insert, then we should be doing two separate checks:  First check on the removed position/length (and adjust our indices), second check on the  inserted position/length (and adjust again). I don't think it's worth trying to optimize that into a single check, though you're welcome to try.  But that single check needs to be provably equivalent to doing two checks.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why do you think that 1.8 is correct in that case? Following your syntax earlier:

<Hel[lo!]>
<H><el[><lo!]>
<H>[<lo!]>
<H>[<MID><lo!]>
<H[MIDlo!]>

Comment 30

[Boris Zbarsky [:bzbarsky]]

I'd think it's more like:

<Hel[lo!]>
<H><el><[lo!]>
<H><[lo!]>
<H><MID><[lo!]>
<HMID[lo!]>

But yes, that's not how it would work if we think of each text character as a child node of the text node, conceptually.  This spec is more of a mess than I thought. :(

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Right, I think using <= in both places gives the most consistent behavior. It would treat replace as a delete-and-insert but always insert to the right of any boundaries. Which is what the spec says to do when inserting children. Right now we insert chars to the left but children to the right. I think :)

Comment 32

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

Attachment: <= for both.

So the difference to 1.8 is MIDlo! testcase, but at least we're consistent.

Comment 33

[Ed Lee :Mardak]

Did you mean to post checkin and resolve fixed?

Comment 34

[Ed Lee :Mardak]

Well, my testcase attachment 301626 [details] reports success for all on trunk.