Skip to content

Conversation

@Antreesy
Copy link
Contributor

☑️ Resolves

getPopoverContentElement() can be undefined, so it should not fail when trying to add a listener

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 2️⃣ Backport to stable8 for maintained Vue 2 version or not applicable

@Antreesy Antreesy added this to the 9.0.0-rc.3 milestone Jun 23, 2025
@Antreesy Antreesy requested review from ShGKme and susnux June 23, 2025 16:08
@Antreesy Antreesy self-assigned this Jun 23, 2025
@Antreesy Antreesy added bug Something isn't working 3. to review Waiting for reviews feature: popover Related to the popovermenu component labels Jun 23, 2025
@ShGKme
Copy link
Contributor

ShGKme commented Jun 23, 2025

getPopoverContentElement() can be undefined

Is it expected? Why?

@Antreesy
Copy link
Contributor Author

Is it expected? Why?

I haven't checked, but probably cause it's a ref. Didn't you add it yourself? 👀

@susnux
Copy link
Contributor

susnux commented Jun 23, 2025

so it should not fail when trying to add a listener

when is this the case? We had the same discussion when I added the optional chaining operator:
#6802 (comment)

@susnux
Copy link
Contributor

susnux commented Jun 23, 2025

And from the code I do not see where this can ever be undefined in those event listeners

@Antreesy
Copy link
Contributor Author

Antreesy commented Jun 23, 2025

Currently happening, when there is an error while closing popover. Maybe it's redundant, but nevertheless - we can put it in one place and skip in another two

@ShGKme
Copy link
Contributor

ShGKme commented Jun 23, 2025

I haven't checked, but probably cause it's a ref. Didn't you add it yourself? 👀

When it's undefined, afterShow and afterHide events are not triggered.

If undefined is expected here, then the component triggers no afterShow/afterHide event in a specific case. Then it's interesting, what is the case. E.g., this behavior should be documented.

If undefined is not expected here, then something goes wrong, and we need to fix it. By adding ?. we hide an issue caused by another issue.

@ShGKme
Copy link
Contributor

ShGKme commented Jun 23, 2025

Could you confirm, that undefined is expected here and it's fine to not emit the events?
Best with a comment in the code explaining the case (it seems non-obious to me)

@Antreesy
Copy link
Contributor Author

Antreesy commented Jun 24, 2025

  1. Adjust NcPopover rendering logic
    1.1 Click should cause v-if condition to render a popover to change, and it's unmounted before apply-hide
  2. See result:
image

P.S. deferring unmount with nextTick also works, but I don't think it's the best way to control it

@Antreesy
Copy link
Contributor Author

Antreesy commented Jun 24, 2025

See reproduction details cc @susnux @ShGKme

Details

WIP error a <NcButton> as a trigger:

<template>
	<div style="display: flex">
		<NcButton @click="mountPopover = !mountPopover">Mount-unmount</NcButton>
		<NcPopover v-if="mountPopover" popup-role="dialog">
			<template #trigger>
				<NcButton>I am the trigger</NcButton>
			</template>
			<template #default>
				<form tabindex="0" role="dialog" aria-labelledby="popover-example-dialog-header-1" @submit.prevent>
					<h2 id="popover-example-dialog-header-1">this is some content</h2>
					<p>
						Lorem ipsum dolor sit amet, consectetur adipiscing elit. <br/>
						Vestibulum eget placerat velit.
					</p>
					<label>
						Label element
						<input type="text" placeholder="input element"/>
					</label>
				</form>
				<NcButton @click="mountPopover = !mountPopover">Mount-unmount</NcButton>
			</template>
		</NcPopover>
	</div>
</template>

<script>
	export default {
		data() {
			return {
				mountPopover: false,
			}
		},
	}
</script>

@Antreesy Antreesy force-pushed the fix/noid/ncpopover-listeners branch from 69f669a to bc02bc9 Compare June 24, 2025 10:24
@Antreesy Antreesy force-pushed the fix/noid/ncpopover-listeners branch from bc02bc9 to 3dc7f45 Compare June 24, 2025 11:42
@Antreesy Antreesy requested a review from DorraJaouad June 24, 2025 11:43
@ShGKme ShGKme changed the title fix(NcPopover): do not throw if element is not present fix(NcPopover): component crash when unmounted shown Jun 24, 2025
@ShGKme
Copy link
Contributor

ShGKme commented Jun 24, 2025

For the history: Vue 3 sets template ref to null before starting the unmounting, including beforeUnmount event.

https://github.com/vuejs/core/blob/v3.5.17/packages/runtime-core/src/renderer.ts#L2110

@Antreesy Antreesy merged commit f1e23d1 into main Jun 24, 2025
25 checks passed
@Antreesy Antreesy deleted the fix/noid/ncpopover-listeners branch June 24, 2025 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug Something isn't working feature: popover Related to the popovermenu component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants