-
Notifications
You must be signed in to change notification settings - Fork 166
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.
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
Add polyfills for select ParentNode APIs. #389
Changes from 21 commits
57d816f
6a82c7b
1c4f39d
cc7e026
5af15ea
43aa4a6
3358ac1
1f31671
615805e
6a3de6c
bf56cd0
b99798a
e1e498f
faf0511
a78100b
65134c3
d8c8854
d7e72c3
4c53900
ce8b140
210e89b
4dd308c
e915283
664abc2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -230,3 +230,17 @@ export const assign = (target, source) => { | |
export const arrayFrom = (object) => { | ||
return [].slice.call(/** @type {IArrayLike} */(object)); | ||
}; | ||
|
||
// Implements 'convert nodes into a node'. | ||
// https://dom.spec.whatwg.org/#converting-nodes-into-a-node | ||
export const convertNodesIntoANode = (...args) => { | ||
if (args.length === 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the only argument is a string? It won't get converted to a node in that case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix and add test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, should be fixed and tested now. |
||
return (typeof args[0] === 'string' ? document.createTextNode(args[0]) : args[0]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's factor this out since it's done 2x? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I moved it out. I also found that this expression doesn't exactly match what current browsers do so I updated it also. (AFAICT, the spec text for this doesn't say what happens to non-string, non-node values in this list but all browsers seem to just coerce to a string and make a text node from that string.) |
||
} | ||
|
||
const fragment = document.createDocumentFragment(); | ||
for (const arg of args) { | ||
fragment.appendChild(typeof arg === 'string' ? document.createTextNode(arg) : arg); | ||
} | ||
return fragment; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
<!doctype html> | ||
<!-- | ||
@license | ||
Copyright (c) 2020 The Polymer Project Authors. All rights reserved. | ||
This code may only be used under the BSD style license found at http://polymer.github.io/LICENSE.txt | ||
The complete set of authors may be found at http://polymer.github.io/AUTHORS.txt | ||
The complete set of contributors may be found at http://polymer.github.io/CONTRIBUTORS.txt | ||
Code distributed by Google as part of the polymer project is also | ||
subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt | ||
--> | ||
<html> | ||
<head> | ||
<title>ParentNode append</title> | ||
<script> | ||
for (const item of [ | ||
Document.prototype, | ||
DocumentFragment.prototype, | ||
Element.prototype, | ||
]) { | ||
delete item.append; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This and the others are here to remove the built-in implementations so that we can test it in browsers that actually do support it. I could also remove it if you think it's better to just run the tests against the built-in implementation, WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I missed the fact that this isn't a shadydom test. |
||
} | ||
</script> | ||
<script src="../../node_modules/@webcomponents/webcomponentsjs/bundles/webcomponents-pf_dom.js"></script> | ||
<script src="../wct-config.js"></script> | ||
<script src="../../node_modules/wct-browser-legacy/browser.js"></script> | ||
</head> | ||
<body> | ||
<script> | ||
suite('ParentNode append', function() { | ||
suite('Document', function() { | ||
test('Appends an element', () => { | ||
const doc = document.implementation.createHTMLDocument(''); | ||
while (doc.firstChild) { | ||
doc.removeChild(doc.firstChild); | ||
} | ||
|
||
const child = document.createElement('html'); | ||
doc.append(child); | ||
|
||
assert(doc.childNodes.length === 1); | ||
assert(doc.childNodes[0] === child); | ||
}); | ||
}); | ||
|
||
suite('DocumentFragment', function() { | ||
test('Appends an element', () => { | ||
const container = document.createDocumentFragment(); | ||
container.appendChild(document.createElement('div')); | ||
|
||
const child = document.createElement('div'); | ||
container.append(child); | ||
|
||
assert(child.parentNode === container); | ||
assert(container.childNodes.length === 2); | ||
assert(container.childNodes[1] === child); | ||
}); | ||
|
||
test('Appends a text node', () => { | ||
const container = document.createDocumentFragment(); | ||
container.appendChild(document.createElement('div')); | ||
|
||
container.append('This is some text.'); | ||
|
||
assert(container.childNodes.length === 2); | ||
assert(container.childNodes[1].nodeType === Node.TEXT_NODE); | ||
assert(container.childNodes[1].textContent === 'This is some text.'); | ||
}); | ||
|
||
test('Appends multiple types of nodes', () => { | ||
const container = document.createDocumentFragment(); | ||
container.appendChild(document.createElement('div')); | ||
|
||
const child1 = document.createElement('div'); | ||
const child3 = document.createElement('div'); | ||
container.append(child1, 'This is some text.', child3); | ||
|
||
assert(container.childNodes.length === 4); | ||
assert(container.childNodes[1] === child1); | ||
assert(container.childNodes[2].nodeType === Node.TEXT_NODE); | ||
assert(container.childNodes[2].textContent === 'This is some text.'); | ||
assert(container.childNodes[3] === child3); | ||
}); | ||
}); | ||
|
||
suite('Element', function() { | ||
test('Appends an element', () => { | ||
const container = document.createElement('div'); | ||
container.appendChild(document.createElement('div')); | ||
|
||
const child = document.createElement('div'); | ||
container.append(child); | ||
|
||
assert(child.parentNode === container); | ||
assert(container.childNodes.length === 2); | ||
assert(container.childNodes[1] === child); | ||
}); | ||
|
||
test('Appends a text node', () => { | ||
const container = document.createElement('div'); | ||
container.appendChild(document.createElement('div')); | ||
|
||
container.append('This is some text.'); | ||
|
||
assert(container.childNodes.length === 2); | ||
assert(container.childNodes[1].nodeType === Node.TEXT_NODE); | ||
assert(container.childNodes[1].textContent === 'This is some text.'); | ||
}); | ||
|
||
test('Appends multiple types of nodes', () => { | ||
const container = document.createElement('div'); | ||
container.appendChild(document.createElement('div')); | ||
|
||
const child1 = document.createElement('div'); | ||
const child3 = document.createElement('div'); | ||
container.append(child1, 'This is some text.', child3); | ||
|
||
assert(container.childNodes.length === 4); | ||
assert(container.childNodes[1] === child1); | ||
assert(container.childNodes[2].nodeType === Node.TEXT_NODE); | ||
assert(container.childNodes[2].textContent === 'This is some text.'); | ||
assert(container.childNodes[3] === child3); | ||
}); | ||
}); | ||
}); | ||
</script> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<!doctype html> | ||
<script src="../../../../node_modules/wct-browser-legacy/browser.js"></script> | ||
<script> | ||
WCT.loadSuites([ | ||
'./append.html', | ||
'./prepend.html', | ||
'./replace-children.html', | ||
]); | ||
</script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this the right thing to do here? I'm not totally sure what this list is about. There seems to be some stuff later in this file that implies that they're on the wrong prototype in older Safari and that this is just about putting them in the correct spot - should I consider doing this for ChildNode in #390 too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the concern was IE/Edge.
It would be good to verify (perhaps just via one-off manual testing rather than in the code itself) that these properties do not exist higher on the prototype chain than where we patch them on any browser. This would cause the patch to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the earliest version of the browsers implementing
append
(Edge 17, Firefox 49, Chrome 54, Safari 10) and they all have the descriptor onDocument
,DocumentFragment
, andElement
prototypes but not theHTMLDocument
,HTMLElement
, orShadowRoot
(if it exists) prototypes, so they should be alright assuming they haven't regressed and moved them around since.