Fix onInput events inside decorator nodes

[Piliuta]

Problem

Input elements inside decorator nodes do not receive input events because lexical stops propagation in the parent editor.

Solution

Check if the target element of the input event is from a decorator node, if so - do not stop propagation of the event.

Related conversation in discord: https://discord.com/channels/953974421008293909/1006976156748222464/1007339073683337327

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
lexical	✅ Ready (Inspect)	Visit Preview	Aug 12, 2022 at 9:57AM (UTC)
lexical-playground	✅ Ready (Inspect)	Visit Preview	Aug 12, 2022 at 9:57AM (UTC)

[Piliuta]

I do not know what was the driver for adding this originally, but if nested editors can be only inside decorator nodes, then the proposed change is going to cross out this functionality entirely.

Does it need an additional check if the target node is inside a lexical editor instance?

[Piliuta]

4b72ae0#diff-e3254645cf320ff0eefbbf117e94d636c4e6cea5d293e7d095328c8a9a7e2fefR976-R977

[echarles]

On https://developer.mozilla.org/en-US/docs/Web/API/Element/closest The closest() method of the Element interface traverses the element and its parents (heading toward the document root) until it finds a node that matches the specified CSS selector.

Is there a risk to impact the performance, as on each event, there could be a full tree traversal to the top?

[zurfyx]

Thank you! Would you mind adding a bit more context on your use case and how the current stopPropagation blocks you?

AFAIK this should prevent bubbling but you can still handle whatever you need within your own component. If you need to pass any data outside the editor you can leverage a React Context to send and receive information (inc. your own custom event subscriptions) or commands to certain extend

[Piliuta]

Thank you! Would you mind adding a bit more context on your use case and how the current stopPropagation blocks you?

AFAIK this should prevent bubbling but you can still handle whatever you need within your own component. If you need to pass any data outside the editor you can leverage a React Context to send and receive information (inc. your own custom event subscriptions) or commands to certain extend

My component is listening for user input changes and depending on the value converts it to a different thing. All this happens internally inside the component (It doesn't send anything outside the editor).
More specifically, imagine a field that recognizes urls and converts them into a preview. (The field has its own view and that's why it is not a part of the root editor flow and is based off the decorator node, i.e. it is not the same as the emoji conversion plugin)

Because lexical stops input event propagation my component does not receive the event.

[acywatson]

Thank you! Would you mind adding a bit more context on your use case and how the current stopPropagation blocks you?

AFAIK this should prevent bubbling but you can still handle whatever you need within your own component. If you need to pass any data outside the editor you can leverage a React Context to send and receive information (inc. your own custom event subscriptions) or commands to certain extend

@zurfyx see Discord thread: https://discord.com/channels/953974421008293909/1006976156748222464/1007575453076299797

[Piliuta]

@zurfyx does this PR need anything else to be able to move this forward?

[acywatson]

@zurfyx does this PR need anything else to be able to move this forward?

Thinking about @echarles concern, it does seem less-than-ideal to run this check on every single input event. I'm wondering if we can just get rid of this stopPropagation call. I'm looking into whether it makes sense to have nested editors outside of DecoratorNodes. I think that it's possible - for instance, we've discussed using nested editors for table cells.

Give me and @zurfyx a bit more time on this, if you don't mind. Sorry for the delay.

[fantactuka]

I've tried to test this with PollNode which also has <input> and added onInput for it, but even if completely removed stopPropagation, it still didn't get onInput called. Wondering if you had similar setup to test it out?

[Piliuta]

I've tried to test this with PollNode which also has <input> and added onInput for it, but even if completely removed stopPropagation, it still didn't get onInput called. Wondering if you had similar setup to test it out?

I have a custom component with a contenteditable field. Removing or wrapping stop propagation makes oninput callback work for me.

Regarding the performance concern: I do not think this would cause any big issues as the frequency of the events is not that tense, however, I do agree that this is "less-than-ideal" and avoiding having the condition (and removing stop propagation) would be preferable but because I am not familiar with the code and do not know the consequences, I cannot insist on this change. (Though I was trying to dig up why it was added originally, and found that it was added here and perhaps @trueadm who added it originally has more insights)

[Piliuta]

p.s. @acywatson I guess it was not intentionally commented out and merged into the main branch 3f339a7#diff-13ea778ff524191baf03fce4f1a86503d50f068ef5e14bd4e064a1fd69fcf054R593

[fantactuka]

I'm still struggling to reproduce the issue, e.g. I've added content editable into <PollComponent> component in a playground:

<div contentEditable={true} onInput={console.log} style={{border: '1px solid red'}}>
  This is contenteditable
</div>

And it gets input events just fine. Is it different from what you have?

Screen.Recording.2022-08-18.at.10.35.01.AM.mov

[acywatson]

p.s. @acywatson I guess it was not intentionally commented out and merged into the main branch 3f339a7#diff-13ea778ff524191baf03fce4f1a86503d50f068ef5e14bd4e064a1fd69fcf054R593

Whoops - I was testing locally on my typedoc branch lol

[Piliuta]

I'm still struggling to reproduce the issue, e.g. I've added content editable into <PollComponent> component in a playground:

<div contentEditable={true} onInput={console.log} style={{border: '1px solid red'}}>
  This is contenteditable
</div>

And it gets input events just fine. Is it different from what you have?

Screen.Recording.2022-08-18.at.10.35.01.AM.mov

It works for me the way you add it. My case though much more complicated. I am trying to wrap a legacy component into the decorator node, where the component is something weirdly nested as [react [backbone [react:contenteditable]]] which apparently somehow changes the order of events propagation and the parent callback (in the lexical editor) is triggered first.

Also, I am on React v16.14.0, and I know React 17 handles events differently, so it can be another reason why it is not reproducible on the playground.

If this is caused by Backbone.js, then I guess this is not a common use case and might not be worse to make a fix for it.

I am going to investigate it on my side and perhaps it is going to be easier cleaner to just update my code.

I'll update here once I have more information. Thank you.

[Piliuta]

I upgraded my codebase to use React v17.0.2 and now this is no longer an issue. So apparently this was caused by React.

I am going to close this PR unless you think this still needs to be fixed for those who have earlier React versions.

Thanks for reviewing the PR and helping with it.

[echarles]

I upgraded my codebase to use React v17.0.2 and now this is no longer an issue. So apparently this was caused by React.

Great to read that.

p.s. @acywatson I guess it was not intentionally commented out and merged into the main branch 3f339a7#diff-13ea778ff524191baf03fce4f1a86503d50f068ef5e14bd4e064a1fd69fcf054R593
Whoops - I was testing locally on my typedoc branch lol

Does that change need to be reverted?

[trueadm]

We don’t actually support earlier versions of React than 17. We should document this somewhere and revert the change

[acywatson]

We don’t actually support earlier versions of React than 17. We should document this somewhere and revert the change

Change was reverted several days ago: #2849

PR welcome to update the docs re: React support version.

Thanks all!