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

State-preserving atomic move integration #10657

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

State-preserving atomic move integration #10657

wants to merge 15 commits into from
+119 −23

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Sep 29, 2024 •
edited by pr-preview bot
Loading

This PR is a supplementary PR to whatwg/dom#1307. It provides the HTML-overrides for the DOM Standard's "moving steps" extension, to allow pieces of the HTML Standard to react to atomic DOM moves without the use of the "insertion steps" or "removing steps", which by default, intentionally fail to preserve most state. The changes in this PR are inspired by the audit carried out in https://docs.google.com/document/d/1qfYyvdK4zhzloABKeh0K1lHPm-SpnEcsWEE9UdDuoMk/edit.

(See WHATWG Working Mode: Changes for more details.)

💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Nov 4, 2024, 1:08 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

      <!DOCTYPE html>
      <html>
      <head>
          <meta name="viewport" content="width=device-width, initial-scale=1">
          <meta name="robots" content="noindex">
          <style>body,html{height:100%;margin:0}body{display:flex;align-items:center;justify-content:center;flex-direction:column;-webkit-font-smoothing:antialiased;text-rendering:optimizeLegibility}p{text-align:center;font-family:-apple-system,BlinkMacSystemFont,Segoe UI,Roboto,Oxygen,Ubuntu,Cantarell,Fira Sans,Droid Sans,Helvetica Neue,sans-serif;color:#000;font-size:14px;margin-top:-50px}p.code{font-size:24px;font-weight:500;border-bottom:1px solid #e0e1e2;padding:0 20px 15px}p.text{margin:0}a,a:visited{color:#aaa}</style>
      </head>
      <body>
      <p class="code">
        Error
      </p>
      <p class="text">
        We encountered an error when trying to load your application and your page could not be served. Check the logs for your application in the App Platform dashboard.
      </p>
        <div style="display:none;">
          <h1>
    upstream_reset_before_response_started{connection_termination} (503 UC)      </h1>
          <p data-translate="connection_timed_out">App Platform failed to forward this request to the application.</p>
      </div>
      </body>
      </html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@domfarolino domfarolino mentioned this pull request Oct 10, 2024
Open
19 tasks
smaug----
smaug---- reviewed Nov 8, 2024
View reviewed changes
source

<ol>
<li><p>If the <span>form-associated element</span>'s <span>parser inserted flag</span> is set,
then return.</p></li>
Copy link

Choose a reason for hiding this comment

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

I assume this special case is here because something similar happens currently when removing and adding such element back?

Copy link
Member Author

@domfarolino domfarolino Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See the first row in https://docs.google.com/document/d/1qfYyvdK4zhzloABKeh0K1lHPm-SpnEcsWEE9UdDuoMk/edit?tab=t.0 for this in general.

However regarding this condition specifically, if I remember correctly it was just copied from the insertion steps to preserve the behavior of remove+insert during a move. Looking at it again, I'm unsure how this is supposed to work though.

Imagine a form-associated element with a form owner being removed from the DOM. There are two cases:

  1. During removal, that element gets its form owner reset because it hits the conditions in the "HTML element removing steps". This unsets the parser inserted bit. Next time that element gets inserted into the DOM, we'll reset the form owner again, always to the nearest form element.
  2. During removal, that element does not get its form owner reset. The parser-inserted bit remains set even though the element is disconnected. When it's inserted next, we trip the insertion condition "If the parser inserted flag is set", and we do not reset the form owner, ever.

It isn't clear to me when (2) above can ever happen. That is, when during removal will we not "reset the form owner" of a form-associated element that has a non-null form owner? The condition reads:

If the form-associated element has a form owner and the form-associated element and its form owner are no longer in the same tree, then reset the form owner of the form-associated element.

So it seems that (during removal) we'd avoid resetting the owner if the element is still in the same tree as its form owner. But removing makes an element disconnected, so I don't think it could ever be in the same tree as anything. I'm left to think that:

  1. There's a bug in the current HTML element removing steps
  2. Removed form-associated elements with non-null form owners are always reset (during removal)
  3. We don't need the parser inserted condition in the HTML element moving steps

If I am wrong about (1)/(2) in the immediately-above list, then we may still need the parser inserted condition in the moving steps.

I don't know much about forms and their associated mechanics but I feel like @zcorpan does and would maybe be willing to take a look here.

smaug----
smaug---- reviewed Nov 8, 2024
View reviewed changes
source
@@ -72810,7 +72874,10 @@ customElements.define("x-foo", class extends HTMLElement {
<li><p>When it <span>becomes disconnected</span>, its <code data-x="">disconnectedCallback</code>
is called, with no arguments.</p></li>

<li><p>When it is <span data-x="concept-node-adopt">adopted</span> into a new document, its <code
<li><p>When it is <span data-x="concept-node-move-ext">moved</span>, its <code
data-x="">connectedMoveCallback</code> is called, with no arguments.</p></li>
Copy link

Choose a reason for hiding this comment

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

Shouldn't callback get the old subtree owner as an argument, so that one can know easily where one was.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would require holding a reference to the old tree which might be gone by the time the CE callbacks are fired. Perhaps we can consider this as a future enhancement? I think we should be able to do that in a compatible way.

@domfarolino domfarolino marked this pull request as ready for review November 20, 2024 21:52
@domfarolino domfarolino added impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation addition/proposal New features or enhancements labels Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smaug---- smaug---- smaug---- left review comments

@noamr noamr noamr left review comments

@annevk annevk Awaiting requested review from annevk

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@domfarolino @noamr @smaug----