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

Why does upgrade-an-element queue attribute, connected reactions before exiting early? #523

Closed
dominiccooney opened this issue Jun 16, 2016 · 2 comments · Fixed by whatwg/html#1450

Comments

@dominiccooney
Copy link
Contributor

Upgrade an element steps 1-2 queue reactions before checking whether the element is custom. AFAIK the step 3 "already custom" check is designed to prevent double upgrade in the case and element is reinserted while upgrade is pending.

Does this mean that there's no "double upgrade" but there is "double attribute changed dispatch" and "double connected?"

For what it is worth, "classic" custom elements fixed this using the queues themselves. It basically made a distinction between the state where upgrade has been queued, but not started; and upgrade had started. Blink implemented this by actually looking at a combination of the element state and whether the element queue's first reaction was upgrade, IIRC, and that seemed to work well.

/cc @domenic

@domenic
Copy link
Collaborator

domenic commented Jun 16, 2016

My initial impression is that this is accidental. I'll try to take a look at the best solution, whether it be just rearranging the steps or implementing something more complex like the solution you describe. It might take a day or two due to BlinkOn though.

@kojiishi
Copy link

FYI, double-queueing upgrade reactions could also happen in fragment parser. Create an element enqueues upgrade reactions for fragment parser where sync flag is unset. Then inserting the node tries to upgrade before the upgrade is invoked, which enqueues again.

We found innerHTML setter test case hits this.

domenic added a commit to whatwg/html that referenced this issue Jun 21, 2016
As already discussed in the spec (as of #1365), it is sometimes possible
to enqueue two upgrade reactions, via a constructor that does things you
are not supposed to do. The fix in #1365 prevented upgrading twice.
However, it did not bail out early enough, and allowed
attributeChangedCallback and connectedCallback reactions to be enqueued
(redundantly) before the bail-out. This commit moves the bail-out logic
to the beginning of the algorithm, so as to avoid any such
double-enqueuing.

Fixes WICG/webcomponents#523.
domenic added a commit to whatwg/html that referenced this issue Jun 23, 2016
As already discussed in the spec (as of #1365), it is sometimes possible
to enqueue two upgrade reactions, via a constructor that does things you
are not supposed to do. The fix in #1365 prevented upgrading twice.
However, it did not bail out early enough, and allowed
attributeChangedCallback and connectedCallback reactions to be enqueued
(redundantly) before the bail-out. This commit moves the bail-out logic
to the beginning of the algorithm, so as to avoid any such
double-enqueuing.

Fixes WICG/webcomponents#523.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants