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

Adoption and DocumentFragment, part two #819

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 10, 2020

This reverts commit c825cea and adds a new solution for DocumentFragment adoption on top. See #813 and #814 for context.

HTML: whatwg/html#5413

Tests: web-platform-tests/wpt#22504

jsdom: jsdom/jsdom#2925


Preview | Diff

@annevk annevk force-pushed the annevk/revert-abort-changes branch from 32cfc18 to 6eb77d0 Compare March 27, 2020 19:01
annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 27, 2020
annevk added a commit to whatwg/html that referenced this pull request Mar 28, 2020
@annevk annevk changed the title Revert "Run adopt as part of insert" Adoption and DocumentFragment, part two Mar 30, 2020
@domenic
Copy link
Member

domenic commented Apr 27, 2020

Anything I can help with on the jsdom side, or on reviews, to land this and the associated HTML/WPT PRs?

@annevk annevk force-pushed the annevk/revert-abort-changes branch from 8dafaf7 to 9a1e814 Compare November 6, 2020 12:39
@annevk
Copy link
Member Author

annevk commented Nov 6, 2020

Verifying that this is correct would help as well as a review. Perhaps @mfreed7 is interested in prototyping in Chromium?

@mfreed7
Copy link
Contributor

mfreed7 commented Nov 6, 2020

Verifying that this is correct would help as well as a review. Perhaps @mfreed7 is interested in prototyping in Chromium?

The change looks reasonable, but from #814 it might be tricky to get exactly right. I'm happy to prototype in Chromium, but I'd prefer to do that after jsdom has had a chance to confirm the change passes existing WPTs... @TimothyGu?

@annevk annevk force-pushed the annevk/revert-abort-changes branch from 9a1e814 to 5c12594 Compare December 1, 2020 09:40
Base automatically changed from master to main January 15, 2021 07:32
@annevk annevk force-pushed the annevk/revert-abort-changes branch from 5c12594 to 77ad4ca Compare January 19, 2021 11:18
bwrrp added a commit to bwrrp/slimdom.js that referenced this pull request Feb 14, 2021
The DOM spec is currently broken because of changes around the adoption of
nodes. There is an open PR which reverts some of these changes. This
implements the same changes as in that PR to fix issues where some mutations
would otherwise generate invalid mutation records.
bwrrp added a commit to bwrrp/slimdom.js that referenced this pull request Dec 20, 2021
The DOM spec is currently broken because of changes around the adoption of
nodes. There is an open PR which reverts some of these changes. This
implements the same changes as in that PR to fix issues where some mutations
would otherwise generate invalid mutation records.
dom.bs Outdated Show resolved Hide resolved
Copy link

@meshl555 meshl555 left a comment

Choose a reason for hiding this comment

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

Noneo8

@meshl555

This comment was marked as spam.

@annevk annevk force-pushed the annevk/revert-abort-changes branch from 77ad4ca to 7afc950 Compare October 18, 2022 09:18
@annevk annevk requested review from rniwa and mfreed7 October 18, 2022 09:19
annevk added a commit to whatwg/html that referenced this pull request Oct 18, 2022
@rniwa
Copy link
Collaborator

rniwa commented Oct 19, 2022

FWIW, I've implemented this new behavior in https://commits.webkit.org/252098@main

Comment on lines +5388 to +5392
<li>
<p>If <var>node</var> is a {{DocumentFragment}} <a for=/>node</a> and its
<a for=DocumentFragment>host</a> is non-null, then return <var>node</var>.

<p class=note>Unfortunately this does not throw for web compatibility.
Copy link

@bigopon bigopon Oct 25, 2022

Choose a reason for hiding this comment

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

What does "does not throw for web compatibility" mean? Any one who employs template.content node adoption before will now face with breakage with the sudden change of "whose host is non-null, then return".

If we no longer guarantee old API evolves without breaking, how do I find some rule of thumbs what is dangerous and what is not? An example issue is here aurelia/framework#1003

Copy link
Member Author

Choose a reason for hiding this comment

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

@bigopon before this change adoptNode() would have returned undefined which would result in undefined behavior as its IDL claims its return value is a Node.

From reading the issue you linked it seems that implementations never matched the specification and some applications (such as yours) relied on that. Perhaps we need to continue to allow that, but it would be a strange special case for HTMLTemplateElement's DocumentFragment usage as we cannot do the same thing if you were to pass a ShadowRoot for instance. It would also result in a rather weird HTMLTemplateElement instance, but since nothing much builds upon it maybe that is okay.

Copy link

@jded76 jded76 Oct 27, 2022

Choose a reason for hiding this comment

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

From reading the issue you linked it seems that implementations never matched the specification and some applications (such as yours) relied on that.

That's not true. The implementations were according to the spec until #754, that inserted a rule about template's fragment for the first time.
That rule returned undefined, there were some issues with that and the browsers didn't implemented (according to this ).
Then with #819 the spec changed again.
On July webkit implemented the new rule. Before one month Safari went public with the new version of webkit and many applications broke on production.
And since then we are trying to figure out what's going on.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it sounds from the linked issues and comment like this change isn't web compatible as-is, right? Sounds like another approach is needed.

Copy link

Choose a reason for hiding this comment

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

Any updates on this?
Right now our application is not working on Safari on production.
Webkit says that this is intentional behavior change and we don't know how to proceed to fix our application...

@jded76
Copy link

jded76 commented Nov 5, 2022

Sorry for coming back on this again and again, but we have a problem in production with Safari and it seems that we got stuck...
Just ignoring it doesn't solve anything. I think a decision has to be made here soon...

@bigopon
Copy link

bigopon commented Jan 30, 2023

Is there a process that I need to follow or do I just create a PR to fix this? The easiest option is to revert the change and find a more reasonable one later.

@bigopon
Copy link

bigopon commented Feb 15, 2023

It's been awhile without answer, where can I shoot my concerns over this issues? It's breaking both apps and hearts.

@mfreed7
Copy link
Contributor

mfreed7 commented Feb 22, 2023

It's been awhile without answer, where can I shoot my concerns over this issues? It's breaking both apps and hearts.

I believe you're referring to the WebKit/Safari behavior described here, correct? https://bugs.webkit.org/show_bug.cgi?id=246899

If so, that bug is the better place to raise the issue, since this issue is about the spec. That bug is informative about the direction the spec needs to take, though, since it seems the proposed direction isn't web compatible.

@karlcow
Copy link
Member

karlcow commented Mar 10, 2023

so adoptNode behavior has been reverted in WebKit
WebKit/WebKit#11347
because of aurelia/framework#1003
WebKit being the only to have adopted the new behavior, it was having the burden of the breakage.

There is a need for a better path forward.

@bigopon
Copy link

bigopon commented Mar 10, 2023

There is a need for a better path forward.

What we want is the ability to adopt the document fragment. Currently adopting the template element doesn't adopt the document fragment itself. What if we do that?

I dont know all the reasons why the fragment of the template element isn't associated with the document that created it, but if calling adoptNode explicitly on template does it then maybe this behavior is safe to go in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants