[CSS Modules] Implement cloning behavior for shadowrootserializable [chromium/src : main]
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
// Cross-document importNode: the source host lives in an iframe that
// declares its own import map. importNode must resolve specifiers
// against the destination document's import map (not the source's),
// because ResolveAdoptedStyleSheets uses the cloned root's document.This seems kind of dangerous, given that we're re-resolving specifiers without the same import map - should we make sure the documents match (and thus import maps match) before calling `ProcessAdoptedStylesheetAttribute` on the clone? Or is this fine?
Related details
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. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 2 comments![]( "Open in Gerrit")
// NEW: Re-resolve the shadowrootadoptedstylesheets attribute against theMaybe just "Resolve"?
// Cross-document importNode: the source host lives in an iframe that
// declares its own import map. importNode must resolve specifiers
// against the destination document's import map (not the source's),
// because ResolveAdoptedStyleSheets uses the cloned root's document.This seems kind of dangerous, given that we're re-resolving specifiers without the same import map - should we make sure the documents match (and thus import maps match) before calling `ProcessAdoptedStylesheetAttribute` on the clone? Or is this fine?
After more thought, this is not that different than any other import (although it is less explicit).
Related details
- Dan Clark
- 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. |
Mason Freed (Gerrit)
Mason Freed voted and added 4 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
4 comments
This LGTM with just some small nits and comments.
// NEW: Re-resolve the shadowrootadoptedstylesheets attribute against theAlso, remove `NEW:`. Almost as obvious a tell as the em-dash. 😊
cloned_shadow_root.GetDocument().GetExecutionContext())) {Perhaps just `factory`?
// "iframe-only" is not a valid specifier in the parent doc's import
// map, so resolution silently skips it (per ResolveAdoptedStyleSheets:
// invalid specifiers are dropped). The clone's adoptedStyleSheets
// should be empty.This behavior is interesting. I trust that you've thought about it and it's correct. Just might be a bit surprising to developers?
Related details
- Dan Clark
- Kurt Catti-Schmidt
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. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 5 comments![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Commit-Queue | +1 |
5 comments
// NEW: Re-resolve the shadowrootadoptedstylesheets attribute against theAlso, remove `NEW:`. Almost as obvious a tell as the em-dash. 😊
I've seen that as well. Funnily enough, for this instance, I added it myself 😄
// NEW: Re-resolve the shadowrootadoptedstylesheets attribute against theKurt Catti-SchmidtMaybe just "Resolve"?
Done
cloned_shadow_root.GetDocument().GetExecutionContext())) {Kurt Catti-SchmidtPerhaps just `factory`?
Done
// Cross-document importNode: the source host lives in an iframe that
// declares its own import map. importNode must resolve specifiers
// against the destination document's import map (not the source's),
// because ResolveAdoptedStyleSheets uses the cloned root's document.Kurt Catti-SchmidtThis seems kind of dangerous, given that we're re-resolving specifiers without the same import map - should we make sure the documents match (and thus import maps match) before calling `ProcessAdoptedStylesheetAttribute` on the clone? Or is this fine?
After more thought, this is not that different than any other import (although it is less explicit).
Done
// "iframe-only" is not a valid specifier in the parent doc's import
// map, so resolution silently skips it (per ResolveAdoptedStyleSheets:
// invalid specifiers are dropped). The clone's adoptedStyleSheets
// should be empty.This behavior is interesting. I trust that you've thought about it and it's correct. Just might be a bit surprising to developers?
Yeah, it's not great, but it's the best we can do. I thought about it some more, and it's not that different than other module imports - you need to be aware of the current document's import map no matter what.
Related details
- Dan Clark
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/59668.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Dan Clark
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |