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])

Testcase passes in firefox 2

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

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.

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.

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

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

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.

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.

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.

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

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

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

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.

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

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

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.

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... ;)

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.

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

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.

It becomes []hello

I mean on 1.8

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

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.

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.

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!]>

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

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 :)

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

Did you mean to post checkin and resolve fixed?

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