Add polyfills for select ParentNode APIs. #389

bicknellr · 2020-10-07T01:55:39Z

Adds polyfills for ParentNode's append, prepend, and removeChildren functions and wrappers to support these in Shady DOM.

…d be reentrant.

…ned.

I need to figure out what to do about all of the tests that use @webcomponents/platform. It is ostensibly where the ParentNode polyfills would go, but we no longer vend it or any other way to get these polyfills outside of a complete bundle. For now, importing them directly from the other package's build output seems to work.

bicknellr · 2020-10-08T19:01:21Z

packages/shadydom/src/patch-native.js

+  'querySelectorAll',
+  'append',
+  'prepend',
+  'replaceChildren',


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?

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.

I checked the earliest version of the browsers implementing append (Edge 17, Firefox 49, Chrome 54, Safari 10) and they all have the descriptor on Document, DocumentFragment, and Element prototypes but not the HTMLDocument, HTMLElement, or ShadowRoot (if it exists) prototypes, so they should be alright assuming they haven't regressed and moved them around since.

packages/shadydom/src/patches/ParentNode.js

bicknellr · 2020-10-08T21:29:04Z

Another issue regarding MutationObserver: these functions are meant to create one entry, but this implementation probably creates many. Is that something we care about?

…tationObserver entries when inserting many nodes.

bicknellr · 2020-10-08T22:47:45Z

So following the spec more closely actually results in this being less work. The 'convert nodes into a node' algorithm basically just takes a list of nodes and puts them all in a fragment if there's not exactly one item, which can then be passed as one item to Shady DOM's insertBefore. This doesn't solve the multiple MutationObserver entries problem for replaceChildren though.

sorvell · 2020-10-12T18:54:50Z

packages/shadydom/src/utils.js

+// Implements 'convert nodes into a node'.
+// https://dom.spec.whatwg.org/#converting-nodes-into-a-node
+export const convertNodesIntoANode = (...args) => {
+  if (args.length === 1) {


What if the only argument is a string? It won't get converted to a node in that case.

Fix and add test?

Good catch, should be fixed and tested now.

sorvell · 2020-10-12T18:56:49Z

packages/tests/webcomponentsjs_/parent-node/append.html

+      DocumentFragment.prototype,
+      Element.prototype,
+    ]) {
+      delete item.append;


Why is this needed?

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?

OK, I missed the fact that this isn't a shadydom test.

sorvell · 2020-10-12T18:58:00Z

packages/tests/webcomponentsjs_/parent-node/append.html

+</head>
+<body>
+  <script>
+    const testAppend = (createContainer) => {


It's simpler to keep the suite with the test functions. Let's get rid of this wrapper function. Same with other test files.

Ok, I inlined these.

sorvell · 2020-10-12T18:58:17Z

packages/tests/webcomponentsjs_/parent-node/prepend.html

+      DocumentFragment.prototype,
+      Element.prototype,
+    ]) {
+      delete item.prepend;


Why is this needed?

(replied in earlier comment)

sorvell · 2020-10-12T18:58:40Z

packages/tests/webcomponentsjs_/parent-node/prepend.html

+</head>
+<body>
+  <script>
+    const testAppend = (createContainer) => {


testPrepend

(Removed based on another comment in your review.)

sorvell · 2020-10-12T18:59:28Z

packages/tests/webcomponentsjs_/parent-node/prepend.html

+<body>
+  <script>
+    const testAppend = (createContainer) => {
+      test('Appends an element', () => {


These titles should say "Prepends"... same for all tests here.

sorvell · 2020-10-12T19:00:20Z

packages/tests/webcomponentsjs_/parent-node/replace-children.html

+      DocumentFragment.prototype,
+      Element.prototype,
+    ]) {
+      delete item.replaceChildren;


Why is this needed?

(replied in earlier comment)

sorvell · 2020-10-12T19:00:35Z

packages/tests/webcomponentsjs_/parent-node/replace-children.html

+</head>
+<body>
+  <script>
+    const testAppend = (createContainer) => {


testReplaceWith

(Removed based on another comment in your review.)

sorvell · 2020-10-12T19:00:51Z

packages/tests/webcomponentsjs_/parent-node/replace-children.html

+<body>
+  <script>
+    const testAppend = (createContainer) => {
+      test('Appends an element', () => {


Titles of these tests should say "ReplaceWith"

sorvell · 2020-10-12T19:01:59Z

packages/webcomponentsjs/ts_src/platform/parent-node/append.ts

@@ -0,0 +1,37 @@
+/**


Can you explain why these APIs are being added here rather than just in shadydom?

This is to separate the polyfill for the feature itself from the part that adds support for the feature to Shady DOM. In this case (and for ChildNode), it was easier not to wrap the existing versions of these functions (built-in or polyfilled) since everything goes through Shady DOM's implementation of insertBefore and removeChild anyway, so Shady DOM doesn't actually use them. However, without them, we won't have support for these features without enabling Shady DOM.

I'm just not sure this is worth doing since there is no browser that has Shadow DOM but does not have ParentNode/ChildNode.

I decided to check out caniuse.com and Chrome 53 is the only browser that has Shadow DOM v1 but not ParentNode. Should I remove these?

On that note, I wonder if it's time to drop Chrome 41 from the polyfill tests.

I didn't realize that there was any browser that had this configuration. It seems ok then.

…n passing a single string.

sorvell · 2020-10-14T14:45:00Z

packages/shadydom/src/patch-native.js

+  'querySelectorAll',
+  'append',
+  'prepend',
+  'replaceChildren',


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.

sorvell · 2020-10-14T14:51:18Z

packages/shadydom/src/utils.js

+// https://dom.spec.whatwg.org/#converting-nodes-into-a-node
+export const convertNodesIntoANode = (...args) => {
+  if (args.length === 1) {
+    return (typeof args[0] === 'string' ? document.createTextNode(args[0]) : args[0]);


Let's factor this out since it's done 2x?

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.)

sorvell · 2020-10-14T14:52:01Z

packages/tests/webcomponentsjs_/parent-node/append.html

+      DocumentFragment.prototype,
+      Element.prototype,
+    ]) {
+      delete item.append;


OK, I missed the fact that this isn't a shadydom test.

sorvell · 2020-10-14T14:54:17Z

packages/webcomponentsjs/ts_src/platform/parent-node/append.ts

@@ -0,0 +1,37 @@
+/**


I'm just not sure this is worth doing since there is no browser that has Shadow DOM but does not have ParentNode/ChildNode.

…ow when trying to append a symbol.

bicknellr · 2020-10-15T20:08:04Z

packages/shadydom/src/utils.js

+ * @return {!Node}
+ */
+const convertIntoANode = (arg) => {
+  return !(arg instanceof Node) ? document.createTextNode(String(arg)) : arg;


FYI, I removed this explicit conversion to string because browsers don't seem to do this and will throw when trying to append a symbol.

bicknellr added 16 commits October 1, 2020 11:41

Add MDN snippets for ParentNode's append and prepend.

57d816f

Add test stubs for ParentNode append and prepend.

6a82c7b

Add tests for append.

1c4f39d

Add tests for prepend.

cc7e026

Test for append vs prepend behavior.

5af15ea

IE11 requires an argument for createHTMLDocument.

43aa4a6

Write custom append / prepend polyfills because the MDN snippets woul…

3358ac1

…d be reentrant.

In Safari 9, the firstChild descriptor's get and set are undefi…

1f31671

…ned.

In Chrome 41, firstChild is a per-instance value descriptor.

615805e

Polyfill replaceChildren.

6a3de6c

Shady DOM: Add basic wrappers for append, prepend, and replaceChildren.

bf56cd0

Shady DOM: Add tests for append, prepend, and replaceChildren.

b99798a

Prevent Closure from renaming replaceChildren.

faf0511

The linter doesn't like raw assignments as conditionals.

a78100b

Merge remote-tracking branch 'origin/master' into parent-node

65134c3

googlebot added the cla: yes label Oct 7, 2020

Update changelogs.

d8c8854

bicknellr marked this pull request as ready for review October 7, 2020 20:55

bicknellr requested a review from aomarks as a code owner October 7, 2020 20:55

bicknellr requested a review from sorvell October 7, 2020 20:55

bicknellr mentioned this pull request Oct 8, 2020

[lit-next] List changes for lit-next (newly used DOM APIs, etc.) #382

Closed

Add ParentNode methods to Wrapper.

d7e72c3

bicknellr commented Oct 8, 2020

View reviewed changes

Implement 'convert nodes into a node' to reduce Shady DOM work and Mu…

4c53900

…tationObserver entries when inserting many nodes.

sorvell requested changes Oct 12, 2020

View reviewed changes

bicknellr added 2 commits October 12, 2020 12:15

Fix a bug where append, prepend, replaceChildren was broken whe…

ce8b140

…n passing a single string.

Refactor and fix titles in tests.

210e89b

bicknellr requested a review from sorvell October 12, 2020 19:52

sorvell reviewed Oct 14, 2020

View reviewed changes

Factor out a function to convert a value into a node.

4dd308c

bicknellr requested a review from sorvell October 15, 2020 00:18

Merge remote-tracking branch 'origin/master' into parent-node

e915283

sorvell approved these changes Oct 15, 2020

View reviewed changes

Remove explicit string conversion in convertIntoANode; browsers thr…

664abc2

…ow when trying to append a symbol.

bicknellr commented Oct 15, 2020

View reviewed changes

bicknellr merged commit 1aff3fa into master Oct 15, 2020

bicknellr deleted the parent-node branch October 15, 2020 20:37

dependabot bot mentioned this pull request Mar 12, 2021

Bump @webcomponents/webcomponentsjs from 2.1.3 to 2.5.0 bogdan-sava/egeria-ui#4

Closed

Add polyfills for select ParentNode APIs. #389

Add polyfills for select ParentNode APIs. #389

Conversation

bicknellr commented Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bicknellr commented Oct 8, 2020

bicknellr commented Oct 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bicknellr commented Oct 7, 2020 •

edited

Loading