Comments (21)
ayg
commented on August 15, 2024
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.
from dom.
ayg
commented on August 15, 2024
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.
from dom.
annevk
commented on August 15, 2024
Note that we previously went through this algorithm: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17541. And in #11 too.
from dom.
ayg
commented on August 15, 2024
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?
from dom.
annevk
commented on August 15, 2024
@travisleithead? I don't have strong opinions on this personally.
from dom.
travisleithead
commented on August 15, 2024
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?
from dom.
smaug----
commented on August 15, 2024
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.)
from dom.
ayg
commented on August 15, 2024
@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?
from dom.
ayg
commented on August 15, 2024
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?
from dom.
annevk
commented on August 15, 2024
from dom.
foolip
commented on August 15, 2024
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?
from dom.
ayg
commented on August 15, 2024
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?
from dom.
smaug----
commented on August 15, 2024
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.
from dom.
foolip
commented on August 15, 2024
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?
from dom.
ayg
commented on August 15, 2024
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).
from dom.
foolip
commented on August 15, 2024
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?
from dom.
ayg
commented on August 15, 2024
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.
from dom.
foolip
commented on August 15, 2024
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.
from dom.
ehsan
commented on August 15, 2024
@travisleithead Is Edge planning to change the behavior here as per #63 (comment) given @smaug----'s objection?
from dom.
smaug----
commented on August 15, 2024
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 ;) )
from dom.
ayg
commented on August 15, 2024
Firefox has changed to match the spec.
from dom.
Related Issues (20)
- Add extend and modify methods for the Range object
- Add an optional options dictionary to `closest` to allow jumping across shadow boundaries HOT 1
- Efficient DOM Removal with the removeElement Function
- Meta: a list of side effects that would be impacted by state-preserving move HOT 7
- Steps of removing a node definition about live range do not match with browsers if the range is in a shadow DOM and the shadow itself is removed HOT 2
- Node Tree Element potential child: Attr ? HOT 5
- Proposal: AbortSignal.none() HOT 4
- I can't find where inserting DocumentType nodes into Element nodes is disallowed HOT 1
- Proposal AbortSignal.prototype.filter(compare) HOT 4
- `mutationObserver.disconnect({ flush: true })` HOT 1
- Proposal: SelectorObserver HOT 2
- Proposal: elementsFromRect() HOT 2
- `MutationObserver` flag to observe mutations in (open) shadow trees to facilitate polyfills of HTML syntax HOT 9
- Request for mechanism to determine if children changed steps are called as a result of script or parser HOT 7
- Add a method like .addTeardown to AbortSignal
- Closed Shadow DOM blocks accessibility testing HOT 1
- Should post-connected steps be able to run multiple times during insertion?
- AbortSignal.any() assertion failure HOT 4
- Proposal: getCornerPoints() HOT 5
- Proposal: rate limiting event listeners (debounce / throttle) HOT 11
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from dom.