Last Comment Bug 191864 - Range and Selection broken with splitText() and normalize()
: Range and Selection broken with splitText() and normalize()
Status: RESOLVED FIXED
[qa-]
: dev-doc-complete, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P3 normal with 5 votes (vote)
: mozilla9
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 619273 679459 682463 684661 803924
Blocks: 30838 673108
  Show dependency treegraph
 
Reported: 2003-02-04 04:23 PST by Nathan Kurz
Modified: 2014-04-26 03:09 PDT (History)
23 users (show)
jonas: blocking1.9-
reed: wanted1.9+
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case for range and selection used with splitText() and normalize() (5.38 KB, text/html)
2003-02-04 04:25 PST, Nathan Kurz
no flags Details
part 1, update Range (13.29 KB, patch)
2011-08-01 16:29 PDT, Mats Palmgren (:mats)
jonas: review+
Details | Diff | Review
part 2, make newly created text frames draw as selected if they are (3.32 KB, patch)
2011-08-01 16:36 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
part 3, tests (6.96 KB, patch)
2011-08-01 16:36 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
part 1, rev 2, update Range (13.28 KB, patch)
2011-08-04 16:59 PDT, Mats Palmgren (:mats)
mats: review+
Details | Diff | Review
part 2, reftests (7.21 KB, patch)
2011-08-04 17:03 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
part 3, mochitest (9.63 KB, patch)
2011-08-04 17:18 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
part 3, mochitest (8.31 KB, patch)
2011-08-13 10:30 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
part 3, mochitest (8.23 KB, patch)
2011-08-14 12:59 PDT, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Review

Description Nathan Kurz 2003-02-04 04:23:32 PST
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 2003-02-04 04:25:39 PST
Created attachment 113484 [details]
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 2003-02-04 11:18:33 PST
anthonyd no longer works on range stuff.
Comment 3 Markus Hübner 2004-06-21 03:29:11 PDT
.
Comment 4 Darin Fisher 2005-05-17 16:44:29 PDT
Any chance of this bug being fixed for Gecko 1.8?
Comment 5 Alex Vincent [:WeirdAl] 2005-08-11 22:41:38 PDT
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 2005-08-12 00:07:54 PDT
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) PTO Until July 5th 2006-11-28 13:42:58 PST
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 2007-08-10 08:43:45 PDT
Nominating for 1.9 since we've head from webdevs that this is a cause of issues for them.
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2007-08-10 13:54:00 PDT
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 2007-08-13 11:01:39 PDT
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) PTO Until July 5th 2007-09-04 16:46:02 PDT
Not a blocker since this isn't a regression. But it'd certainly be nice to have fixed.
Comment 12 Kyle Butt 2007-09-24 06:43:14 PDT
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) 2008-01-05 00:16:27 PST
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 2008-01-16 07:22:38 PST
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 (:mats) 2011-08-01 16:29:05 PDT
Created attachment 549964 [details] [diff] [review]
part 1, update Range

This updates the Range boundaries as needed for splitText() and normalize().
Comment 16 Mats Palmgren (:mats) 2011-08-01 16:36:10 PDT
Created attachment 549969 [details] [diff] [review]
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 (:mats) 2011-08-01 16:36:51 PDT
Created attachment 549971 [details] [diff] [review]
part 3, tests
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-01 17:47:28 PDT
I think we can skip the workaround.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-01 20:59:06 PDT
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) PTO Until July 5th 2011-08-03 10:28:20 PDT
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] (high review load, please consider other reviewers) 2011-08-03 10:30:07 PDT
I think the new range spec is specifying this new behavior.
Comment 22 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-03 10:30:46 PDT
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 (:mats) 2011-08-03 10:53:52 PDT
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 (:mats) 2011-08-04 16:59:47 PDT
Created attachment 550898 [details] [diff] [review]
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 (:mats) 2011-08-04 17:03:16 PDT
Created attachment 550901 [details] [diff] [review]
part 2, reftests

Added workaround for painting bug per comment 23.
Comment 26 Mats Palmgren (:mats) 2011-08-04 17:18:01 PDT
Created attachment 550904 [details] [diff] [review]
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) PTO Until July 5th 2011-08-12 14:16:16 PDT
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 (:mats) 2011-08-13 10:30:21 PDT
Created attachment 552878 [details] [diff] [review]
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) PTO Until July 5th 2011-08-14 11:11:12 PDT
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 (:mats) 2011-08-14 12:59:07 PDT
Created attachment 553009 [details] [diff] [review]
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) 2011-08-15 03:24:47 PDT
Could you please email public-webapps@w3.org with the suggested changes Jonas?
Comment 32 Mats Palmgren (:mats) 2011-08-15 18:02:12 PDT
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 34 Eric Shepherd [:sheppy] 2011-08-24 08:52:21 PDT
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 (:mats) 2011-09-04 23:46:08 PDT
I filed bug 684661 on backing out this change from aurora, for causing regressions.
Comment 36 Mats Palmgren (:mats) 2011-09-15 17:13:31 PDT
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] (high review load, please consider other reviewers) 2011-09-20 01:24:29 PDT
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 (:mats) 2011-09-23 18:27:53 PDT
Landed the mochitest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f620801bc7
Comment 39 Matt Brubeck (:mbrubeck) 2011-09-24 08:38:27 PDT
https://hg.mozilla.org/mozilla-central/rev/55f620801bc7
Comment 40 Eric Shepherd [:sheppy] 2011-10-02 14:56:14 PDT
Did this re-land after the previous backing out? If so, on which channel?
Comment 41 Matt Brubeck (:mbrubeck) 2011-10-02 15:04:21 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.