Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove doubling proxied event handlers in NcRichContenteditable #3875

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Mar 9, 2023

A problem

All component's event handlers were set as native event handlers with by v-on="$listeners".
It allows using event handlers without the .native modifier for native events.

But the component also raised custom events manually by $emit, including corresponding native events. As a result, it triggers such handlers twice.

Related issue: nextcloud/spreed#9004

Steps to reproduce

  1. Set paste event handler:
    <NcRichContenteditable @paste="handler" />
  2. Paste anything

Actual behavior

handler is called twice:

  • Once handler will be called by $emit('paste')
  • One more time, handler will be called by v-on="$listeners".

Expected behavior

handler is called once.

Proposed solution

In this PR with the v-on="listeners" directive only proxied native event handlers are set, without custom events.

Alternative solution

There is a simple alternative solution: remove this.$emit('paste', event) from onPaste handler. It also removes the doubling paste event.

I suggest the solution from the PR because:

  • It doesn't change the component's interface (emits option) and behavior with conditions in onPaste.
  • It is scalable: it allows adding more custom processing to paste event handling as well as custom processing of other "native" events.

submit and update:value event handlers are not removed because they don't exist as native events on div.

@ShGKme ShGKme added bug Something isn't working feature: rich-contenteditable Related to the rich-contenteditable components labels Mar 9, 2023
@ShGKme ShGKme added this to the 7.8.1 milestone Mar 9, 2023
@ShGKme ShGKme added the 3. to review Waiting for reviews label Mar 10, 2023
@ShGKme ShGKme requested review from raimund-schluessler and removed request for ChristophWurst March 10, 2023 12:56
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Feels a bit hackish to me?
What about just stopping the propagation and preventing defaults on the @paste event?

@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 14, 2023

What about just stopping the propagation and preventing defaults on the @paste event?

The problem is not about the DOM event but the Vue component custom event handler.
The component receives a Vue custom event handler for the paste custom event.

<NcRichContenteditable @paste="customEventHandler" />

Then the component adds native DOM event handlers on the div:

<div @paste="onPaste" v-on="$listeners" />

Which is "equal" to

<div @paste="$emit('paste')" @paste="$listeners.paste()" />

So on the paste:

  1. $emit('paste') - calls the customEventHandler
  2. $listeners.paste()" - calls the customEventHandler

Feels a bit hackish to me?

Maybe 😀. I personally always use this approach on a wrapper-component if I need to patch specific native event handlers while proxying all handlers. It is always a problem if you combine v-on="$listeners" with custom events.

A simple alternative is to remove paste from emitted component events. But it's less flexible - no control over the event. Though, in this component, it won't break anything.

I'm also okay with removing $emit('paste').

P.S. This problem was solved in Vue 3. They removed $listeners and added the emits option to divide custom and native events.

@julien-nc julien-nc modified the milestones: 7.8.1, 7.8.2 Mar 16, 2023
@mejo- mejo- modified the milestones: 7.8.2, 7.8.3 Mar 22, 2023
@nickvergessen nickvergessen modified the milestones: 7.8.3, 7.8.4, 7.8.5, 7.8.6 Mar 23, 2023
@julien-nc julien-nc modified the milestones: 7.8.6, 7.9.1 Apr 7, 2023
@Pytal Pytal modified the milestones: 7.9.1, 7.11.0 Apr 18, 2023
@ShGKme
Copy link
Contributor Author

ShGKme commented Apr 27, 2023

@skjnldsv Hi, do you have any thoughts about this solution or an alternative way to solve duplicating native + vue events?

@skjnldsv
Copy link
Contributor

@skjnldsv Hi, do you have any thoughts about this solution or an alternative way to solve duplicating native + vue events?

With the amount of work I've spent on this component to try to mitigate and handle every use case, I'm too afraid to touch it. It's been a while, but I do remember the struggle back then. Sorry, but I don't have time right now to give a proper review and in-depth check 😞

@ShGKme ShGKme requested review from mejo- and julien-nc April 27, 2023 13:38
@ShGKme ShGKme requested a review from Pytal April 27, 2023 13:38
@artonge artonge modified the milestones: 7.11.0, 7.12.0 May 3, 2023
@nickvergessen nickvergessen modified the milestones: 7.12.0, 8.0.0 May 8, 2023
@ShGKme
Copy link
Contributor Author

ShGKme commented May 13, 2023

Just found that the same approach is described in the Vue 2 documentation:
So it is not so hackish, recommended and documented way to solve this problem.

https://v2.vuejs.org/v2/guide/components-custom-events.html#Binding-Native-Events-to-Components

The only difference is that in the docs computed does not remove, but replaces listeners. While in this PR computed removes listeners that are replaced in the template. The result is the same, but replacing the listeners in the computed is fine by me either.

It allows removing timestamp check on path event handling: https://github.com/nextcloud/spreed/blob/cd0a971a2bdad0c8f5a177e958f0ebe99d33d505/src/components/NewMessage/NewMessage.vue#L596-L603

@ShGKme ShGKme force-pushed the fix/rich-contenteditable-fix-doubled-past-event branch from b0dfd3c to 4baee0c Compare June 30, 2023 13:35
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 30, 2023

Rebased onto master

Co-authored-by: Pytal <24800714+Pytal@users.noreply.github.com>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/rich-contenteditable-fix-doubled-past-event branch from 3752706 to 9bdba1b Compare June 30, 2023 19:31
@ShGKme ShGKme merged commit 687e1f8 into master Jun 30, 2023
16 checks passed
@ShGKme ShGKme deleted the fix/rich-contenteditable-fix-doubled-past-event branch June 30, 2023 19:34
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 30, 2023

/backport to stable7

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: rich-contenteditable Related to the rich-contenteditable components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants