Allow the creation of null registry element and shadow root [chromium/src : main]
YeongHan Kim (Gerrit)
YeongHan Kim voted and added 1 comment![]( "Open in Gerrit")
Votes added by YeongHan Kim
| Commit-Queue | +1 |
1 comment
Please take a look.
This CL reflects Anne’s latest changes.
Thank you!
Related details
- Jayson Chen
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jayson Chen (Gerrit)
Jayson Chen added 3 comments![]( "Open in Gerrit")
The implementation looks good to me, but it seems like it's causing some WPTs and unit tests failure. Would you be able to take a look at the unit test failures on CQ to see if it's something you're able to fix? Feel free to leave it to me to investigate if you're not sure about it.
Also, it may be a good idea to use virtual test suite for now.
// 3-2. If options[""customElementRegistry"] exists:nit: extra "
This is a testharness.js-based test.These test failures seem a bit odd. Could you help me check if these assert failures are happening on [this check](https://github.com/web-platform-tests/wpt/blob/71663448bfcaabc177533e4d6ce0dfd743aed6bd/custom-elements/registries/scoped-registry-initialize.html#L75)? May be something I need to go investigate.
Related details
- Mason Freed
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Thanks for the review!
I uploaded this CL mainly to confirm the overall direction first.
I’ll take a closer look at the WPT and unit test failures on CQ and investigate what’s going wrong.
Thanks again for the helpful inputs!
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 4 comments![]( "Open in Gerrit")
Sorry for the wait! When you get a chance, could you take another look?
Thanks!
// 3-2. If options[""customElementRegistry"] exists:YeongHan Kimnit: extra "
Done
is = AtomicString(string_or_options->GetAsString());According to the specification, the element creation options parameter can accept either a string or an ElementCreationOptions dictionary, and the string must be ignored.
This behavior is covered by the `Document-createElement-customized-builtins` test.
However, the current implementation incorrectly assigns the is attribute when a string is provided.
The reason the test previously passed is that when flattening the CreateElementOptions, the default value of the registry was null, which caused the `definition` step to be skipped.
Due to the recent spec change, the default registry is now the global custom element registry, which exposed this issue.
Therefore, this patch updates the behavior so that when a string is passed as the element creation options, it is ignored as required by the specification.
These test failures seem a bit odd. Could you help me check if these assert failures are happening on [this check](https://github.com/web-platform-tests/wpt/blob/71663448bfcaabc177533e4d6ce0dfd743aed6bd/custom-elements/registries/scoped-registry-initialize.html#L75)? May be something I need to go investigate.
Previously, if the `registry` is coming as null, `SetCustomElementRegisty()` is skipped in `CreateUncustomizedOrUndefinedElementTemplate`[1]. However, with the spec change, if ElementCreationOptions.registry is set to null, we now need to call SetCustomElementRegistry() with null.
Since `SetCustomElementRegistry` can be invoked in various situations, it should only set the `registry` when ElementCreationOptions.registry is explicitly null. To address this, I introduced an `is_explicit_null` flag.
I’m not entirely sure if this is the best approach, so I’d appreciate any feedback or suggestions for a better solution.
Related details
- Jayson Chen
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jayson Chen (Gerrit)
Jayson Chen added 3 comments![]( "Open in Gerrit")
Thanks for looking into the test failures and finding this issue. Left some thoughts about how we can handle the two null cases (explicit vs implicit) in CreateElement
is = AtomicString(string_or_options->GetAsString());According to the specification, the element creation options parameter can accept either a string or an ElementCreationOptions dictionary, and the string must be ignored.
This behavior is covered by the `Document-createElement-customized-builtins` test.
However, the current implementation incorrectly assigns the is attribute when a string is provided.The reason the test previously passed is that when flattening the CreateElementOptions, the default value of the registry was null, which caused the `definition` step to be skipped.
Due to the recent spec change, the default registry is now the global custom element registry, which exposed this issue.Therefore, this patch updates the behavior so that when a string is passed as the element creation options, it is ignored as required by the specification.
Sounds good, thanks for finding this issue.
This is a testharness.js-based test.YeongHan KimThese test failures seem a bit odd. Could you help me check if these assert failures are happening on [this check](https://github.com/web-platform-tests/wpt/blob/71663448bfcaabc177533e4d6ce0dfd743aed6bd/custom-elements/registries/scoped-registry-initialize.html#L75)? May be something I need to go investigate.
Previously, if the `registry` is coming as null, `SetCustomElementRegisty()` is skipped in `CreateUncustomizedOrUndefinedElementTemplate`[1]. However, with the spec change, if ElementCreationOptions.registry is set to null, we now need to call SetCustomElementRegistry() with null.
Since `SetCustomElementRegistry` can be invoked in various situations, it should only set the `registry` when ElementCreationOptions.registry is explicitly null. To address this, I introduced an `is_explicit_null` flag.
I’m not entirely sure if this is the best approach, so I’d appreciate any feedback or suggestions for a better solution.
Thanks for looking into this issue! Now it seems like the main issue is that CreateElement function needs to know whether the passed in nullptr is an explicit null or implicit null (so default registry). I wonder if this will be a good chance for us to overload CreateElement function (as mentioned in the comment in code), one without registry argument so we provide the default registry in the impl and one with registry argument and we always respect the passed in value. This way we don't need to differentiate two different kinds of nullptr in CreateElement and also don't need to have extra explicit null optional argument that may be confusing down the line. This may require some changes to update CreateElement usages though.
One related question: in flatten element creation option, do we need `is_explicit_null`? Given that we by default use the default registry if not provided by option, is it possible to have a null registry value that should actually be considered as default value? If we do get nullptr from `document->customElementRegistry()` as the default registry value, we'd want to consider that an explicit null registry, right?
Related details
- Mason Freed
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 2 comments![]( "Open in Gerrit")
Thanks for the review!
This is a testharness.js-based test.YeongHan KimThese test failures seem a bit odd. Could you help me check if these assert failures are happening on [this check](https://github.com/web-platform-tests/wpt/blob/71663448bfcaabc177533e4d6ce0dfd743aed6bd/custom-elements/registries/scoped-registry-initialize.html#L75)? May be something I need to go investigate.
Jayson ChenPreviously, if the `registry` is coming as null, `SetCustomElementRegisty()` is skipped in `CreateUncustomizedOrUndefinedElementTemplate`[1]. However, with the spec change, if ElementCreationOptions.registry is set to null, we now need to call SetCustomElementRegistry() with null.
Since `SetCustomElementRegistry` can be invoked in various situations, it should only set the `registry` when ElementCreationOptions.registry is explicitly null. To address this, I introduced an `is_explicit_null` flag.
I’m not entirely sure if this is the best approach, so I’d appreciate any feedback or suggestions for a better solution.
Thanks for looking into this issue! Now it seems like the main issue is that CreateElement function needs to know whether the passed in nullptr is an explicit null or implicit null (so default registry). I wonder if this will be a good chance for us to overload CreateElement function (as mentioned in the comment in code), one without registry argument so we provide the default registry in the impl and one with registry argument and we always respect the passed in value. This way we don't need to differentiate two different kinds of nullptr in CreateElement and also don't need to have extra explicit null optional argument that may be confusing down the line. This may require some changes to update CreateElement usages though.
One related question: in flatten element creation option, do we need `is_explicit_null`? Given that we by default use the default registry if not provided by option, is it possible to have a null registry value that should actually be considered as default value? If we do get nullptr from `document->customElementRegistry()` as the default registry value, we'd want to consider that an explicit null registry, right?
Thanks for the guidance. I think overloading the CreateElement without a registry parameter is a great approach. Really appreciate the idea!
Regarding your question: I believe the spec intends for an explicit null registry only when ElementCreationOptions.registry is explicitly set to null. So the nullptr returned from `document->customElementRegistry()` as the default registry should not count as an explicit null. (And this distinction is necessary for the tests to pass.)
Right now, `is_explicit_null` is used during flattening so that CreateElement can determine whether the null was explicitly provided. But if we move forward with overloading CreateElement to have a version without the registry argument, then I think we can remove is_explicit_null entirely from the flattening step.
I’ll update the patch soon and ask for another review. Thanks so much for your help!
Related details
- Jayson Chen
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Sorry for the delay.
Please review it when you get a chance. Thanks!
Related details
- Jayson Chen
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |