fix bug: decorate is not called for immediate children of editor

[jaked]

Description
The combination of #4152 and #4138 broke decoration in Editable: the decorate function is not called on immediate children of the editor (because the function is retrieved from a React context before the context is set).

This change fixes the bug by adding a wrapper component around each child in useChildren, so the context is set by the time we retrieve the decorate function.

Issue
Fixes: #4277

Example
A decorate function passed to Editable is called on the immediate children of the editor.

Context
As above, the issue is that a React context (added in #4138) is used before being set (because #4152 replaced the Children component with a call to useChildren). In order to fix the bug without defeating the goals of these two changes, we wrap a Child component around each child to defer the use of the context.

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)
• You've added a changeset if changing functionality. (Add one with yarn changeset add.)

[changeset-bot]

🦋 Changeset detected

Latest commit: 8a623a8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate-react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dylans]

This change looks reasonable to me (not that my opinion matters much).

[timbuckley]

Excellent, we badly needed testing for slate-react

[timbuckley]

I'm not too familiar with jsdom testing, but is there any way we could extract this global setting from this one suite, so it's done for all suites (when they exist)? Or must it be done a per-suite basis?

In fact, could you put the whole decorate test in a decorate.spec.ts or similar file? The index could just be common utilities among tests for example.

[jaked]

I mostly use Jest which has special support for jsdom https://jestjs.io/docs/configuration#testenvironment-string. I didn't find an equivalent for Mocha but there is https://github.com/rstacruz/jsdom-global which does pretty much what I do here.

I think it could be done for the whole test file, but I didn't want to presume. I figured someone with more Mocha experience would have opinions about how to set things up.

[dylans]

I would suggest we land this as it's an improvement all around, and then we can do a separate PR to look at extracting into a global setting.

[jaked]

thanks for the review @timbuckley !

[timbuckley]

Looks good to me!

[ulion]

why wrap the Child, not the Children? Child is indeed Element or Text, shouldn't just restore to old Children implement? or Children tag, since it may gain some performance if Children not updated, while Child does nothing since it's indeed Element or Text?

[jaked]

why wrap the Child, not the Children? Child is indeed Element or Text, shouldn't just restore to old Children implement? or Children tag, since it may gain some performance if Children not updated, while Child does nothing since it's indeed Element or Text?

The point is to preserve the list of children, to avoid undoing #4152. Child doesn't do nothing; it defers the useDecorate() call until after the decorate context is set in Editable.

Another way to fix the issue would be to leave useChildren alone but wrap a Children component around useChildren in Editable only (but not in Element). This would work because the bug only arises for the immediate children of Editable (in Element the decorate context is already set); but #4152 only needs the list of children preserved for Element, not for Editable (since Editable's children are not passed to renderElement).

I suggested this approach in #4277 (comment) but you didn't like it 😀.

[ulion]

I'm curious, why to avoid undo #4152? I don't like #4152 indeed, to support some abnormal use of React.Children, and lost performance for other cases. I would vote to revert it if possible. If do not want to revert it, but need fix the useDecorate error for the top Children env, just do the other dummy Tag wrapper between the context provider and its children, you don't need add the common Child wrapper which hurt performance anyway. slate may have better performance before these changes, hope you do some performance test for such render level / structure change. since as a serious user for performance for large collaborate document, I really need that. Jake Donham ***@***.***> 于2021年8月7日周六 上午2:00写道：

…

why wrap the Child, not the Children? Child is indeed Element or Text, shouldn't just restore to old Children implement? or Children tag, since it may gain some performance if Children not updated, while Child does nothing since it's indeed Element or Text? The point is to preserve the list of children, to avoid undoing #4152 <#4152>. Child doesn't do nothing; it defers the useDecorate() call until after the decorate context is set in Editable. Another way to fix the issue would be to leave useChildren alone but wrap a Children component around useChildren in Editable only (but not in Element). This would work because the bug only arises for the immediate children of Editable (in Element the decorate context is already set); but #4152 <#4152> only needs the list of children preserved for Element, not for Editable (since Editable's children are not passed to renderElement). I suggested this approach in #4277 (comment) <#4277 (comment)> but you didn't like it 😀. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4394 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKYHMWZIDT4TKDNKUXGTLT3QPK5ANCNFSM5BBCDN3Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[jaked]

I'm curious, why to avoid undo #4152? I don't like #4152 indeed, to support some abnormal use of React.Children, and lost performance for other cases. I would vote to revert it if possible.

