-
Notifications
You must be signed in to change notification settings - Fork 13
fix(core): chrome scoped registry and popover behavior #117
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,10 +1,50 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||
| // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||||||||||||||||||||||||||||||||||||||||||||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| import type { RenderOptions } from 'lit'; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import { GlobalStateService } from '../services/global.service.js'; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import type { ElementDefinition, LegacyDecoratorTarget } from '../types/index.js'; | ||||||||||||||||||||||||||||||||||||||||||||||||
| import { defineElement, supportsScopedRegistry } from '../utils/dom.js'; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| interface ScopedRegistryHost extends HTMLElement { | ||||||||||||||||||||||||||||||||||||||||||||||||
| createRenderRoot?: () => HTMLElement | DocumentFragment; | ||||||||||||||||||||||||||||||||||||||||||||||||
| renderOptions?: RenderOptions; | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| const litCreationScopeElements = new WeakSet<ElementDefinition>(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| /** Lit passes a legacy `deep` boolean, but scoped registries require `ImportNodeOptions`. https://html.spec.whatwg.org/multipage/custom-elements.html#scoped-custom-element-registries */ | ||||||||||||||||||||||||||||||||||||||||||||||||
| function createScopedCreationScope(ownerDocument: Document, customElementRegistry: CustomElementRegistry) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||
| importNode: (node: Node, deep = false) => | ||||||||||||||||||||||||||||||||||||||||||||||||
| ownerDocument.importNode(node, { | ||||||||||||||||||||||||||||||||||||||||||||||||
| customElementRegistry, | ||||||||||||||||||||||||||||||||||||||||||||||||
| selfOnly: !deep | ||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||
| } satisfies NonNullable<RenderOptions['creationScope']>; | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| function attachLitCreationScope(element: ElementDefinition, customElementRegistry: CustomElementRegistry) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| if (litCreationScopeElements.has(element)) return; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| const host = element.prototype as ScopedRegistryHost; | ||||||||||||||||||||||||||||||||||||||||||||||||
| const createRenderRoot = host.createRenderRoot; | ||||||||||||||||||||||||||||||||||||||||||||||||
| if (!createRenderRoot) return; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| litCreationScopeElements.add(element); | ||||||||||||||||||||||||||||||||||||||||||||||||
| Object.defineProperty(host, 'createRenderRoot', { | ||||||||||||||||||||||||||||||||||||||||||||||||
| configurable: true, | ||||||||||||||||||||||||||||||||||||||||||||||||
| value(this: ScopedRegistryHost) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| const renderRoot = createRenderRoot.call(this); | ||||||||||||||||||||||||||||||||||||||||||||||||
| if (renderRoot instanceof ShadowRoot) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| this.renderOptions ??= {}; | ||||||||||||||||||||||||||||||||||||||||||||||||
| this.renderOptions.creationScope = createScopedCreationScope(renderRoot.ownerDocument, customElementRegistry); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| return renderRoot; | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+35
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify whether any code reassigns createRenderRoot (would be impacted by writable:false).
rg -nP --type=ts -C3 '\bcreateRenderRoot\s*='
rg -nP --type=ts -C3 'Object\.defineProperty\([^)]*createRenderRoot'Repository: NVIDIA/elements Length of output: 1377 🏁 Script executed: #!/bin/bash
set -euo pipefail
# 1) Check for any direct reassignments to createRenderRoot (should catch strict-mode assignment hazards)
rg -n --type=ts -S '\.createRenderRoot\s*=' .
rg -n --type=tsx -S '\.createRenderRoot\s*=' .
rg -n --type=ts -S '\bcreateRenderRoot\s*=' projects/core projects/site projects/starters 2>/dev/null || true
# 2) Check for defineProperty redefinitions targeting the 'createRenderRoot' key
rg -n --type=ts -S "Object\.defineProperty\([^)]*'createRenderRoot'" .
rg -n --type=ts -S "Object\.defineProperty\([^)]*\"createRenderRoot\"" .
rg -n --type=ts -S "'createRenderRoot'\s*," projects/core projects/site projects/starters 2>/dev/null || true
# 3) Sanity: list all other occurrences to see if any mixins/decorators reference this symbol
rg -n --type=ts -S '\bcreateRenderRoot\b' projects/core/src/internal/decoratorsRepository: NVIDIA/elements Length of output: 41 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Count matches (exit code 1 if 0 matches; so force through with || true)
echo "== direct assignments to createRenderRoot =="
rg -n --type=ts -S '\.createRenderRoot\s*=' projects || true
rg -n --type=tsx -S '\.createRenderRoot\s*=' projects || true
echo
echo "== any identifier createRenderRoot in core decorators =="
rg -n --type=ts -S '\bcreateRenderRoot\b' projects/core/src/internal/decorators || true
echo
echo "== defineProperty targeting createRenderRoot key =="
rg -n --type=ts -S "Object\.defineProperty\([^)]*'createRenderRoot'" projects || true
rg -n --type=ts -S "Object\.defineProperty\([^)]*\"createRenderRoot\"" projects || true
echo
echo "== other decorators mixins usage =="
rg -n --type=ts -S '\bcreateRenderRoot\b' projects/core/src/internal || trueRepository: NVIDIA/elements Length of output: 1599 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== scoped-registry.ts (around createRenderRoot patch) =="
sed -n '1,120p' projects/core/src/internal/decorators/scoped-registry.ts | nl -ba | sed -n '1,120p'
echo
echo "== i18n.controller.examples.ts (around createRenderRoot) =="
sed -n '1,120p' projects/core/src/internal/controllers/i18n.controller.examples.ts | nl -ba | sed -n '1,120p'Repository: NVIDIA/elements Length of output: 177 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== projects/core/src/internal/decorators/scoped-registry.ts (lines 1-120) =="
cat -n projects/core/src/internal/decorators/scoped-registry.ts | sed -n '1,140p'
echo
echo "== projects/core/src/internal/controllers/i18n.controller.examples.ts (lines 1-120) =="
cat -n projects/core/src/internal/controllers/i18n.controller.examples.ts | sed -n '1,140p'Repository: NVIDIA/elements Length of output: 5995 Hardening: consider setting This repo has no additional 🔧 Optional hardening Object.defineProperty(host, 'createRenderRoot', {
configurable: true,
+ writable: true,
value(this: ScopedRegistryHost) {
const renderRoot = createRenderRoot.call(this);
if (renderRoot instanceof ShadowRoot) {
this.renderOptions ??= {};
this.renderOptions.creationScope = createScopedCreationScope(renderRoot.ownerDocument, customElementRegistry);
}
return renderRoot;
}
});📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| /** decorator which registers element dependencies with the scoped custom element registry when available */ | ||||||||||||||||||||||||||||||||||||||||||||||||
| export function scopedRegistry(): ClassDecorator { | ||||||||||||||||||||||||||||||||||||||||||||||||
| return (target: LegacyDecoratorTarget) => { | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -16,6 +56,7 @@ export function scopedRegistry(): ClassDecorator { | |||||||||||||||||||||||||||||||||||||||||||||||
| configurable: true, | ||||||||||||||||||||||||||||||||||||||||||||||||
| value: { ...(element.shadowRootOptions ?? { mode: 'open' }), customElementRegistry } | ||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||
| attachLitCreationScope(element, customElementRegistry); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| defineElement(element, customElementRegistry); | ||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
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.
Use
elementIsStablein this unit test before asserting render internals.This test uses
createFixturebut skips theelementIsStablepattern required for.test.ts, which can make stability timing brittle across environments.🧪 Suggested update
As per coding guidelines "
**/*.test.ts: Unit tests (*.test.ts) must follow patterns defined in/projects/site/src/docs/internal/guidelines/testing-unit.md, including createFixture and elementIsStable patterns".Also applies to: 115-118
🤖 Prompt for AI Agents