Consider banning insertNode() of the Range's start node #63

ayg · 2015-08-11T18:09:21Z

Test-case:

<!doctype html>
foo
<script>
var range = document.createRange();
range.setStart(document.body.firstChild, 1);
var s = "Body children before: " + document.body.childNodes.length;
try {
  range.insertNode(range.startContainer);
  document.body.textContent = s + ", after: " + document.body.childNodes.length;
} catch (e) {
  document.body.textContent = "Exception";
}
</script>

Firefox outputs "Body children before: 2, after: 3", which matches the spec: there is no special handling of this case, and it winds up happily splitting the text node, then removing the first text node and re-inserting it back where it was, leaving the first portion no longer selected. Chrome throws. IE is the same as Firefox, but if you change the offset from 1 to 0 it outputs "Body children before: 2, after: 2", so it probably just aborts silently in that case.

IE/Firefox's behavior is just silly, and Chrome's seems a bit harsh. I'm inclined to go with Chrome, out of the options available. Failing that, I'd go with IE and at least special-case the scenario where we create an empty text node. Firefox (which is the current spec) would be my last choice.

ayg · 2015-08-11T18:14:55Z

More thorough test-case:

<!doctype html>
foo
<script>
var range = document.createRange();
range.setStart(document.body.firstChild, 1);
var s = "Body children before: " + document.body.childNodes.length;
try {
  range.insertNode(range.startContainer);
  document.body.textContent = s + ", after: " + document.body.childNodes.length + ".  Range: (" + range.startContainer.nodeName + " " + range.startOffset + ", " + range.endContainer.nodeName + " " + range.endOffset + ").";
} catch (e) {
  document.body.textContent = "Exception";
}
</script>

Firefox outputs "Body children before: 2, after: 3. Range: (BODY 0, BODY 2)." This is what you'd expect: both text nodes selected. Same if offset is changed from 1 to 0. IE outputs "Body children before: 2, after: 3. Range: (#text 0, BODY 2).", which is basically equivalent to Firefox. But if you change the offset to 0, IE gives "Body children before: 2, after: 2. Range: (#text 0, #text 4)." -- so it selects the inserted node, it doesn't abort silently. Presumably it has a special case to avoid splitting a text node at the start or end, which sounds like a good idea in general! I'll file a separate issue for that.

ayg · 2015-08-11T18:46:10Z

According to the spec comments that I wrote so long ago:

  <!-- Chrome 12 dev throws "HierarchyRequestError" if node is the same
  as the start node (at least for text nodes). This doesn't seem to make
  much sense, since insertBefore() works fine to move a node to its current
  position, and other browsers disagree, so the spec follows the majority.
  -->

I agree that throwing here is wrong, but just splitting the text node and adjusting the selection seems even worse.

annevk · 2015-08-11T18:53:41Z

Note that we previously went through this algorithm: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17541. And in #11 too.

ayg · 2015-08-12T11:20:20Z

Bah, I tried to search in the W3C bugs but didn't do it properly. Thanks for the pointer. Those changes improved things, but I don't think it fixed it fully.

On reflection, a couple of principles:

We should never split a text node if we're not going to put anything in between the two nodes. It's pointless and messy. insertNode is supposed to move nodes around, possibly splitting if necessary. It's not meant to split a node if not necessary for inserting something in between the halves. This rules out the behavior of IE and Firefox. If we're going to do anything here, we shouldn't split the text node.
Inserting a text node into the middle of itself doesn't make sense. We already throw if you try to insert an element if the start of the range is inside the element, so it makes sense to do the same for a text node. What sensible meaning does it have to insert a text node into itself? This argues in favor of Chrome's behavior. Although it's true that "insertBefore() works fine to move a node to its current position", that means if you try inserting it before or after itself, not inside itself!

So I've come to the conclusion that we should match Chrome here. @annevk @Ms2ger, what do you think?

annevk · 2015-08-12T14:20:44Z

@travisleithead? I don't have strong opinions on this personally.

travisleithead · 2015-09-08T21:48:25Z

This is very timely. Edge recently started failing a WPT range test due to a change we made to exit early from an insertNode operation when we detect the same node will be inserted in the same place at the start of the node (and we failed up update the range boundary points after splitting the text).

Given @ayg proposal above, I reviewed these principles with the team, who are strongly in agreement. In summary, we would definitely prefer to have the rules of DOM reject certain pointless operations, as the cost of an implicit split such as that caused in the above example have downstream performance impact via all the notifications that must be sent to update layout, collections, etc. We suspect that the web compat impact for making these change is minimal given Chrome currently throws under these conditions.

Principle 1 is great. We can consider it a bug any time an implicit split happens without inserting something in between.

I wonder if Principle 2 can be generalized a bit more. For example, does it ever make sense to insert a node from the context node's inclusive ancestry or vice-versa (where the node to be inserted is an inclusive descendent of the context node)? Should this apply to other operations besides insertNode? (e.g., replaceWith might be able to avoid some remove/insert combinations when it operates on itself)? Or is this taking the restriction too far?

smaug---- · 2015-10-07T16:55:37Z

I would have added principal 3 (which should be stronger than 1 or 2), be consistent whenever possible. No special cases unless absolutely needed. Inconsistencies make (a) implementations just more complicated and (b) tend to lead harder to read specs and (c) harder to understand APIs.

In other words, I don't understand the need for this change.
(Range API used to be internally more consistent but some hacks have been added to it over the time.)

ayg · 2015-10-07T17:46:18Z

@SmauG The change improves consistency from a certain perspective. If the range's start is inside a non-text node and you try to insert the start node, it will throw. With this change, if it's in a text node and you try to insert the start node, it will also throw. What is the advantage in consistency in the other way? Why should text nodes behave differently from elements?

ayg · 2015-10-07T17:53:27Z

Also, and this should go without saying -- the number one principle is always that we should try to get interop. It seems IE wants to switch to WebKit/Blink behavior, and Gecko wants to remain with IE/Gecko behavior. How willing are WebKit/Blink to change to IE/Gecko behavior? If they're okay with that, then we should go back to speccing that. If not, then IMO the spec should go with the majority and remain as it is now (WebKit/Blink + IE), and Gecko should change.

Does anyone know who to ask in WebKit- or Blink-land on whether they're interested in changing?

annevk · 2015-10-08T10:14:49Z

@foolip?

foolip · 2015-10-08T11:11:11Z

This code doesn't change often in Blink:
https://chromium.googlesource.com/chromium/src/+blame/master/third_party/WebKit/Source/core/dom/Range.cpp

However, I think @yosin-chromium does work on the editing code and may have some thoughts.

There are a bunch of cases where HierarchyRequestError is thrown and it doesn't look like the sum of them add up to precisely what the spec now says in https://dom.spec.whatwg.org/#concept-range-insert

What are the exact suggestions on the table right now?

ayg · 2015-10-08T11:17:01Z

The question currently being discussed is: if the start node of the range is a text node, and you do range.insertNode(range.startContainer), what should happen? IE/Gecko split the text node and then actually insert the first bit into its present location. WebKit/Blink throw. @travisleithead is in favor of the WebKit/Blink behavior, @SmauG is in favor of the IE/Gecko behavior. The question is, are WebKit and Blink strongly attached to their current behavior, do they strongly want to change to match IE/Gecko, or are they indifferent?

smaug---- · 2015-10-08T11:37:03Z

To be more precise, I'm in favor of adding as few special cases as possible. The simpler we can keep the APIs the better.

foolip · 2015-10-08T11:49:48Z

So here's the code that throws in Blink in the test case from #63 (comment)

    for (Node* n = m_start.container(); n; n = n->parentNode()) {
        if (n == newNode) {
            exceptionState.throwDOMException(HierarchyRequestError, "The node to be inserted contains the insertion point; it may not be inserted into itself.");
            return;
        }
    }

In other words, being a text node isn't relevant here, if the start node of the range is an element, the same thing happens. Also note the traversal of parents of the start node.

