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

Always enqueue an element regardless of callback #4127

Closed
wants to merge 3 commits into from
Closed

Always enqueue an element regardless of callback #4127

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34f9019
Always enqueue an element regardless of callback
annevk Oct 26, 2018
2b3d3bb
always enqueue
annevk Oct 30, 2018
c165c78
feedback from Tomek and Domenic
annevk Nov 1, 2018
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
57 changes: 46 additions & 11 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67033,28 +67033,62 @@ customElements.define("x-foo", class extends HTMLElement {
data-x="concept-custom-element-definition-lifecycle-callbacks">lifecycle callbacks</span> with
key <var>callbackName</var>.</p></li>

<li><p>If <var>callback</var> is null, then return</p></li>

<li>
<p>If <var>callbackName</var> is "<code data-x="">attributeChangedCallback</code>", then:</p>
<p>If <var>callback</var> is non-null, then:</p>

<ol>
<li><p>Let <var>attributeName</var> be the first element of <var>args</var>.</p></li>
<li><p>Let <var>skip</var> be false.</p></li>

<li>
<p>If <var>callbackName</var> is "<code data-x="">attributeChangedCallback</code>", then:</p>

<li><p>If <var>definition</var>'s <span
data-x="concept-custom-element-definition-observed-attributes">observed attributes</span> does
not contain <var>attributeName</var>, then return.</p></li>
<ol>
<li><p>Let <var>attributeName</var> be the first element of <var>args</var>.</p></li>

<li><p>If <var>definition</var>'s <span
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this correct? This is still an early return... An alternative would be to set a flag here to only skip the next step, which is a lil ugly, but okay.

Copy link

Choose a reason for hiding this comment

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

WebKit's implementation always enqueues the element regardless of which attribute is observed. But perhaps that's not as bad as connected/disconnected case since the observedAttributes is the only thing that controls whether an attribute change is observed or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah okay, so this should not be observable?

Copy link

@rniwa rniwa Oct 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It IS observable. When mutating an attribute that's not being observed would invoke custom elements reactions at that point if the mutation was done inside a connected callback of another element, for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, sigh. Okay, I think in that case we should always enqueue, and add tests for both scenarios.

Copy link

@rniwa rniwa Nov 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So this would incur quite a bit of performance cost because then every DOM attribute mutation even ones that are not observed would result in the element to be inserted into a custom element reaction queue. I don't think we want to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I was thinking that too. However, it's hard to square that with sometimes making the order of callbacks predictable and sometimes not. Shouldn't we then maintain the status quo of the standard?

Copy link

Choose a reason for hiding this comment

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

I'm fine with that as long as other browser vendors are okay with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this change.
I think the performance penalty by this isn't large.

data-x="concept-custom-element-definition-observed-attributes">observed attributes</span>
does not contain <var>attributeName</var>, then set <var>skip</var> to true.</p></li>
</ol>
</li>

<li><p>If <var>skip</var> is false, then add a new <span>callback reaction</span> to
<var>element</var>'s <span>custom element reaction queue</span>, with callback function
<var>callback</var> and arguments <var>args</var>.</p></li>
</ol>
</li>

<li><p>Add a new <span>callback reaction</span> to <var>element</var>'s <span>custom element
reaction queue</span>, with callback function <var>callback</var> and arguments
<var>args</var>.</p></li>

<li><p><span>Enqueue an element on the appropriate element queue</span> given
<var>element</var>.</p></li>
</ol>

<div class="example">
<p>An element's <code data-x="">connectedCallback</code> can be queued before the element is
domenic marked this conversation as resolved.
Show resolved Hide resolved
disconnected, but as the callback queue is still processed, it results in a <code
domenic marked this conversation as resolved.
Show resolved Hide resolved
data-x="">connectedCallback</code> for an element that is no longer connected:</p>

<pre><code class="js" data-x="">class CParent extends HTMLElement {
connectedCallback() {
this.firstChild.remove();
domenic marked this conversation as resolved.
Show resolved Hide resolved
}
}
customElements.define("c-parent", CParent);

class CChild extends HTMLElement {
connectedCallback() {
console.log("CChild connectedCallback: isConnected =", this.isConnected);
domenic marked this conversation as resolved.
Show resolved Hide resolved
}
}
customElements.define("c-child", CChild);

const container = document.createElement("div");
container.innerHTML = "&lt;c-parent>&lt;c-child>&lt;/c-child>&lt;/c-parent>";
customElements.upgrade(container);
document.body.appendChild(container);

domenic marked this conversation as resolved.
Show resolved Hide resolved
// Logs:
// CChild connectedCallback: isConnected = false</code></pre>
</div>

<p>To <dfn data-export="">enqueue a custom element upgrade reaction</dfn>, given an element
<var>element</var> and <span>custom element definition</span> <var>definition</var>, run the
following steps:</p>
Expand Down Expand Up @@ -122324,6 +122358,7 @@ INSERT INTERFACES HERE
Tom Pike,
Tom Schuster,
Tomasz Jakut, <!-- Comandeer on GitHub -->
Tomek Wytrębowicz,
Tommy Thorsen,
Tony Ross,
Tooru Fujisawa,
Expand Down