getAutocompleterUI: don't redefine ListBox component on every render #61877
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
jsnajdr
added
[Type] Bug
ellatrix
approved these changes
May 22, 2024
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @17cliu. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ellatrix
reviewed
May 22, 2024
| ) ) } | ||
| </Component> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Do we need it to be inside of getAutoCompleterUI?
There was a problem hiding this comment.
It can be outside! Moved 🙂
|
Flaky tests detected in d166614. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9203507985
|
jsnajdr
force-pushed
the
update/autocompleter-listbox
branch
from
e0eed38 to
d166614
Compare
Good idea, added a test that autocompletes inside a table. Verified that it fails on trunk and succeeds after the fix. I think I addressed all feedback, going to merge now. |
|
Size Change: +35 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
tyxla
approved these changes
May 23, 2024
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jun 4, 2024
…ordPress#61877) * getAutocompleterUI: don't redefine ListBox component on every render * Move ListBox to top level * Add components changelog entry * Add e2e test for autocomplete inside table Unlinked contributors: 17cliu. Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: alexstine <alexstine@git.wordpress.org> Co-authored-by: johngodley <johnny5@git.wordpress.org>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…ordPress#61877) * getAutocompleterUI: don't redefine ListBox component on every render * Move ListBox to top level * Add components changelog entry * Add e2e test for autocomplete inside table Unlinked contributors: 17cliu. Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: alexstine <alexstine@git.wordpress.org> Co-authored-by: johngodley <johnny5@git.wordpress.org>
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.
Fixes #50562. The
AutocompleterUIcomponent defined a nestedListBoxcomponent inside its render function, but this is an antipattern! TheListBoxcomponent has different identity on every render, and that causes it to completely unmount and remount on every render.One of the symptoms is that the
onClickhandler on theButtonsubcomponents in theListBoxis never called when the autocompleter is inside the table. Don't ask me why 🙂 There is probably some chaotic unstable rerendering that destroys theonClickhandler before it gets a chance to be called.Another symptom that you can easily reproduce is that the gravatar image on the user autocomplete is blinking as you type. Because the items are not rendered in a stable way, the
imgelement gets repeatedly destroyed and created and loaded again.How to test:
Verify that the symptoms reported in #50562 (user autocomplete inside table cell) are fixed. Also, the gravatar images should be stabilized.