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

Take care of missing custom element reactions #1412

Merged
merged 1 commit into from Jun 29, 2016

Conversation

4 participants
@domenic
Member

domenic commented Jun 9, 2016

As discussed in w3c/webcomponents#186, there
are certain scenarios where custom element reactions could be enqueued,
but the custom element reactions stack is empty. The most prominent of
these is editing operations.

We solve this issue in two parts:

  • First, introduce the backup element queue (similar to the old "base
    element queue" concept; see
    w3c/webcomponents#457). This takes care of
    cases like editing which cannot be solved in any other way.
  • Also, we mandate that [CEReactions] be applied to nonstandard APIs
    that modify the DOM, including a list of what we've identified so far.

Closes w3c/webcomponents#186.


Careful review appreciated. /cc @dominiccooney @kojiishi @rniwa @annevk

Take care of missing custom element reactions
As discussed in w3c/webcomponents#186, there
are certain scenarios where custom element reactions could be enqueued,
but the custom element reactions stack is empty. The most prominent of
these is editing operations.

We solve this issue in two parts:

- First, introduce the backup element queue (similar to the old "base
element queue" concept; see
w3c/webcomponents#457). This takes care of
cases like editing which cannot be solved in any other way.
- Also, we mandate that [CEReactions] be applied to nonstandard APIs
that modify the DOM, including a list of what we've identified so far.

Closes w3c/webcomponents#186.
@kojiishi

This comment has been minimized.

kojiishi commented Jun 16, 2016

Any nonstandard APIs introduced by the user agent...must also be decorated with the [CEReactions] attribute.

doesn't sound like it covers editing, CSS OM, nor CSS Animations, but you mentioned editing in the original comment, so the intention is to wrap any such internal operations with something conceptually equal to the [CEReactions] attribute?

@domenic

This comment has been minimized.

Member

domenic commented Jun 16, 2016

doesn't sound like it covers editing, CSS OM, nor CSS Animations, but you mentioned editing in the original comment, so the intention is to wrap any such internal operations with something conceptually equal to the [CEReactions] attribute?

The intention is that IDL-based APIs are covered by [CEReactions], and things that are not IDL-based APIs, like editing, are not---they fall back to the backup element queue. It sounds like we should send a pull request to CSSOM to add [CEReactions](unless the previous consensus is overturned in w3c/webcomponents#521).

I am not familiar with the issues involved in web animations, but my guess is that they involve changing the style attribute from event loop animation frames, not from IDL-based APIs, so they would use the backup element queue.

There is no intention to wrap things that are not IDL-based APIs in some [CEReactions] equivalent. But every IDL-based API that could cause custom element reactions should get annotated with [CEReactions]---including nonstandard ones, like those listed in this patch, or ones that have been forgotten, like CSSOM.

@kojiishi

This comment has been minimized.

kojiishi commented Jun 16, 2016

What about editing? Pressing ENTER, DEL, or Ctrl-V can clone, remove, or append elements without going through execCommand.

@domenic

This comment has been minimized.

Member

domenic commented Jun 16, 2016

Hmm, I thought I was pretty clear. The entire purpose of the backup element queue is to take care of those cases.

@kojiishi

This comment has been minimized.

kojiishi commented Jun 16, 2016

Ah, sorry, I didn't read that part carefully enough. Microtask invokes the backup queue, understood. Sorry for the trouble.

@zcorpan

This comment has been minimized.

Member

zcorpan commented Jun 18, 2016

Filed w3c/csswg-drafts#200 for CSSOM.

data-x="concept-element-custom">custom</span> yet, since this queue is used for <span
data-x="custom-element-upgrades">upgrades</span> as well.)</p>
<p>Each <span>custom element reactions stack</span> has an associated <dfn>backup element
queue</dfn>, which an initially-empty <span>element queue</span>. Elements are pushed onto the

This comment has been minimized.

@annevk

annevk Jun 20, 2016

Member

I find it a little weird that a stack has an associated queue. Aren't they both bound to the unit of ...?

This comment has been minimized.

@domenic

domenic Jun 20, 2016

Member

Yes, that's an alternate way to phrase it. Keeping things together seemed clearer to me...

This comment has been minimized.

@annevk

annevk Jun 21, 2016

Member

It's just that this kind of thing will go away if we formalize more. E.g., if a stack becomes a formalized List or Set, we wouldn't really associate other things with it. We might create a Record that holds it and some other related information I suppose... Oh well. Up to you.

This comment has been minimized.

@domenic

domenic Jun 21, 2016

Member

Hmm I see what you mean. I'll give it a shot rewriting them as separate things and see how it feels...

editing operation which modifies the descendants or attributes of an <span>editable</span>
element. To prevent reentrancy when processing the <span>backup element queue</span>, each
<span>custom element reactions stack</span> also has a <dfn>processing the backup element
queue</dfn> flag, initially unset.</p>

This comment has been minimized.

@annevk

annevk Jun 20, 2016

Member

Same here.

@annevk

This comment has been minimized.

Member

annevk commented Jun 20, 2016

LGTM editorially otherwise.

@kojiishi

This comment has been minimized.

kojiishi commented Jun 24, 2016

LGTM, WIP patch in Blink for this change looks working good when Enter key cloned an element in contenteditable.

@domenic

This comment has been minimized.

Member

domenic commented Jun 24, 2016

Based on seeing how Blink implements this and on trying to separate out the stack from from its associated helpers (backup queue and flag) and the spec getting uglier, I think I prefer to stick with the current approach instead of separating them. However, the WIP Blink patch does not match the spec very much as far as I can tell (I commented over there), so maybe we should not merge this until I understand the reason for the mismatch.

@zcorpan

This comment has been minimized.

Member

zcorpan commented Jun 28, 2016

(CSSOM fixed in w3c/csswg-drafts@39be12a)

dstockwell pushed a commit to dstockwell/chromium that referenced this pull request Jun 29, 2016

Add backup element queue to CustomElementReactionStack
This patch adds a backup element queue[1], which is to be added to the
spec in PR[2].

The attached test not only fail but also crashes without this change.

[1] https://html.spec.whatwg.org/multipage/scripting.html#backup-element-queue
[2] whatwg/html#1412

BUG=594918

Review-Url: https://codereview.chromium.org/2097463002
Cr-Commit-Position: refs/heads/master@{#402776}
@domenic

This comment has been minimized.

Member

domenic commented Jun 29, 2016

Some discussion with @kojiishi and updates to the Blink patch later, it seems like this is good as-is \o/

@domenic domenic merged commit a57c887 into master Jun 29, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@domenic domenic deleted the more-ce-reactions branch Jun 29, 2016

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