Skip to content
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 ChildNode APIs. #390

Merged
merged 20 commits into from
Oct 15, 2020
Merged

Add polyfills for ChildNode APIs. #390

merged 20 commits into from
Oct 15, 2020

Conversation

bicknellr
Copy link
Collaborator

@bicknellr bicknellr commented Oct 7, 2020

Adds polyfills for ChildNode's after, before, remove, and replaceWith functions and wrappers to support these in Shady DOM.

@google-cla google-cla bot added the cla: yes label Oct 7, 2020
Window: [WindowPatches]
Window: [WindowPatches],
CharacterData: [ChildNodePatches],
DocumentType: [ChildNodePatches],
Copy link
Collaborator Author

@bicknellr bicknellr Oct 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it isn't worth trying to make this work on DocumentType. Testing it is pretty awkward and the restrictions on where a DocumentType can go in a document (only first child of a document, maybe?) and what can go around one (almost nothing except comments and a single following <html>?) makes most ChildNode APIs on DocumentType pretty useless.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, let's remove that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, removed.

Comment on lines 49 to 59
replaceWith(...args) {
const parentNode = this[utils.SHADY_PREFIX + 'parentNode'];
if (parentNode === null) {
return;
}
for (const arg of args) {
const newChild = typeof arg === 'string' ? document.createTextNode(arg) : arg;
parentNode[utils.SHADY_PREFIX + 'insertBefore'](newChild, this);
}
parentNode[utils.SHADY_PREFIX + 'removeChild'](this);
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check the order here since this probably feeds into MutationObservers. Likely, this should be remove first, then insert new children.

@bicknellr bicknellr marked this pull request as ready for review October 8, 2020 19:19
@bicknellr bicknellr requested a review from aomarks as a code owner October 8, 2020 19:19
@bicknellr bicknellr requested a review from sorvell October 8, 2020 19:19
Window: [WindowPatches]
Window: [WindowPatches],
CharacterData: [ChildNodePatches],
DocumentType: [ChildNodePatches],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, let's remove that.

DocumentType.prototype,
Element.prototype,
]) {
delete item.after;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have to do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

</head>
<body>
<script>
const testBefore = (createChildNode) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testAfter?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's simpler not to have the suite separated from the tests. You can just make the createChildNode function inside the suite, can't you? (same for other test files).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, these are inlined now.

DocumentType.prototype,
Element.prototype,
]) {
delete item.before;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why is this here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DocumentType.prototype,
Element.prototype,
]) {
delete item.remove;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why is this here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DocumentType.prototype,
Element.prototype,
]) {
delete item.replaceWith;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why is this here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

</head>
<body>
<script>
const testBefore = (createChildNode) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testReplaceWith?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Removed based on another review comment.)

@@ -0,0 +1,56 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why it's important to polyfill these APIs in platform rather than just doing it in shadydom?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bicknellr bicknellr requested a review from sorvell October 12, 2020 20:02
Base automatically changed from parent-node to master October 15, 2020 20:37
@bicknellr bicknellr merged commit 903b81c into master Oct 15, 2020
@bicknellr bicknellr deleted the child-node branch October 15, 2020 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants