-
Notifications
You must be signed in to change notification settings - Fork 288
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
Add "precustomized" custom element state #894
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good. And I guess also suggests we need a test for defined.
dom.bs
Outdated
<dfn export id=concept-element-custom for=Element>custom</dfn>. | ||
"<code>undefined</code>", "<code>failed</code>", "<code>uncustomized</code>", | ||
"<code>precustomized</code>", or "<code>custom</code>". An <a for=/>element</a> whose <a | ||
for=Element>custom element state</a> is "<code>uncustomized</code>", "<code>precustomized</code>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong. Why would a custom element be considered as defined before super()
call is made in the constructor? This will be incompatible with the shipping behavior of custom elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I agree. I've removed "precustomized" from the definition of "defined".
@annevk do you still think there is a need for more testing of :defined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a test yes, to ensure that it does not match that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll add a test that :defined
doesn't match, pre-super()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we already have a test for :defined during an upgrade, and I think it's good enough to catch problems here. Note a couple things:
:defined
already doesn't match during an upgrade, even aftersuper()
, because the custom element state is only set to "custom" after the constructor completes.- Testing
:defined
prior tosuper()
shouldn't be typical anyway, since you can't accessthis
prior tosuper()
. You could use an external reference to the element to test, of course, but that doesn't seem like it'd be common.
I'm sure I'm missing something here - please let me know what it is, and what you'd like me to test for. I can add a test for the second bullet above, but it doesn't seem terribly useful. It would catch the case where :defined
matches prior to super()
but stops matching it after super()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this additional testing, in case that's what you're looking for here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think those asserts are helpful.
Ok, I think I addressed everything here. Note that I did add "precustomized" to the definition of "custom element". I'm using that in my other PR. |
Per the conversation here [1], there is a desire to add more testing of the :defined pseudo state, prior to the call to super(). Note that `this` is not accessible prior to super(), so the instance itself is used. Also note that :defined already does not match anywhere inside the constructor, for upgrades. [1] whatwg/dom#894 (comment) Bug: 1042130 Change-Id: I2372900981247ea5624e737d2597d004398de477
Per the conversation here [1], there is a desire to add more testing of the :defined pseudo state, prior to the call to super(). Note that `this` is not accessible prior to super(), so the instance itself is used. Also note that :defined already does not match anywhere inside the constructor, for upgrades. [1] whatwg/dom#894 (comment) Bug: 1042130 Change-Id: I2372900981247ea5624e737d2597d004398de477 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2431558 Auto-Submit: Mason Freed <masonfreed@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#810991}
Per the conversation here [1], there is a desire to add more testing of the :defined pseudo state, prior to the call to super(). Note that `this` is not accessible prior to super(), so the instance itself is used. Also note that :defined already does not match anywhere inside the constructor, for upgrades. [1] whatwg/dom#894 (comment) Bug: 1042130 Change-Id: I2372900981247ea5624e737d2597d004398de477 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2431558 Auto-Submit: Mason Freed <masonfreed@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#810991}
Per the conversation here [1], there is a desire to add more testing of the :defined pseudo state, prior to the call to super(). Note that `this` is not accessible prior to super(), so the instance itself is used. Also note that :defined already does not match anywhere inside the constructor, for upgrades. [1] whatwg/dom#894 (comment) Bug: 1042130 Change-Id: I2372900981247ea5624e737d2597d004398de477 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2431558 Auto-Submit: Mason Freed <masonfreed@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#810991}
… super(), a=testonly Automatic update from web-platform-tests Add testing of :defined prior to call to super() Per the conversation here [1], there is a desire to add more testing of the :defined pseudo state, prior to the call to super(). Note that `this` is not accessible prior to super(), so the instance itself is used. Also note that :defined already does not match anywhere inside the constructor, for upgrades. [1] whatwg/dom#894 (comment) Bug: 1042130 Change-Id: I2372900981247ea5624e737d2597d004398de477 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2431558 Auto-Submit: Mason Freed <masonfreed@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#810991} -- wpt-commits: 0e252e7d3d8f95725f6ae177148da85eac333ca2 wpt-pr: 25794
… super(), a=testonly Automatic update from web-platform-tests Add testing of :defined prior to call to super() Per the conversation here [1], there is a desire to add more testing of the :defined pseudo state, prior to the call to super(). Note that `this` is not accessible prior to super(), so the instance itself is used. Also note that :defined already does not match anywhere inside the constructor, for upgrades. [1] whatwg/dom#894 (comment) Bug: 1042130 Change-Id: I2372900981247ea5624e737d2597d004398de477 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2431558 Auto-Submit: Mason Freed <masonfreed@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#810991} -- wpt-commits: 0e252e7d3d8f95725f6ae177148da85eac333ca2 wpt-pr: 25794
Per the conversation here [1], there is a desire to add more testing of the :defined pseudo state, prior to the call to super(). Note that `this` is not accessible prior to super(), so the instance itself is used. Also note that :defined already does not match anywhere inside the constructor, for upgrades. [1] whatwg/dom#894 (comment) Bug: 1042130 Change-Id: I2372900981247ea5624e737d2597d004398de477 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2431558 Auto-Submit: Mason Freed <masonfreed@chromium.org> Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#810991} GitOrigin-RevId: cae287a3d5a71aa359c7c9d2e238a18f0babf37a
Add the "precustomized" state needed by the HTML PR that prevents usage of
attachInternals()
prior to the constructor being run.💥 Error: 500 Internal Server Error 💥
PR Preview failed to build. (Last tried on Sep 28, 2020, 11:11 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.
🔗 Related URL
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.