Fix newlines in block editor after a user mention #41749
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chad1008
added
[Type] Bug
|
Size Change: +10 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
mirka
reviewed
Jun 16, 2022
There was a problem hiding this comment.
In testing, I'm still seeing some occasional glitches 😢 Not only the newline issues, but also the suggestions popover not appearing when it should.
From what I could tell, these changes from #41382 are also problematic. If you console.log in that useEffect, you can tell that the frequency of it firing has greatly increased (while typing in the Paragraph block). And the number of uncaught promise errors have gone up as well. (A small number of those were pre-existing, so there's probably also a pre-existing race bug somewhere.)
Do you want to try reworking that useEffect so we can keep the previous behavior (i.e. only fire on textContent changes) at least? We could also revert #41382 in the meantime, if it isn't a quick fix.
Comment on lines
+217
to
+226
| const onChangeOptions = useCallback( | ||
| ( options ) => { | ||
| setSelectedIndex( | ||
| options.length === filteredOptions.length ? selectedIndex : 0 | ||
| ); | ||
| setFilteredOptions( options ); | ||
| announce( options ); | ||
| }, | ||
| [ announce, filteredOptions.length, selectedIndex ] | ||
| ); |
There was a problem hiding this comment.
So... I'm starting to think about when it's better to use a useCallback vs. putting the callback in a ref.
In this case, the ref approach would be to change this
gutenberg/packages/components/src/autocomplete/autocompleter-ui.js
Lines 40 to 42 in 982cfc7
to something like this:
const onChangeOptionsRef = useRef( onChangeOptions );
onChangeOptionsRef.current = onChangeOptions;
useLayoutEffect( () => {
onChangeOptionsRef.current( items );
}, [ items ] );The reason I think this approach is preferable here is because, it should be AutocompleterUI's responsibility that its own effects work correctly. If we instead address it with a useCallback in the consumer, then all potential consumers will need to do the same thing, which is hard to know about.
I might be missing something though. What do you think?
|
I'm going to review this later today, but here's also an alternative plan, in case we want to fix ASAP the regression explained in this PR (and in #41770):
This approach could take some pressure away from landing these 2 bug fixes quickly. What do folks think? |
|
Closing, as the regression this was meant to address has been reverted: #41820 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fix #41724
Memoize
onChangeOptionsand subsequent dependencies.Why?
When
onChangeOptionswas added as auseLayoutEffectdependency in #41382, new lines after user mentions became very unreliable.I think (not 100% sure, but it bears out in my testing) that a race condition was introduced. When the new dep was placed, the effect began firing on every render because
onChangeOptionswas actually being redeclared each time. This effect firing would in turn call two state setters (setSelectedIndexandsetFilteredOptions). If either of these was updated with a new value, a new render would trigger, repeating the cycle. It wasn't quite an infinite loop, so no errors appeared, but it did create frequent cases where theuseEffectresponsible for callingreset()would be prevented from firing.My working theory is that because
useLayoutEffectfires before the render completes, the state was changing before thatresetfunction could do its job and clear the stored autocomplete input... so whenEnter/Returnwas pressed, the component inserted the value it had previously been asked to insert from the completion popover.How?
Since the problem appeared to stem from firing that
useLayoutEffecttoo often, we can wraponChangeOptionsin auseCallback. The function itself shouldn't change, so effectively I expect this effect to only fire on changes toitems.This, in turn, required
announceto get a similaruseCallbackwrapper.onChangeOptionsshould now only actually fire when there's a change to either the length of thefilterOptionsarray, or theselectedIndex(which should be exactly what we want).Testing Instructions
@followed by part of another user's name. Select them withReturn/Enterspaceand enter a few characters of textReturn/EnterAt least some (probably most if not all) of the time, your followup text should be erased by the autocompleter.
After applying this PR, repeat the same steps. You should get a new paragraph block when hitting
Return/Entereach time.