fix(core): chrome scoped registry and popover behavior - #117
Merged
Conversation
- fixes issues due to chromium behavior changes for scoped registries - fixes issues due to chromium behavior changes for html popover elements Signed-off-by: Cory Rylan <crylan@nvidia.com>
📝 WalkthroughWalkthroughThis PR extends the ChangesLit Scoped Registry Support
Listbox Click Activation Handler
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/core/src/internal/utils/focus.ts (1)
77-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the disabled-click unit test;
clicktyping asPointerEventis correct.
onListboxActivate’sfn: (KeyboardEvent | PointerEvent)and the'click'listener parameter type are consistent with TypeScript’s DOM typings (whereHTMLElementEventMap['click']isPointerEvent), so theMouseEvent-based type change isn’t needed.projects/core/src/internal/utils/focus.test.ts: the “click and disabled” test dispatches'pointerup'even though the handler now activates on'click'; switch it to a real click (e.g., setselect.disabled = trueand callemulateClick(select)) to actually cover the disabled behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/internal/utils/focus.ts` around lines 77 - 90, The failing unit test should be updated to trigger the actual 'click' path the onListboxActivate handler uses: leave onListboxActivate's signature and the 'click' listener parameter as PointerEvent (no MouseEvent change), then update the "click and disabled" test in focus.test.ts to set select.disabled = true and invoke a real click (e.g., call emulateClick(select) or dispatch a 'click' event) instead of dispatching 'pointerup' so the disabled-branch in onListboxActivate (and the fn callback) is exercised correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/internal/decorators/scoped-registry.test.ts`:
- Line 8: The test uses createFixture without waiting for stability; update the
test to import and await elementIsStable from the testing helpers (e.g., import
{ createFixture, removeFixture, elementIsStable } from '`@internals/testing`') and
call await elementIsStable(fixture) immediately after createFixture(...) and
before any assertions that inspect render internals (apply same change for the
other occurrences in this file such as the blocks around the checks at the later
assertions).
In `@projects/core/src/internal/decorators/scoped-registry.ts`:
- Around line 35-45: The Object.defineProperty call that patches
host.createRenderRoot should explicitly set writable: true to harden the
override; update the descriptor passed to Object.defineProperty for the
createRenderRoot patch (the function defined on host, i.e. value(this:
ScopedRegistryHost) { ... }) to include writable: true alongside configurable:
true so future reassignments to createRenderRoot succeed; keep the existing
logic that computes renderRoot, checks ShadowRoot, sets
this.renderOptions.creationScope via
createScopedCreationScope(renderRoot.ownerDocument, customElementRegistry), and
returns renderRoot.
---
Outside diff comments:
In `@projects/core/src/internal/utils/focus.ts`:
- Around line 77-90: The failing unit test should be updated to trigger the
actual 'click' path the onListboxActivate handler uses: leave
onListboxActivate's signature and the 'click' listener parameter as PointerEvent
(no MouseEvent change), then update the "click and disabled" test in
focus.test.ts to set select.disabled = true and invoke a real click (e.g., call
emulateClick(select) or dispatch a 'click' event) instead of dispatching
'pointerup' so the disabled-branch in onListboxActivate (and the fn callback) is
exercised correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1f1422b6-19c6-4c0e-8dfa-77c554f98225
📒 Files selected for processing (3)
projects/core/src/internal/decorators/scoped-registry.test.tsprojects/core/src/internal/decorators/scoped-registry.tsprojects/core/src/internal/utils/focus.ts
| import { LitElement } from 'lit'; | ||
| import { html as staticHtml, unsafeStatic } from 'lit/static-html.js'; | ||
| import { describe, expect, it, beforeEach, afterEach } from 'vitest'; | ||
| import { createFixture, removeFixture } from '@internals/testing'; |
There was a problem hiding this comment.
Use elementIsStable in this unit test before asserting render internals.
This test uses createFixture but skips the elementIsStable pattern required for .test.ts, which can make stability timing brittle across environments.
🧪 Suggested update
-import { createFixture, removeFixture } from '`@internals/testing`';
+import { createFixture, elementIsStable, removeFixture } from '`@internals/testing`';
...
try {
await element.updateComplete;
+ await elementIsStable(element);
expect(typeof element.renderOptions.creationScope?.importNode).toBe('function');
expect(element.shadowRoot!.querySelector(child.metadata.tag)).toBeInstanceOf(child);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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/internal/decorators/scoped-registry.test.ts` at line 8, The
test uses createFixture without waiting for stability; update the test to import
and await elementIsStable from the testing helpers (e.g., import {
createFixture, removeFixture, elementIsStable } from '`@internals/testing`') and
call await elementIsStable(fixture) immediately after createFixture(...) and
before any assertions that inspect render internals (apply same change for the
other occurrences in this file such as the blocks around the checks at the later
assertions).
Comment on lines
+35
to
+45
| 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; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 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 writable: true on the createRenderRoot patch
This repo has no additional createRenderRoot = ... reassignment or other Object.defineProperty(..., 'createRenderRoot' ...) uses beyond the scoped-registry patch, so writable: false is unlikely to break existing composition.
🔧 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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; | |
| } | |
| }); | |
| 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; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/internal/decorators/scoped-registry.ts` around lines 35 -
45, The Object.defineProperty call that patches host.createRenderRoot should
explicitly set writable: true to harden the override; update the descriptor
passed to Object.defineProperty for the createRenderRoot patch (the function
defined on host, i.e. value(this: ScopedRegistryHost) { ... }) to include
writable: true alongside configurable: true so future reassignments to
createRenderRoot succeed; keep the existing logic that computes renderRoot,
checks ShadowRoot, sets this.renderOptions.creationScope via
createScopedCreationScope(renderRoot.ownerDocument, customElementRegistry), and
returns renderRoot.
johnyanarella
approved these changes
Jun 3, 2026
Collaborator
Author
|
🎉 This issue has been resolved in version 0.2.2 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.1.0 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.12 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.3.0 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.10 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.4.0 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests