Bug 13843 - Define Range mutation for normalize()
Summary: Define Range mutation for normalize()
Status: RESOLVED FIXED
Alias: None
Product: WebAppsWG
Classification: Unclassified
Component: DOM (show other bugs)
Version: unspecified
Hardware: All All
: P2 normal
Target Milestone: ---
Assignee: Anne
QA Contact: public-webapps-bugzilla
URL:
Whiteboard:
Keywords:
Depends on: 13868
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-19 21:42 UTC by Aryeh Gregor
Modified: 2012-03-21 21:18 UTC (History)
8 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Aryeh Gregor 2011-08-19 21:42:00 UTC
We already special-case splitText().  normalize() should be special-cased too.  See Mozilla bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=191864
Comment 1 Olli Pettay 2011-08-19 21:57:07 UTC
Just to remind, for some reason normalize() is removed from
http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-core
atm

(I think normalize() is useful, and if splitText() has special case in
range spec, so should normalize() have. Personally I think neither of them
should be special cased, but I don't care too much)
Comment 2 Aryeh Gregor 2011-08-19 22:07:37 UTC
Hmm, that's weird.  normalize() is useful, I've used it.  It should be re-added.  (This might explain why I didn't special-case it, because I didn't see it in the spec.)
Comment 3 Aryeh Gregor 2011-08-22 19:48:20 UTC
Filed bug 13868.
Comment 4 Anne 2011-12-20 19:52:45 UTC
Is this done by virtue of reusing other algorithms?

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#dom-node-normalize
Comment 5 Aryeh Gregor 2011-12-27 20:14:43 UTC
Test-case:

data:text/html,<!DOCTYPE html>
<script>
var range = document.createRange();
document.head.appendChild(document.createTextNode("abc"));
range.setStart(document.head.lastChild, 2);
document.head.appendChild(document.createTextNode("def"));
range.setEnd(document.head.lastChild, 2);
document.head.normalize();
alert(range.startContainer + "," + range.startOffset + "," + range.endContainer + "," + range.endOffset);
</script>

Chrome 17 dev and Firefox 12.0a1 both alert "[object Text],2,[object Text],5" (so ab[cde]f stays selected).  Opera Next 12.00 alpha gives "[object Text],2,[object HTMLHeadElement],2" (changing selection to ab[cdef}).  I don't have IE to test.  The spec requires "[object Text],2,[object Text],6", as far as I read it (changing selection to ab[cdef]).  Basically, unless Gecko and WebKit are both willing to drop their special-case behavior, we should add similar behavior to the spec.
Comment 6 Anne 2011-12-28 09:23:03 UTC
IE9 gives "[object HTMLHeadElement],1,[object Text],3".
Comment 7 Aryeh Gregor 2011-12-28 16:41:43 UTC
In that case, I still say we go with Gecko/WebKit, since it's the most interoperable right now.  The spec doesn't match anyone.  The spec should probably go something like this:

"""
For each Text node descendant of the context object:

1. Let node be the Text node descendant.

2. Let previous sibling be node's previous sibling.

3. If previous sibling is not a Text node, continue with the next Text node descendant of the context object.

4. Let length be previous sibling's length attribute value.

4. Replace data with node previous sibling, offset length, count 0, and data node's data.

5. For each boundary point whose node is node, increase its offset by length and set its node to previous sibling.

6. Remove node.
"""

Does that sound good?  If so, I'll write some tests and make the change.
Comment 8 Anne 2011-12-28 16:47:35 UTC
If you have three Text-nodes the operation described does not seem ideal. Also, for mutation observers, do we want a single MutationRecord for each set of contiguous Text nodes affected? Because that would not work either with this description.
Comment 9 Aryeh Gregor 2011-12-28 17:24:47 UTC
Well, you need one MutationRecord for each CharacterData change anyway, but yes, we could get away with fewer childList changes than my text allows.  Although we'd have to define "remove" for multiple nodes at a time.  How about:

"""
For each Text node descendant of the context object:

1. Let node be the Text node descendant.

2. Let length be node's length attribute value.

3. Let data be the concatenation of the data of node's contiguous Text nodes (excluding itself), in tree order.

4. Replace data with node node, offset length, count 0, and data data.

5. Let current node be node's next sibling.

6. While current node is a Text node:

  1. For each boundary point whose node is current node, add length to its offset and set its node to node.

  2. Add current node's length attribute value to length.

  3. Set current node to its next sibling.

7. Remove node's contiguous Text nodes (excluding itself), in tree order.
"""
Comment 10 Anne 2011-12-28 17:45:41 UTC
My point was that you'd only need one "characterData" MutationRecord for three consecutive Text nodes. The comment 9 algorithm also replaces when there's no need. Other than that I suppose it's okay. (Until we add mutation observers.)
Comment 11 Anne 2012-03-21 12:09:31 UTC
Aryeh, can you have another look at this now we have mutation observers? I don't think there's a need to optimize removal anymore.
Comment 12 Aryeh Gregor 2012-03-21 19:16:16 UTC
Is there anything wrong with the algorithm in comment 9 now, relative to the current spec's algorithm?  If not, I'll change the spec.
Comment 13 Anne 2012-03-21 19:25:16 UTC
So why do you need to manually update the boundary points? Does concept-node-remove not take care of that?

Also currently the other text updating Ranges does not use the boundary points terminology. Still not quite sure whether it should.
Comment 14 Aryeh Gregor 2012-03-21 19:51:02 UTC
(In reply to comment #13)
> So why do you need to manually update the boundary points? Does
> concept-node-remove not take care of that?

That will collapse the boundary point to the end of the node.  See comment 5 -- the selected text should remain "ab[cde]f", but per spec it would become "ab[cdef]".

> Also currently the other text updating Ranges does not use the boundary points
> terminology. Still not quite sure whether it should.

Okay, I can modify it for that.
Comment 15 Aryeh Gregor 2012-03-21 21:18:07 UTC
https://bitbucket.org/ms2ger/dom-core/changeset/bd9c816a31da

No tests for now, but I added a TODO to the mutation tests.