Skip to content

fix: re-index childNodes on insertBefore #766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 26, 2024
Merged

fix: re-index childNodes on insertBefore #766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
686a301
fix: re-index childNodes on insertBefore
karfau Oct 23, 2024
2bf0333
test: assert more details
karfau Oct 26, 2024
147905b
docs: explain code in _onUpdateChild
karfau Oct 26, 2024
2a4c6e5
Merge remote-tracking branch 'origin/master' into fix/insertBefore-ap…
karfau Oct 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions lib/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1623,37 +1623,39 @@ function _onRemoveAttribute(doc, el, newAttr, remove) {
}

/**
* Updates `el.childNodes`, adjusting the indexed items and its `length`.
* If `newChild` is provided, it will be appended to the childNodes list.
* Otherwise, it's assumed that an item has been removed,
* and `el.firstNode` and its `.nextSibling` are used to iterate over the current list of child
* nodes, effectively reindexing them.
* Updates `parent.childNodes`, adjusting the indexed items and its `length`.
* If `newChild` is provided and has no nextSibling, it will be appended.
* Otherwise, it's assumed that an item has been removed or inserted,
* and `parent.firstNode` and its `.nextSibling` to re-indexing all child nodes of `parent`.
*
* @param {Document} doc
* The parent document of `el`.
* @param {Node} el
* @param {Node} parent
* The parent node whose childNodes list needs to be updated.
* @param {Node} [newChild]
* The new child node to be appended. If not provided, the function assumes a node has been
* removed.
* @private
*/
function _onUpdateChild(doc, el, newChild) {
function _onUpdateChild(doc, parent, newChild) {
if (doc && doc._inc) {
doc._inc++;
//update childNodes
var cs = el.childNodes;
if (newChild) {
cs[cs.length++] = newChild;
var childNodes = parent.childNodes;
// assumes nextSibling and previousSibling were already configured upfront
if (newChild && !newChild.nextSibling) {
// if an item has been appended, we only need to update the last index and the length
childNodes[childNodes.length++] = newChild;
} else {
var child = el.firstChild;
// otherwise we need to reindex all items,
// which can take a while when processing nodes with a lot of children
var child = parent.firstChild;
var i = 0;
while (child) {
cs[i++] = child;
childNodes[i++] = child;
child = child.nextSibling;
}
cs.length = i;
delete cs[cs.length];
childNodes.length = i;
delete childNodes[childNodes.length];
}
}
}
Expand Down
27 changes: 27 additions & 0 deletions test/dom/document.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,33 @@ describe('Document.prototype', () => {
expect(doc.childNodes).toHaveLength(0);
expect(root.parentNode).toBeNull();
});
test('should insert doctype between processing instruction and element', () => {
const doc = new DOMImplementation().createDocument(null, '');
expect(doc.childNodes).toHaveLength(0);
expect(doc.documentElement).toBeNull();

const instruction = doc.createProcessingInstruction('target', 'data');
doc.appendChild(instruction);

const root = doc.createElement('root');
doc.appendChild(root);
expect(doc.childNodes).toHaveLength(2);
expect(doc.childNodes.item(0)).toBe(instruction);
expect(doc.childNodes.item(1)).toBe(root);

const doctype = doc.implementation.createDocumentType('qualifiedName', '', '');
doc.insertBefore(doctype, root);
expect(doc.childNodes).toHaveLength(3);
expect(instruction.previousSibling).toBeNull();
expect(doc.childNodes.item(0)).toBe(instruction);
expect(instruction.nextSibling).toBe(doctype);
expect(doctype.previousSibling).toBe(instruction);
expect(doc.childNodes.item(1)).toBe(doctype);
expect(doctype.nextSibling).toBe(root);
expect(root.previousSibling).toBe(doctype);
expect(doc.childNodes.item(2)).toBe(root);
expect(root.nextSibling).toBeNull();
});
});
describe('replaceChild', () => {
test('should remove the only element and add the new one', () => {
Expand Down
10 changes: 5 additions & 5 deletions test/dom/element.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@ const { DOMException, DOMExceptionName } = require('../../lib/errors');
const { expectDOMException } = require('../errors/expectDOMException');

describe('documentElement', () => {
describe('constructor', () => {
test('should throw Illegal constructor TypeError when trying to access constructor directly', () => {
expect(() => new Element()).toThrow(TypeError);
});
});
test('can properly append exist child', () => {
const doc = new DOMParser().parseFromString(
'<xml xmlns="http://test.com" id="root">' +
Expand Down Expand Up @@ -157,6 +152,11 @@ describe('Element', () => {
const ATTR_LOWER_CASE = 'attr';
const VALUE = '2039e2dk';
describe('constructor', () => {
test('should throw Illegal constructor TypeError when trying to access constructor directly', () => {
expect(() => new Element()).toThrow(TypeError);
});
});
test('_nsMap has no prototype properties', () => {
const element = new DOMImplementation().createDocument(null, 'doc').documentElement;
expect(element._nsMap).not.toHaveProperty('prototype');
expect(element._nsMap).not.toHaveProperty('__proto__');
Expand Down