Skip to content

Conversation

@skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Apr 2, 2025

NcAcions

Following #6065

The evets are after-hide and after-show, not apply-hide and apply-show
https://github.com/nextcloud/nextcloud-vue/blob/ae77137aee5097dcbe8cef6c53eb1ce3bc81c7ee/src/components/NcPopover/NcPopover.vue#L230-L237

NcPopover

The vue-floating lib does not properly compute how long it needs to wait to know the popup have been shown/hidden.
I investigated and it's only using requestAnimationFrame which in that case isn't enough to ensure its completion.
Because we have our own custom animation variables, we need to manually check for it ourselves.

Tested on Server with log entries to see the duration (we use --animation-quick which is 100ms for us)

Before After
image 2025-04-02_10-23

@skjnldsv skjnldsv added bug Something isn't working 3. to review Waiting for reviews feature: actions Related to the actions components labels Apr 2, 2025
@skjnldsv skjnldsv requested review from ShGKme and susnux April 2, 2025 08:25
@skjnldsv skjnldsv self-assigned this Apr 2, 2025
* Called when popover is shown after the show delay
*/
onOpen() {
onOpened() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only renamed for consistency with the onClosed method name

@skjnldsv skjnldsv force-pushed the fix/actions-events branch from ed4b48d to 8c997ce Compare April 2, 2025 08:33
@skjnldsv skjnldsv enabled auto-merge April 2, 2025 08:45
await this.useFocusTrap()
this.addEscapeStopPropagation()
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of reading a CSS variable and adding a new task with timeout, can we use a native event here?

this.getPopoverContentElement().addEventListener('transitionend', () => {
	this.$emit('after-show')
}, { once: true })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it yeah! Didn't realized transitionend was an option

*
* This event is emitted after `update:open` was emitted and the opening transition finished.
*/
this.$emit('opened')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature not fix (new event)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for NcPopover, feat for NcActions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the fix in ncpopover is unrelated to the feature for NcActions.
So this is semantically a feature PR ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, was missing 🤷

@ShGKme ShGKme added enhancement New feature or request and removed bug Something isn't working labels Apr 2, 2025
@ShGKme
Copy link
Contributor

ShGKme commented Apr 2, 2025

/backport to next

on: {
show: this.openMenu,
'apply-show': this.onOpen,
'after-show': this.onOpened,
Copy link
Contributor

@susnux susnux Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will now run quite late.
after-show will already await the next tick and the focus trap, so the additional tick in onOpened will cause a 1 tick delay which seems to not be needed. Probably the nextTick in onOpened can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to stay consistent. I'd like to avoid to have three different timings to watch for.
Two are perfectly fine:

  1. we start closing
  2. we are sure everything is finished and closed/hidden

@susnux susnux changed the title fix: [NcActions] listening to wrong event names / [NcPopover] animation wait feat(NcActions): fix listening to wrong event names and add opened event / [NcPopover] animation wait Apr 2, 2025
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the additional tick we now have, but in general makes sense

@skjnldsv skjnldsv merged commit efd6f0d into master Apr 2, 2025
23 checks passed
@skjnldsv skjnldsv deleted the fix/actions-events branch April 2, 2025 10:05
@skjnldsv
Copy link
Contributor Author

skjnldsv commented Apr 2, 2025

Instead of reading a CSS variable and adding a new task with timeout, can we use a native event here?

this.getPopoverContentElement().addEventListener('transitionend', () => {
	this.$emit('after-show')
}, { once: true })

Was still testing this, let me push a followup if it works ⌛
I should have disabled auto merge, my bad

@ShGKme ShGKme changed the title feat(NcActions): fix listening to wrong event names and add opened event / [NcPopover] animation wait feat(NcActions): add opened and closed events to handle closing/opening end | fix(NcPopover): correctly wait for animation end in after-show/after-hide events Apr 2, 2025
@ShGKme
Copy link
Contributor

ShGKme commented Apr 2, 2025

A bit renamed for change log generation

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Apr 2, 2025

@ShGKme tested addEventListener('transitionend' a bit, and it is slower than a simple setTimeout. Weirdly waiting much longer (50ms difference)

setTimeout transitionend
2025-04-02_10-23 image

@skjnldsv
Copy link
Contributor Author

skjnldsv commented Apr 2, 2025

Doesn't matter, much cleaner anyway
#6687

@ShGKme
Copy link
Contributor

ShGKme commented Apr 2, 2025

@skjnldsv On my test:

afterHide | 10967
transitionstart | 10992
afterHide:setTimeout | 11072
transitionend | 11085

So it's 13ms longer, than timeout, but the animation might also have started later, than we think in the timeout

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 enhancement New feature or request feature: actions Related to the actions components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants