feat(custom-element): allow specifying additional options for shadowRoot in custom elements
#12965
Conversation
padcom
changed the title
shadowRoot in custom elements
padcom
changed the title
shadowRoot in custom elementsshadowRoot in custom elements
padcom
force-pushed
the
GH-12964
branch
2 times, most recently
from
045fa73 to
be80e79
Compare
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
edison1105
added
wait changes
🔨 p3-minor-bug
edison1105
added
ready to merge
|
Thanks @padcom |
|
Please excuse my impatience, but when can we expect this to be released? I have a project that makes heavy use of web components and it is something that's blocking the release of interactive components. |
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CE as VueElement (Custom Element)
participant SR as ShadowRoot
Dev->>CE: defineCustomElement({ ..., shadowRootOptions })
CE->>CE: constructor reads _def.shadowRootOptions
CE->>SR: this.attachShadow( extend({ mode: 'open' }, shadowRootOptions) )
SR-->>CE: created with merged options (e.g., delegatesFocus)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (1)
packages/runtime-dom/__tests__/customElement.spec.ts (1)
475-493: Good test coverage for the new feature.The test correctly verifies that the
delegatesFocusoption is properly passed to the shadow root. Skipping the test due to jsdom limitations is appropriate, with clear comments explaining why it's skipped and referencing the relevant GitHub issue.Given the jsdom limitation, consider adding a comment in the test description suggesting that this should be manually tested in a real browser environment.
// https://github.com/vuejs/core/issues/12964 // Disabled because of missing support for `delegatesFocus` in jsdom // https://github.com/jsdom/jsdom/issues/3418 // eslint-disable-next-line vitest/no-disabled-tests -test.skip('shadowRoot should be initialized with delegatesFocus', () => { +test.skip('shadowRoot should be initialized with delegatesFocus (requires manual testing in a real browser)', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
packages/runtime-dom/__tests__/customElement.spec.ts(1 hunks)packages/runtime-dom/src/apiCustomElement.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/runtime-dom/src/apiCustomElement.ts (1)
packages/shared/src/general.ts (1)
extend(24-24)
packages/runtime-dom/__tests__/customElement.spec.ts (2)
packages/runtime-dom/src/apiCustomElement.ts (1)
defineCustomElement(168-186)packages/runtime-core/src/index.ts (1)
h(109-109)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / unit-test-windows
- GitHub Check: test / e2e-test
🔇 Additional comments (2)
packages/runtime-dom/src/apiCustomElement.ts (2)
56-56: Great addition! This extends Vue's custom elements API.Adding the
shadowRootOptionsproperty to theCustomElementOptionsinterface allows users to specify additional shadow DOM initialization options, such asdelegatesFocus, while keeping the shadow root mode enforced as 'open'.
267-271: Well implemented shadow root options handling.The implementation properly uses
extendto merge the user-providedshadowRootOptionswith{ mode: 'open' }, ensuring the shadow root is always created with an 'open' mode while allowing additional configuration options to be passed.This approach addresses the requirements from issue #12964 while maintaining compatibility with existing code.
edison1105
changed the title
shadowRoot in custom elementsshadowRoot in custom elements
|
LGTM~ Maybe we can use vitest's browser mode for testing, but it's an experimental feature for now. |
edison1105
changed the title
shadowRoot in custom elementsshadowRoot in custom elements
edison1105
added
version: minor
✨ feature request
|
@edison1105 This PR is here since February. It's June now. Can we get it released or do I really need to fork Vue to be able to use it? I am blocked by this particular PR with the next project which will be mostly about web components... |
|
@padcom |
|
All good. Any plans on the next big release where this could find its way in? |
|
@padcom |
|
Can I expect this change to be merged this year? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/runtime-dom/__tests__/customElement.spec.ts (2)
505-523: Run conditionally instead of permanently skipping; enable in real browser runnersGate the test via feature detection so it executes under Vitest browser/Playwright and remains skipped on jsdom. Removes the lint suppression and keeps CI green.
-// eslint-disable-next-line vitest/no-disabled-tests -test.skip('shadowRoot should be initialized with delegatesFocus', () => { +const supportsDelegatesFocus = + typeof ShadowRoot !== 'undefined' && + ShadowRoot.prototype && + 'delegatesFocus' in (ShadowRoot.prototype as any) +const itIf = supportsDelegatesFocus ? test : test.skip +itIf('shadowRoot should be initialized with delegatesFocus', () => { const E = defineCustomElement( { render() { - return [h('input', { tabindex: 1 })] + return [h('input', { tabindex: 1 })] }, }, { shadowRootOptions: { delegatesFocus: true } }, ) customElements.define('my-el-with-delegate-focus', E) const e = new E() container.appendChild(e) expect(e.shadowRoot!.delegatesFocus).toBe(true) -}) +})If you prefer, I can also provide a Playwright spec asserting behavioral focus delegation (host.focus() moves focus to the inner input).
513-514: Minor: prefer tabindex=0 for focusable content in teststabindex=1 changes tab order and is atypical; tabindex=0 aligns with natural tab flow and is sufficient for this check.
- return [h('input', { tabindex: 1 })] + return [h('input', { tabindex: 0 })]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime-dom/__tests__/customElement.spec.ts(1 hunks)packages/runtime-dom/src/apiCustomElement.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime-dom/src/apiCustomElement.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-dom/__tests__/customElement.spec.ts (1)
packages/runtime-dom/src/apiCustomElement.ts (1)
defineCustomElement(168-186)
|
Strictly speaking, this PR should be considered a feature, but since it's relatively simple, We plan to merge it into the next patch version. However, this change requires updating the documentation at https://vuejs.org/api/custom-elements.html#definecustomelement, where we need to add the new |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/runtime-dom/__tests__/customElement.spec.ts (1)
529-548: Well-structured test for the delegatesFocus functionalityThe test correctly validates that
shadowRootOptionswithdelegatesFocus: trueproperly configures the shadow root. The use oftest.skipwith a detailed comment about jsdom limitations is appropriate for tracking this test until browser mode testing becomes viable.However, there's a duplicate test in the mounting/unmount section that should be consolidated to avoid test redundancy.
Consider consolidating the delegatesFocus tests into a single test block to reduce duplication:
- // https://github.com/vuejs/core/issues/12964 - // Disabled because of missing support for `delegatesFocus` in jsdom - // https://github.com/jsdom/jsdom/issues/3418 - // eslint-disable-next-line vitest/no-disabled-tests - test.skip('shadowRoot should be initialized with delegatesFocus', () => { - const E = defineCustomElement( - { - render() { - return [h('input', { tabindex: 1 })] - }, - }, - { shadowRootOptions: { delegatesFocus: true } }, - ) - customElements.define('my-el-with-delegate-focus', E) - - const e = new E() - container.appendChild(e) - expect(e.shadowRoot!.delegatesFocus).toBe(true) - })And move the comprehensive test to a dedicated
shadowRootOptionsdescribe block to better organize related functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime-dom/__tests__/customElement.spec.ts(1 hunks)packages/runtime-dom/src/apiCustomElement.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime-dom/src/apiCustomElement.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-dom/__tests__/customElement.spec.ts (1)
packages/runtime-dom/src/apiCustomElement.ts (1)
defineCustomElement(168-186)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test / lint-and-test-dts
- GitHub Check: test / e2e-test
- GitHub Check: test / unit-test-windows
- GitHub Check: continuous-release
- GitHub Check: test / unit-test
- GitHub Check: autofix
- GitHub Check: upload
🔇 Additional comments (1)
packages/runtime-dom/__tests__/customElement.spec.ts (1)
149-149: Ignore: delegatesFocus test already present (skipped)packages/runtime-dom/tests/customElement.spec.ts contains test.skip('shadowRoot should be initialized with delegatesFocus') at ~lines 534–547 (disabled due to jsdom); no duplicate test exists in the mounting/unmount describe block.
Likely an incorrect or invalid review comment.
|
Merci! |
3 participants
When creating
shadowRootusingthis.attachShadow()there are more options that can be very useful in specific scenarios.https://developer.mozilla.org/en-US/docs/Web/API/Element/attachShadow#parameters
At the moment there is no way to pass those additional options (like
delegatesFocus: truefor focus management). This PR adds the ability to not only specify those but also to override themode: 'open'should one choose to do that.close #12964
Summary by CodeRabbit
New Features
Tests