(There's another HierarchyRequestError that seems to match the spec's "is a Text node whose parent is null" condition, and yet more still that I'm not sure about.)

Is the IE/Gecko behavior really exactly what the spec said before this change, or is it actually a mess as well?

ayg · 2015-10-08T12:11:44Z

Sorry for being unclear -- all browsers always threw if you did this with a non-text node, so I specified text node. The insert validity checks will throw if it's a non-text node, or if you try to insert an ancestor of the start node, so the difference is only for text nodes.

Gecko's behavior may be exactly what the spec said before. IE is not exactly the same, but is roughly (it has at least one or two extra special cases).

foolip · 2015-10-08T13:19:36Z

all browsers always threw if you did this with a non-text node

Expressed like that, it sounds like also throwing for text nodes makes this less special. Do other browsers have something like the code in #63 (comment) but explicitly excluding text nodes, or what's going on here?

ayg · 2015-10-08T13:35:52Z

In the non-text node case, you're trying to insert a node as a descendant of itself. That will obviously have to throw a HierarchyRequestError in any UA, just like it does if you do it via insertBefore or whatever. This requires no additional logic in insertNode, beyond insertion validity checks that must be present when you do any insertion. In the text node case you're splitting the node and trying to insert it into its current parent, which is perfectly valid in principle, and needs special logic in insertNode if we don't want to allow it.

foolip · 2015-10-08T13:55:54Z

OK, right, some of the checks are part of https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity

At the end of the day I don't have a strong opinion on how this should work, but I want it to be interoperable, and if everbody wants a behavior that differs from Blink, I can try to guess how risky the change would be. It's pretty unusual to depend on exceptions being thrown, so I wouldn't be too worried about breakage.

ehsan · 2016-08-08T15:52:54Z

@travisleithead Is Edge planning to change the behavior here as per #63 (comment) given @smaug----'s objection?

smaug---- · 2016-08-08T16:04:49Z

Well, if others have some particular behavior, I guess Gecko needs to follow. This is an edge case, so I can live with it.

(Overall the quality and consistency of Range API has decreased over time. It all started with buggy Acid 3, which still has a comment about smaug complaining something in line 538 ;) )

ayg · 2016-08-16T13:22:45Z

Firefox has changed to match the spec.

ayg added a commit to ayg/dom that referenced this issue Oct 6, 2015

Fix whatwg#63: Ban insertNode() of Range's start node

055aaf2

annevk closed this as completed in abad55d Oct 6, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider banning insertNode() of the Range's start node #63

Consider banning insertNode() of the Range's start node #63

ayg commented Aug 11, 2015

ayg commented Aug 11, 2015

ayg commented Aug 11, 2015

annevk commented Aug 11, 2015

ayg commented Aug 12, 2015

annevk commented Aug 12, 2015

travisleithead commented Sep 8, 2015

smaug---- commented Oct 7, 2015

ayg commented Oct 7, 2015

ayg commented Oct 7, 2015

annevk commented Oct 8, 2015

foolip commented Oct 8, 2015

ayg commented Oct 8, 2015

smaug---- commented Oct 8, 2015

foolip commented Oct 8, 2015

ayg commented Oct 8, 2015

foolip commented Oct 8, 2015

ayg commented Oct 8, 2015

foolip commented Oct 8, 2015

ehsan commented Aug 8, 2016

smaug---- commented Aug 8, 2016

ayg commented Aug 16, 2016

Consider banning insertNode() of the Range's start node #63

Consider banning insertNode() of the Range's start node #63

Comments

ayg commented Aug 11, 2015

ayg commented Aug 11, 2015

ayg commented Aug 11, 2015

annevk commented Aug 11, 2015

ayg commented Aug 12, 2015

annevk commented Aug 12, 2015

travisleithead commented Sep 8, 2015

smaug---- commented Oct 7, 2015

ayg commented Oct 7, 2015

ayg commented Oct 7, 2015

annevk commented Oct 8, 2015

foolip commented Oct 8, 2015

ayg commented Oct 8, 2015

smaug---- commented Oct 8, 2015

foolip commented Oct 8, 2015

ayg commented Oct 8, 2015

foolip commented Oct 8, 2015

ayg commented Oct 8, 2015

foolip commented Oct 8, 2015

ehsan commented Aug 8, 2016

smaug---- commented Aug 8, 2016

ayg commented Aug 16, 2016