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.

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

Jump to bottom

Adoption and DocumentFragment, part two #819

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

Adoption and DocumentFragment, part two #819

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b137ba
Revert "Run adopt as part of insert"
annevk Jan 10, 2020
7afc950
Make adoption account for DocumentFragment nodes with hosts
annevk Oct 18, 2022
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
30 changes: 24 additions & 6 deletions dom.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2509,6 +2509,8 @@ of a <var>node</var> into a <var>parent</var> before a <var>child</var>, run the
<li><p>If <var>referenceChild</var> is <var>node</var>, then set <var>referenceChild</var> to
<var>node</var>'s <a for=tree>next sibling</a>.

<li><p><a>Adopt</a> <var>node</var> into <var>parent</var>'s <a for=Node>node document</a>.

<li><p><a for=/>Insert</a> <var>node</var> into <var>parent</var> before <var>referenceChild</var>.

<li><p>Return <var>node</var>.
Expand Down Expand Up @@ -2574,8 +2576,6 @@ before a <var>child</var>, with an optional <i>suppress observers flag</i>, run
<p>For each <var>node</var> in <var>nodes</var>, in <a>tree order</a>:

<ol>
<li><p><a>Adopt</a> <var>node</var> into <var>parent</var>'s <a for=Node>node document</a>.

<li><p>If <var>child</var> is null, then <a for=set>append</a> <var>node</var> to
<var>parent</var>'s <a for=tree>children</a>.

Expand Down Expand Up @@ -2689,6 +2689,8 @@ within a <var>parent</var>, run these steps:

<li><p>Let <var>previousSibling</var> be <var>child</var>'s <a>previous sibling</a>.

<li><p><a>Adopt</a> <var>node</var> into <var>parent</var>'s <a for=Node>node document</a>.

<li><p>Let <var>removedNodes</var> be the empty set.

<li>
Expand Down Expand Up @@ -2721,6 +2723,9 @@ within a <var>parent</var>, run these steps:
within a <var>parent</var>, run these steps:

<ol>
<li><p>If <var>node</var> is non-null, then <a>adopt</a> <var>node</var> into <var>parent</var>'s
<a for=Node>node document</a>.

<li><p>Let <var>removedNodes</var> be <var>parent</var>'s <a for=tree>children</a>.

<li><p>Let <var>addedNodes</var> be the empty set.
Expand Down Expand Up @@ -5324,8 +5329,8 @@ method steps are:
algorithm is passed <var>node</var> and <var>oldDocument</var>, as indicated in the <a>adopt</a>
algorithm.

<p>To <dfn export id=concept-node-adopt>adopt</dfn> a <var>node</var> into a <var>document</var>, run
these steps:
<p>To <dfn export id=concept-node-adopt>adopt</dfn> a <var>node</var> into a <var>document</var>,
with an optional <var>forceDocumentFragmentAdoption</var> (default false):

<ol>
<li><p>Let <var>oldDocument</var> be <var>node</var>'s <a for=Node>node document</a>.
Expand All @@ -5342,6 +5347,16 @@ these steps:
<a>shadow-including inclusive descendants</a>:

<ol>
<li>
<p>If <var>forceDocumentFragmentAdoption</var> is false, <var>inclusiveDescendant</var> is a
{{DocumentFragment}} <a for=/>node</a>, <var>inclusiveDescendant</var> is <var>node</var>, and
<var>node</var>'s <a for=DocumentFragment>host</a> is non-null, then
<a for=iteration>continue</a>.

<p class=note>This is only reasonable as long as all <a>adopt</a> callers remove the children
of <var>node</var>. HTML's <{template}> element passes true for
<var>forceDocumentFragmentAdoption</var>.

<li><p>Set <var>inclusiveDescendant</var>'s <a for=Node>node document</a> to <var>document</var>.

<li><p>If <var>inclusiveDescendant</var> is an <a for=/>element</a>, then set the
Expand Down Expand Up @@ -5370,8 +5385,11 @@ these steps:
<li><p>If <var>node</var> is a <a for=/>shadow root</a>, then <a>throw</a> a
"{{HierarchyRequestError!!exception}}" {{DOMException}}.

<li><p>If <var>node</var> is a {{DocumentFragment}} <a for=/>node</a> whose
<a for=DocumentFragment>host</a> is non-null, then return.
<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.
Comment on lines +5388 to +5392
Copy link

@bigopon bigopon Oct 25, 2022 •
edited
Loading

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 •
edited
Loading

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


<li><p><a>Adopt</a> <var>node</var> into <a>this</a>.

Expand Down