191864 - Range and Selection broken with splitText() and normalize()

Status: RESOLVED FIXED

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

Description

[Nathan Kurz]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5

The DOM spec says that ranges should stay as constant as possible while the
document contents mutate
(http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Mutation)
but range.splitText() and node.normalize() break this.  And since the selection
object uses an underlying range, it too is broken.

After a splitText(), any part of the range or selection that should be in the
second (new) text node is not.  After a normalize(), any part of the selection
or range not in (what was) the first text node is no longer selected or included.

Elaborate test case follows.

Elaborate test case follows.



Reproducible: Always

Steps to Reproduce:

Comment 1

[Nathan Kurz]

Attachment: test case for range and selection used with splitText() and normalize()

Here's a test case with directions for reproducing the bug regarding the
interaction of range and selection objects with the range.splitText() and
node.normalize() methods.

Comment 2

[kinmoz]

anthonyd no longer works on range stuff.

Comment 3

[Markus Hübner]

.

Comment 4

[Darin Fisher]

Any chance of this bug being fixed for Gecko 1.8?

Comment 5

[Alex Vincent [:WeirdAl]]

This testcase is partially invalid.  Clicking on the buttons changes the
selection.  You have to type in javascript:viewSelection() and friends manually
into the location bar.

With that taken into effect, this bug is WFM.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050811
SeaMonkey/1.0a

Comment 6

[Nathan Kurz]

Thanks for looking at this.  I'm the original reporter, but haven't really
thought about this since reporting it.  I recall it was a showstopper for me at
the time, though.

Could you clarify exactly how it works for you, step by step in the directions?
 You are saying that the range stays the same (as expected) when you type the
javascript: into the location bar, but changes when you use the buttons?  Is
this necessary for all steps, or only some?

I still see the problem with Firefox 1.0, but don't have a current version to
try.   And is there any reason that clicking on a button _should_ change the
selection?  Because if so, that seems like a new bug that should be added and
cross-referenced.  But if the problem has actually been fixed, great!  

Comment 7

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

FWIW, the way to fix this would be to add a notification to nsIMutationObserver that is sent out after the mutation is done that notifies ranges that a split or a normalize was just done and that the range needs to update itself.

Comment 8

[Mike Schroepfer]

Nominating for 1.9 since we've head from webdevs that this is a cause of issues for them.

Comment 9

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

Ideally we would get clarification from the w3c what the desired behavior is. The only thing that would indicate that this is an actual bug is the very fluffy paragraph:

There are two general principles which apply to Ranges under document mutation: The first is that all Ranges in a document will remain valid after any mutation operation and the second is that, as much as possible, all Ranges will select the same portion of the document after any mutation operation.

So it's not more than a "general pricipal". It's alway questionable what "any mutation" means. Is splitText() one or two mutations? We've implemented it on top of the normal DOM APIs which forces it to be two. Which is why this bug exists.

Comment 10

[David W. Jeske]

I know I'm not the w3c, but the quoted paragraph seems to pretty matter-of-factly explain why this is a bug, and the assertion of ambiguity provided by Jonas is not convincing to me. 

"any mutation" means "any mutation operation provided by the DOM". Of course JS developers can write mutations that will move information outside of the view of the text-range code, and thus break range tracking. This is why it's so important for the built-in text range APIs to be comprehensive, and to have them retain ranges across operations. With this, JS developers can properly use the range APIs and avoid breaking selection or existing ranges.

The current splitText() is implemented as if an external JS developer had to force implement it without connection to the underlying ranges. In order to fit in with the "general principal" it should be made an operation which works in concert with the range-code and behaves as other range operations do. 

Comment 11

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

Not a blocker since this isn't a regression. But it'd certainly be nice to have fixed.

Comment 12

[Kyle Butt]

Any ambiguity in the spec should be cleared up by the next 2 sections: Insertions and Deletion. The text says that:

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

so inserting an empty span, which consists of a splitText and an insertNode, should, as demonstrated by the examples leave the selection intact.

Comment 13

[Hixie (not reading bugmail)]

The spec doesn't actually define how to handle a splitText(). If a splitText() consists of a truncation of one text node and the insertion of another, then the selection should end up not containing the second part.

At least that's what I think the spec says. It seems dumb to me.

Comment 14

[Kyle Butt]

Yes, it does:

There are two general principles which apply to Ranges under document mutation: The first is that all Ranges in a document will remain valid after any mutation operation and the second is that, as much as possible, all Ranges will select the same portion of the document after any mutation operation.

The text in the document doesn't change after a splitText, so neither should the selection.

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 1, update Range

This updates the Range boundaries as needed for splitText() and normalize().

Comment 16

[Mats Palmgren (inactive)]

Attachment: part 2, make newly created text frames draw as selected if they are

This is just a temporary workaround until we move the "selected bit" to
content (bug 619273).  I guess we could skip this part until bug 619273
is fixed if we think we can live with the painting error until then...
It's needed for the reftest to pass though.

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 3, tests

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we can skip the workaround.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 549964 [details] [diff] [review]
part 1, update Range

Going to punt this to sicking...

Jonas, if you really want me to review this, let me know.

Comment 20

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

I actually think that what we're currently doing is the correct behavior per spec.

However I agree that this new behavior is more useful and is what should be specced. Could someone make sure that the new DOM Core spec is updated to reflect this behavior.

Also, please write mochitests that work directly with the DOM that checks that a range has the correct state after a splitText and normalize call for all the various edge cases here.

Comment 21

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

I think the new range spec is specifying this new behavior.

Comment 22

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

Comment on attachment 549964 [details] [diff] [review]
part 1, update Range

Review of attachment 549964 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but I'd like to also review a mochitest that tests through the various edge cases.