OK, I'm just a contributor trying to fix a bug that's affecting my app without breaking other people's stuff. If you think #4152 should be reverted you should take it up with the author / committer.

If do not want to revert it, but need fix the useDecorate error for the top Children env, just do the other dummy Tag wrapper between the context provider and its children, you don't need add the common Child wrapper which hurt performance anyway.

I don't understand how the current approach hurts performance; I chose this path because it seemed just the same as the original code (pre-#4152) as far as rendering goes.

But I find it difficult to understand how rendering will behave from reading the code. If you have some insight (or better yet a way to test rendering performance) I would appreciate it.

[ulion]

Add useless wrapper Tag will add unnecessary extra layer of the render tree, clearly you only need to fix the top Children, not every Child, but you add that Tag so it hurt performance and unnecessary. Jake Donham ***@***.***> 于2021年8月7日周六 上午10:27写道：

…

I'm curious, why to avoid undo #4152 <#4152>? I don't like #4152 <#4152> indeed, to support some abnormal use of React.Children, and lost performance for other cases. I would vote to revert it if possible. OK, I'm just a contributor trying to fix a bug that's affecting my app without breaking other people's stuff. If you think #4152 <#4152> should be reverted you should take it up with the author / committer. If do not want to revert it, but need fix the useDecorate error for the top Children env, just do the other dummy Tag wrapper between the context provider and its children, you don't need add the common Child wrapper which hurt performance anyway. I don't understand how the current approach hurts performance; I chose this path because it seemed just the same as the original code (pre-#4152 <#4152>) as far as rendering goes. But I find it difficult to understand how rendering will behave from reading the code. If you have some insight (or better yet a way to test rendering performance) I would appreciate it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4394 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKYHKMR2OCVL6Z75SHXWDT3SKYZANCNFSM5BBCDN3Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[jaked]

Add useless wrapper Tag will add unnecessary extra layer of the render tree, clearly you only need to fix the top Children, not every Child, but you add that Tag so it hurt performance and unnecessary.

In your earlier comment you seemed to object to this approach:

only adding a wrapper for editor.children does not mean
anything, since useDecorate no longer reduces re-render like before since
the Children element is gone, keeping it there but not functional makes no
sense.

I'm going to make another pull request to wrap only the top-level useChildren, so there won't be extra unnecessary layers of the render tree. If you're still not happy then you should make your own pull request.

[dylans]

@ulion as I'm sure you're aware, rich text editing is one of the most maddening things to get right in a browser. We appreciate your help and contributions.

As far as I know, we do not have automated performance tests. And until yesterday when we landed this PR from @jaked , we had zero automated tests for slate-react.

As a project, we can either be so cautious that we're afraid to commit any changes, or we can do our best to review and understand things and accept that we're sometimes going to have regressions that we can fix quickly if we have enough people looking at the codebase and giving quick feedback.

I tend to prefer the latter, but it requires tolerance for short-term imperfection with the benefit of making progress more quickly.

We're all trying to build the best editor and apps on top of the editor that we can. I certainly appreciate the help and I'm doing what I can to get the pile of stale PRs unstuck as there are a lot of things I personally would benefit from having fixed in Slate.

Editors are so involved that it's difficult to keep all the details in a single brain at one time (or at least my brain struggles, maybe others do not), so let's do what we can to support and help each other on this journey.

Let me know if there's anything I can do differently or better to help.

[ulion]

Add useless wrapper Tag will add unnecessary extra layer of the render tree, clearly you only need to fix the top Children, not every Child, but you add that Tag so it hurt performance and unnecessary.

In your earlier comment you seemed to object to this approach:

only adding a wrapper for editor.children does not mean
anything, since useDecorate no longer reduces re-render like before since
the Children element is gone, keeping it there but not functional makes no
sense.

I'm going to make another pull request to wrap only the top-level useChildren, so there won't be extra unnecessary layers of the render tree. If you're still not happy then you should make your own pull request.

Thank you, that would be great. introduce #4152 break the useDecorate and indeed make useDecorate useless (the decorate func was transferred from Editable to Children then to Element directly before the useDecorate PR), the useDecorate PR keep Children from re-render if decorate func changes, that's why useDecorate was introduced. but since Children is removed, which make using useDecorate indeed useless. To fix the bug case, just fix the top Children as #4277 would do. But for the final performance, someone need to decide whether totally remove useDecorate (since it's useless anyway with no Children Tag), or call Children back.

[dylans]

@ulion #4421 addresses your feedback, it would be great if you have time to review. Thanks.