fix(popconfirm): restore compatibility and popup rendering#8525
fix(popconfirm): restore compatibility and popup rendering#8525xjccc wants to merge 10 commits intovueComponent:feat/vaporfrom
Conversation
restore Popconfirm legacy prop and slot precedence, callback compatibility, async confirm loading, and ESC close behavior fix playground script setup parsing for preview demos switch shared popup positioning to left/top and restore legacy-style arrow rendering add regression coverage for popconfirm behavior, demos, and tooltip popup positioning Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
This PR restores legacy-compatible Popconfirm behavior and fixes popup rendering/positioning regressions across Trigger-based overlays, with updated styling and regression tests to prevent reintroductions.
Changes:
- Reworks Trigger popup positioning to use
left/top(notransform) and restores a legacy-style arrow DOM/CSS structure. - Restores Popconfirm compatibility behaviors (content precedence, async confirm loading, Escape-to-close) and expands type/test coverage.
- Fixes the playground SFC parser to better detect
<script setup>blocks so demos render correctly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/_internal/trigger/Trigger.vue | Switches Floating UI positioning to non-transform styles and updates arrow DOM. |
| packages/ui/src/_internal/trigger/style/index.css | Rebuilds arrow styling; introduces --ant-trigger-arrow-background variable-driven fill. |
| packages/ui/src/components/tooltip/style/index.css | Updates tooltip arrow styling to use the new trigger arrow background variable. |
| packages/ui/src/components/popover/style/index.css | Updates popover arrow styling to use the new trigger arrow background variable. |
| packages/ui/src/components/popconfirm/style/index.css | Updates popconfirm arrow styling to use the new trigger arrow background variable. |
| packages/ui/src/components/tooltip/tests/index.test.ts | Adds regression test to ensure popup uses left/top instead of transform. |
| packages/ui/src/components/popconfirm/types.ts | Adds callback prop types, slot prop types, and updates openChange/visibleChange event typing. |
| packages/ui/src/components/popconfirm/Popconfirm.vue | Implements compatibility logic (content precedence, async confirm loading, Escape-to-close), updates button rendering and slot props. |
| packages/ui/src/components/popconfirm/tests/index.test.ts | Expands Popconfirm regression coverage for rendering, handlers, async confirm loading, escape-to-close, and compatibility behaviors. |
| packages/ui/src/components/popconfirm/tests/snapshots/demo.test.ts.snap | Updates demo snapshots to reflect Trigger/button rendering changes. |
| apps/playground/src/views/EditorView.vue | Updates regex used to extract <script setup> blocks for demo compilation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onClick: onCancel as (event: MouseEvent) => void, | ||
| size: 'small' as const, | ||
| ...(props.cancelButtonProps ?? {}), | ||
| cancel: onCancel as (event: MouseEvent) => void, | ||
| })) | ||
|
|
||
| const okButtonSlotProps = computed(() => ({ | ||
| onClick: onConfirm, | ||
| type: props.okType, | ||
| size: 'small' as const, | ||
| ...(props.okButtonProps ?? {}), |
There was a problem hiding this comment.
cancelButtonSlotProps/okButtonSlotProps spread cancelButtonProps/okButtonProps after setting onClick, so a user-provided onClick inside those props will override the internal handler and the Popconfirm may no longer close / enter loading unless the slot implementation calls cancel/confirm manually. To keep the default behavior reliable, merge in the button props first and then set onClick last (or wrap both handlers so both run).
| onClick: onCancel as (event: MouseEvent) => void, | |
| size: 'small' as const, | |
| ...(props.cancelButtonProps ?? {}), | |
| cancel: onCancel as (event: MouseEvent) => void, | |
| })) | |
| const okButtonSlotProps = computed(() => ({ | |
| onClick: onConfirm, | |
| type: props.okType, | |
| size: 'small' as const, | |
| ...(props.okButtonProps ?? {}), | |
| size: 'small' as const, | |
| ...(props.cancelButtonProps ?? {}), | |
| onClick: onCancel as (event: MouseEvent) => void, | |
| cancel: onCancel as (event: MouseEvent) => void, | |
| })) | |
| const okButtonSlotProps = computed(() => ({ | |
| type: props.okType, | |
| size: 'small' as const, | |
| ...(props.okButtonProps ?? {}), | |
| onClick: onConfirm, |
|
|
||
| const scriptSetupMatch = source.match( | ||
| /<script\s+setup(?:\s+lang="ts")?\s*>([\s\S]*?)<\/script>/, | ||
| /<script\b(?=[^>]*\bsetup\b)[^>]*>([\s\S]*?)<\/script>/, |
There was a problem hiding this comment.
The new <script setup> regex will also match any <script> tag whose attributes/values merely contain the word setup (e.g. src="setup.ts", data-setup="..."), which can cause the playground to mis-detect and miscompile non-setup script blocks. Consider tightening the pattern to specifically match the setup attribute name (optionally with =), rather than searching for a standalone setup token anywhere in the tag.
| /<script\b(?=[^>]*\bsetup\b)[^>]*>([\s\S]*?)<\/script>/, | |
| /<script\b(?=[^>]*\ssetup(?:\s*=\s*(?:"[^"]*"|'[^']*'|[^\s"'=<>`]+))?(?=\s|>))[^>]*>([\s\S]*?)<\/script>/, |
preserve internal confirm and cancel handlers when slot button props provide custom onClick callbacks tighten playground script setup detection to only match the setup attribute add regression coverage for custom slot buttons invoking default actions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('does not warn for the default okType legacy mapping', async () => { | ||
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) | ||
|
|
||
| trackMount(mount(Popconfirm, { | ||
| attachTo: document.body, | ||
| props: { title: 'Are you sure?', open: true }, | ||
| slots: { default: () => h('span', 'Delete') }, | ||
| }) | ||
| const buttons = wrapper.find('.ant-popconfirm-buttons') | ||
| expect(buttons.text()).toContain('OK') | ||
| expect(buttons.text()).toContain('Cancel') | ||
| })) | ||
|
|
||
| await flushPopup() | ||
|
|
||
| expect(warn).not.toHaveBeenCalledWith( | ||
| expect.stringContaining('Button: `type="primary"` is deprecated'), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
The console.warn spy created here is never restored. Since afterEach doesn’t call vi.restoreAllMocks() / warn.mockRestore(), this can leak into later tests and mask real warnings or change assertions. Restore the spy at the end of the test (or add a vi.restoreAllMocks() in afterEach).
restore mocked globals after each Popconfirm test case to avoid warning spy leakage between tests Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface PopconfirmEmits { | ||
| (e: 'update:open', open: boolean): void | ||
| (e: 'openChange', open: boolean): void | ||
| (e: 'confirm', event: MouseEvent): void | ||
| (e: 'cancel', event: MouseEvent): void | ||
| (e: 'openChange', open: boolean, event?: PopconfirmOpenChangeEvent): void | ||
| /** @deprecated */ | ||
| (e: 'update:visible', open: boolean): void | ||
| /** @deprecated */ | ||
| (e: 'visibleChange', open: boolean): void | ||
| (e: 'visibleChange', open: boolean, event?: PopconfirmOpenChangeEvent): void | ||
| } |
There was a problem hiding this comment.
PopconfirmEmits no longer declares confirm/cancel, but the codebase (e.g. demo/promise.vue) uses <a-popconfirm @confirm @cancel>. Removing these emits is a public typing/API regression: template type-checking will flag @confirm/@cancel as invalid, and consumers relying on event typing will break. Consider restoring confirm/cancel in the component’s declared events (or otherwise ensuring @confirm/@cancel remain type-safe), while still supporting Promise-returning handlers for async confirm loading.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function invokeConfirmHandlers(e: MouseEvent) { | ||
| const handlers = getCompatHandlers(props.onConfirm, 'onConfirm') | ||
| const results = handlers.map(handler => handler(e)) | ||
| return results.find(isThenable) ?? results.find(result => result !== undefined) | ||
| } | ||
|
|
||
| function invokeCancelHandlers(e: MouseEvent) { | ||
| const handlers = getCompatHandlers(props.onCancel, 'onCancel') | ||
| handlers.forEach(handler => handler(e)) | ||
| } | ||
|
|
||
| function onConfirm(e: MouseEvent) { | ||
| emit('confirm', e) | ||
| setOpen(false) | ||
| if (confirmLoading.value) { | ||
| return | ||
| } | ||
|
|
||
| const result = invokeConfirmHandlers(e) | ||
| if (isThenable(result)) { | ||
| confirmLoading.value = true | ||
| result.then( | ||
| () => { | ||
| confirmLoading.value = false | ||
| setOpen(false) | ||
| }, | ||
| () => { | ||
| confirmLoading.value = false | ||
| }, | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| setOpen(false, e) | ||
| } | ||
|
|
||
| function onCancel(e: MouseEvent) { | ||
| invokeCancelHandlers(e) | ||
| setOpen(false, e) |
There was a problem hiding this comment.
confirm / cancel are still declared in PopconfirmEmits, but the component no longer emits them. onConfirm/onCancel callbacks are invoked, and the popup closes, but emit('confirm', e) / emit('cancel', e) never run, which breaks consumers relying on emitted events (including Vue listener modifiers like @confirm.once / @cancel.once, which won't be picked up by the new onConfirm/onCancel prop handling). Consider emitting the events in onConfirm/onCancel (and keeping the callback invocation), or else removing the emits and updating typings/docs accordingly.
| it('calls confirm handler and closes popup', async () => { | ||
| const confirm = vi.fn() | ||
| const wrapper = trackMount(mount(Popconfirm, { | ||
| attachTo: document.body, | ||
| props: { title: 'Are you sure?', trigger: 'click' }, | ||
| attrs: { onConfirm: confirm }, | ||
| slots: { default: () => h('span', 'Delete') }, | ||
| })) | ||
| await wrapper.find('.ant-trigger-wrapper').trigger('click') | ||
| await flushPopup() | ||
|
|
||
| getButtons()[1]?.click() | ||
| await flushPopup() | ||
|
|
||
| expect(confirm).toHaveBeenCalledTimes(1) | ||
| expect((wrapper.vm as { getPopupDomNode: () => HTMLElement | null }).getPopupDomNode()?.style.display).toBe('none') | ||
| }) | ||
|
|
||
| it('emits cancel event', async () => { | ||
| const wrapper = mount(Popconfirm, { | ||
| it('calls cancel handler and closes popup', async () => { | ||
| const cancel = vi.fn() | ||
| const wrapper = trackMount(mount(Popconfirm, { | ||
| attachTo: document.body, | ||
| props: { title: 'Are you sure?', open: true }, | ||
| attrs: { onCancel: cancel }, | ||
| slots: { default: () => h('span', 'Delete') }, | ||
| }) | ||
| const buttons = wrapper.findAll('.ant-popconfirm-buttons .ant-btn') | ||
| const cancelBtn = buttons[0] | ||
| await cancelBtn.trigger('click') | ||
| expect(wrapper.emitted('cancel')).toBeTruthy() | ||
| })) | ||
|
|
||
| await flushPopup() | ||
| getButtons()[0]?.click() | ||
| await flushPopup() | ||
|
|
||
| expect(cancel).toHaveBeenCalledTimes(1) | ||
| expect(wrapper.emitted('openChange')?.at(-1)?.[0]).toBe(false) | ||
| expect(wrapper.emitted('openChange')?.at(-1)?.[1]).toBeInstanceOf(MouseEvent) | ||
| expect(wrapper.emitted('visibleChange')?.at(-1)?.[1]).toBeInstanceOf(MouseEvent) | ||
| }) |
There was a problem hiding this comment.
Current tests no longer assert that clicking OK/Cancel emits the confirm / cancel events (only callback props are verified). Since confirm/cancel are still part of the component's declared emits contract, it would be good to add regression assertions for wrapper.emitted('confirm') and wrapper.emitted('cancel') on button clicks (and optionally a .once listener case) to ensure event semantics remain supported alongside the new callback compatibility.
emit confirm and cancel events again without double-invoking callback props add regression coverage for emitted events and once-modified listeners Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isUserControlled = computed(() => { | ||
| const rawProps = instance.vnode.props || {} | ||
| return 'open' in rawProps || 'visible' in rawProps | ||
| return hasExplicitProp('open') || hasExplicitProp('visible') | ||
| }) | ||
|
|
||
| const mergedOpen = computed(() => { | ||
| if (isUserControlled.value) { | ||
| return props.open ?? props.visible ?? false | ||
| } | ||
| if (hasExplicitProp('open')) return props.open ?? false | ||
| if (hasExplicitProp('visible')) return props.visible ?? false | ||
| return internalOpen.value |
There was a problem hiding this comment.
open is documented as “null = uncontrolled”, but the current controlled-mode detection treats the prop as controlled whenever the key exists. If a consumer passes :open="null", isUserControlled becomes true and mergedOpen always resolves to false, preventing the Popconfirm from ever opening. Consider treating open === null as “not explicitly controlled” (e.g., require props.open !== null when deciding controlled mode / when reading the controlled value).
| result.then( | ||
| () => { | ||
| confirmLoading.value = false | ||
| setOpen(false) |
There was a problem hiding this comment.
When onConfirm returns a Promise, the popup closes via setOpen(false) without passing the original click event. This makes openChange / visibleChange emissions inconsistent with the sync-confirm path (which passes e), and can break consumers that rely on the event parameter. Consider calling setOpen(false, e) on promise resolve (and you can also rely on setOpen to reset confirmLoading to avoid duplicated state updates).
| setOpen(false) | |
| setOpen(false, e) |
| function emitCompatEvent(eventName: 'confirm' | 'cancel', event: MouseEvent) { | ||
| const handlerName = eventName === 'confirm' ? 'onConfirm' : 'onCancel' | ||
| const vnodeProps = instance.vnode.props as Record<string, unknown> | null | ||
| const emitEvent = () => { | ||
| if (eventName === 'confirm') { | ||
| emit('confirm', event) | ||
| return | ||
| } | ||
|
|
||
| emit('cancel', event) | ||
| } | ||
|
|
||
| if (!vnodeProps || !(handlerName in vnodeProps)) { | ||
| emitEvent() | ||
| return | ||
| } | ||
|
|
||
| const handler = vnodeProps[handlerName] | ||
|
|
||
| try { | ||
| vnodeProps[handlerName] = undefined | ||
| emitEvent() | ||
| } finally { | ||
| vnodeProps[handlerName] = handler | ||
| } | ||
| } |
There was a problem hiding this comment.
emitCompatEvent temporarily mutates instance.vnode.props to avoid double-invoking onConfirm/onCancel. vnode.props is an internal Vue structure and mutating it is brittle (may break with Vue internal changes, devtools assumptions, or concurrent renders). If possible, prefer an approach that avoids mutating vnode.props (e.g., distinguish callback props from emitted event listeners via a different prop name, or read callback handlers from a dedicated prop while leaving event emission to Vue’s normal listener invocation).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const baseStyles = floatingStyles.value as CSSProperties | ||
|
|
||
| if (props.zIndex == null && !props.popupStyle) { | ||
| return baseStyles | ||
| } | ||
|
|
||
| const styles: CSSProperties[] = [baseStyles] | ||
|
|
||
| if (props.zIndex != null) { | ||
| styles.push({ zIndex: props.zIndex }) | ||
| } | ||
| if (props.popupStyle) { | ||
| styles.push(props.popupStyle as CSSProperties) | ||
| } | ||
|
|
||
| return styles |
There was a problem hiding this comment.
popupStyles merges floatingStyles with props.popupStyle, but this code has to cast props.popupStyle to CSSProperties. In TriggerProps (packages/ui/src/_internal/trigger/types.ts) popupStyle is currently typed as Record<string, string>, which is more restrictive than what this implementation now accepts (numbers, CSS variables, etc.) and forces unsafe casts. Consider updating the prop type to CSSProperties (or StyleValue) so callers and this component stay type-aligned.
| const baseStyles = floatingStyles.value as CSSProperties | |
| if (props.zIndex == null && !props.popupStyle) { | |
| return baseStyles | |
| } | |
| const styles: CSSProperties[] = [baseStyles] | |
| if (props.zIndex != null) { | |
| styles.push({ zIndex: props.zIndex }) | |
| } | |
| if (props.popupStyle) { | |
| styles.push(props.popupStyle as CSSProperties) | |
| } | |
| return styles | |
| if (props.zIndex == null && !props.popupStyle) { | |
| return floatingStyles.value | |
| } | |
| return { | |
| ...floatingStyles.value, | |
| ...(props.zIndex != null ? { zIndex: props.zIndex } : {}), | |
| ...(props.popupStyle ?? {}), | |
| } |
| :where(.ant-trigger-arrow)::after { | ||
| content: ''; | ||
| position: absolute; | ||
| width: 8.970562748477143px; | ||
| height: 8.970562748477143px; | ||
| bottom: 0; | ||
| left: 0; | ||
| right: 0; | ||
| margin: auto; | ||
| border-radius: 0 0 2px 0; | ||
| transform: translateY(50%) rotate(-135deg); | ||
| box-shadow: 3px 3px 7px rgba(0, 0, 0, 0.07); |
There was a problem hiding this comment.
The new arrow CSS uses several high-precision magic numbers (e.g. 8.970562748477143px) and an inline clip-path: path('...') string. This is hard to audit and maintain; please add an explanatory comment/source (e.g. derived from Ant Design geometry) or refactor to named CSS variables so future changes don’t risk subtly breaking the arrow shape.
Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| v-bind="props.cancelButtonProps" | ||
| @click="onCancel" |
There was a problem hiding this comment.
cancelButtonProps.onClick (and array handlers) will never run for the default cancel button because @click="onCancel" overrides any onClick coming from v-bind="props.cancelButtonProps". Consider binding cancelButtonSlotProps to the default <a-button> (or otherwise composing the handlers) so user-provided click handlers are preserved.
| v-bind="props.cancelButtonProps" | |
| @click="onCancel" | |
| v-bind="cancelButtonSlotProps" |
| v-bind="props.okButtonProps" | ||
| :loading="confirmLoading" | ||
| @click="onConfirm" |
There was a problem hiding this comment.
okButtonProps.onClick will be dropped for the default OK button because @click="onConfirm" overrides the onClick from v-bind="props.okButtonProps". Consider using the already-computed okButtonSlotProps for the default button (or composing handlers) so both user and internal logic run.
| v-bind="props.okButtonProps" | |
| :loading="confirmLoading" | |
| @click="onConfirm" | |
| v-bind="okButtonSlotProps" | |
| :loading="confirmLoading" |
| document.addEventListener('keydown', onDocumentKeydown) | ||
| return | ||
| } | ||
|
|
||
| document.removeEventListener('keydown', onDocumentKeydown) |
There was a problem hiding this comment.
confirmLoading is only reset inside setOpen(false, ...). In fully controlled mode, the parent can close the Popconfirm by updating open directly without calling setOpen, leaving confirmLoading stuck true the next time it opens. Consider resetting confirmLoading whenever mergedOpen transitions to false in this watcher.
Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isUserControlled.value) { | ||
| return props.open ?? props.visible ?? false | ||
| } | ||
| if (hasExplicitProp('open') && props.open !== null) return props.open |
There was a problem hiding this comment.
mergedOpen can return props.open directly when the open prop is explicitly present and not null. If a consumer passes :open="undefined" (or otherwise provides an explicit-but-undefined value), mergedOpen becomes undefined and is forwarded to Trigger’s open prop (which expects a boolean), making open-state logic rely on implicit truthiness. Consider coercing to a boolean (e.g. return !!props.open) or falling back to false when props.open is undefined while still treating null as uncontrolled.
| if (hasExplicitProp('open') && props.open !== null) return props.open | |
| if (hasExplicitProp('open') && props.open !== null) return props.open ?? false |
Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| emitCompatEvent('confirm', e) | ||
| const result = invokeConfirmHandlers(e) | ||
| if (isThenable(result)) { | ||
| confirmLoading.value = true | ||
| result.then( | ||
| () => { | ||
| setOpen(false, e) | ||
| }, | ||
| () => { | ||
| confirmLoading.value = false | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Async onConfirm promises can still call setOpen(false, e) after the Popconfirm has already been closed via another path (e.g., click outside / programmatic close). This can re-emit openChange/visibleChange a second time with a stale MouseEvent and may cause controlled parents to run close logic twice. Consider tracking the pending confirm (e.g., incrementing token / capturing the promise) and only closing/emitting when the popup is still open and the token matches, or canceling the pending close when mergedOpen becomes false.
Co-Authored-By: GitHub Copilot <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Testing
Related: #8497