::: content/base/public/nsIMutationObserver.h
@@ +96,5 @@
> +
> +  struct Details {
> +    enum {
> +      kMerge,  // two text nodes are merged as a result of normalize()
> +      kSplit   // a text node is split as a result of splitText()

Nit: I think we usually use 'e' as a prefix for enums. But I don't care that strongly.

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 549969 [details] [diff] [review]
part 2, make newly created text frames draw as selected if they are

(In reply to comment #18)
> I think we can skip the workaround.

Ok, I think it should be easy enough to workaround in script, at least
for the common case of the primary selection.  Eg. copy all the ranges
to an array, removeAllRanges(), add them back with addRange().

Comment 24

[Mats Palmgren (inactive)]

Attachment: part 1, rev 2, update Range

> Nit: I think we usually use 'e' as a prefix for enums. But I don't care that
> strongly.

Fixed.

Comment 25

[Mats Palmgren (inactive)]

Attachment: part 2, reftests

Added workaround for painting bug per comment 23.

Comment 26

[Mats Palmgren (inactive)]

Attachment: part 3, mochitest

Brief overview:
1. save original text nodes in document.original_nodes
2. setup the selection
3. perform splitText() and/or normalize()
4. verify that most child nodes are the same as original_nodes, their
   textContent is expected, and that the number of child nodes is
   the expected
5. verify Selection range nodes/offsets

Comment 27

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

Comment on attachment 550904 [details] [diff] [review]
part 3, mochitest

Review of attachment 550904 [details] [diff] [review]:
-----------------------------------------------------------------

This test is a lot more complicated than it needs to be. First off, testing ranges doesn't need to involve selection. It feels much safer to me to use a plain range object that doesn't involve selection at all.

Second, why do you need to mess with frames and such?

Third, why not have a simple for loop that runs through all tests? There seems to be a lot of infrastructure there just to drive what is essentially just running through an array of test parameters.

Comment 28

[Mats Palmgren (inactive)]

Attachment: part 3, mochitest

> First off, testing
> ranges doesn't need to involve selection. It feels much safer to me to use a
> plain range object that doesn't involve selection at all.

Fixed - I removed all uses of Selection, just using Range now.

> Second, why do you need to mess with frames and such?

The mochitest was derived from the earlier reftest that needed separate
Selections to test the rendering of each case independently.

Fixed - I removed all use of IFRAMEs.

> Third, why not have a simple for loop that runs through all tests?

Fixed.

Comment 29

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

Comment on attachment 552878 [details] [diff] [review]
part 3, mochitest

Review of attachment 552878 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/test/test_selection_splitText-normalize.html
@@ +49,5 @@
> +  }
> +
> +  // setup the range
> +  let sel = t[0]
> +  let startNode = sel.startNode === undefined ? p.firstChild : p.childNodes[sel.startNode];

None of the tests set startNode. Is that intentional?

@@ +59,5 @@
> +
> +  // splitText
> +  let split = t[1]
> +  let c = p.childNodes[split[0]];
> +  p.childNodes[split[0]].splitText(split[1])

c is unused

@@ +94,5 @@
> + [ {endNode:2},                               [ [0, "0"], [1, "1234"], "5678" ]],
> + [ {endNode:3},                               [ [0, "0"], [1, "1234"], "5", [2, "678"] ]],
> +/* test_split_merge */
> + [ {},                                        [ [0, "012345678" ] ]],
> + [ {startParent:true},                        [ "012345678" ]],     /* hmm, shouldn't this normalize back into the original child[0] ? */

If I understand this test correctly, this ends up making the original textnode be an empty textnode placed a newly created textnode which contains "012345678". normalize() is defined to toss out all empty textnodes and merge remaining adjecent textnodes.

Though I agree that that results in behavior which is somewhat inconsistent. Let me know if you want me to raise this on the webapps list.

Comment 30

[Mats Palmgren (inactive)]

Attachment: part 3, mochitest

(In reply to Jonas Sicking (:sicking) from comment #29)
> None of the tests set startNode. Is that intentional?

Yeah, it wasn't needed for any test.  Fixed:
+  let startNode = p.firstChild;

> c is unused

Removed.

> If I understand this test correctly, this ends up making the original
> textnode be an empty textnode placed a newly created textnode which contains
> "012345678". normalize() is defined to toss out all empty textnodes and
> merge remaining adjecent textnodes.

Correct.  I changed the comment to just document why it's different:
/* splitText() creates an empty first child which is removed by normalize() */

> Though I agree that that results in behavior which is somewhat inconsistent.
> Let me know if you want me to raise this on the webapps list.

I don't think we should change it at this point.
We're compatible with Chrome and Opera on this.
IE seems to always create a new text node when normalizing.

Comment 31

[Anne (:annevk)]

Could you please email [e-mail] with the suggested changes Jonas?

Comment 32

[Mats Palmgren (inactive)]

I pushed the fix + reftest to inbound to get it fixed for mozilla8:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4d046f0cdcbd
http://hg.mozilla.org/integration/mozilla-inbound/rev/4c6dfeb5dc3a

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/4d046f0cdcbd
http://hg.mozilla.org/mozilla-central/rev/4c6dfeb5dc3a

Comment 34

[Eric Shepherd [:sheppy]]

Since this appears to just be a bug fix, rather than a change to expected behavior, I've simply listed this fix on Firefox 8 for developers. If you feel that more is needed, please let me know.

Comment 35

[Mats Palmgren (inactive)]

I filed bug 684661 on backing out this change from aurora, for causing regressions.

Comment 36

[Mats Palmgren (inactive)]

Comment on attachment 553009 [details] [diff] [review]
part 3, mochitest

Olli, perhaps you could review this one as well?
It passes with the latest patches in bug 682463.

Comment 37

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

Comment on attachment 553009 [details] [diff] [review]
part 3, mochitest


>+  if (sel.todo) {
>+    alert(r.startContainer + '\n' + r.startOffset + '\n' + r.endContainer + '\n' + r.endOffset + '\n')
>+    return;
>+  }
Remove or comment out this.

Kind of hard to review without comments.
rs=me

Comment 38

[Mats Palmgren (inactive)]

Landed the mochitest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f620801bc7

Comment 39

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/55f620801bc7

Comment 40

[Eric Shepherd [:sheppy]]

Did this re-land after the previous backing out? If so, on which channel?

Comment 41

[Matt Brubeck (:mbrubeck)]

(In reply to Eric Shepherd [:sheppy] from comment #40)
> Did this re-land after the previous backing out? If so, on which channel?

This originally landed in Firefox 8, but it was backed out from Firefox 8 while Firefox 8 was on Aurora.

The patches are still in Firefox 9 (currently Aurora) and 10 (Nightly), because they were never backed out on the mozilla-central/nightly channel.