Components: Stop matching autocompleter upon mismatch #30649
Conversation
|
Size Change: +85 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
|
Thanks for taking this on! Unfortunately, it seems that with this PR, the suggestions box doesn't always appear for me when typing e.g. Furthermore, one of the autocomplete related e2e tests is currently failing: BTW maybe we can avoid introducing |
Thanks for having a look! That's true, I managed to get into a state where the box isn't shown consistently :(. I'll spend a bit more time today seeing if I can fix this. |
fullofcaffeine
force-pushed
the
try/fix-autocomplete-infinite-match
branch
from
37d3e10
to
d39bf7d
Compare
fullofcaffeine
force-pushed
the
try/fix-autocomplete-infinite-match
branch
2 times, most recently
from
77a59d1
to
0d8f0af
Compare
gziolo
added
[Package] Components
fullofcaffeine
changed the title
fullofcaffeine
requested review from
ajitbohra and
chrisvanpatten
as code owners
fullofcaffeine
changed the title
…he whole match, as it is split by space
When backspacing, we were only considering a single word, which was less than ideal and would not trigger the effect if the user was in the middle of fixing a completion for a two-word name for example.
fullofcaffeine
force-pushed
the
try/fix-autocomplete-infinite-match
branch
from
2d020af
to
8a38bcb
Compare
There was a problem hiding this comment.
Thanks @fullofcaffeine, I'm giving tentative approval. We'll want a +1 for which path to take, as I haven't interacted much with this code area before. Either we take this patch and follow up on the approach, or perhaps consider reverting 29939 or making it exclusive to the block inserter auto-complete as @mtias suggested.
Overall, I think this works well as an interim patch to address the performance issue added in #29939. The approach of exiting early when potential completion text is too long is easy to reason about.
If we want to test other approaches in the future, I'd highly recommend adding unit tests (but not component tests). It might take a bit of refactoring, but if we could isolate the completer function, being able to pass in some content, and have it return whether or not it's a match will help document all the cases we want to keep, and make it easier to try other approaches to speeding this up.
We already have some basic E2E coverage. I don't think we need more unless specific things keep breaking that unit tests cannot catch.
Some quick 🤔 ideas to experiment with in follow-ups:
- We're currently running this on every keypress. When a user is typing rapidly we probably want to get out of the way. I suspect folks will slow down on typing speed when trying to autocomplete. Tuning a debounce would likely make things feel more lively.
- In a similar vein should we store more state to avoid running this whenever content changes? (Memo, detect a trigger, stop when a substring has no matches etc). The tradeoff here is that the more complex our approach is, the more likely we'll be wrong. We'll also want to be careful about making something slower accidently so we'll need to perf profile whatever we experiment with.
| // significantly. This could happen, for example, if `matchingWhileBackspacing` | ||
| // is true and one of the "words" end up being too long. If that's the case, | ||
| // it will be caught by this guard. | ||
| if ( tooDistantFromTrigger ) return false; |
There was a problem hiding this comment.
From WP guidelines
if/else/for/while/try blocks should always use braces
This is more of a guard against future changes. Basic idea on the why we prefer braces is that it's easy for folks to accidentally add a line and think it's part of the if block, when it's not.
I'm surprised the linter didn't autofix this.
There was a problem hiding this comment.
Ok, thanks for the nudge!
There was a problem hiding this comment.
It's controlled by Prettier, we can't do much about it. It has to stay as is.
There was a problem hiding this comment.
Interesting, this option is a won't fix too: prettier/prettier#4262
Do we think code style guideline docs could be simplified to lint checks pass (in pretter / phpcs)?
There was a problem hiding this comment.
I'm surprised the linter didn't autofix this.
All ESLint rules that conflict with Prettier formatting are disabled by the config they have for ESLint.
PHP and JS coding guidelines are different. I see that JS coding standards need to clarify that bit in https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/#blocks-and-curly-braces
There was a problem hiding this comment.
I added this topic to the Core JS weekly chat next week: https://docs.google.com/document/d/1Y2Q1iZ89M88NA3iq0TyLzWn9wy6Zxob-wX3tJ4ivHPc/edit#
| index + triggerPrefix.length | ||
| ); | ||
|
|
||
| const tooDistantFromTrigger = textWithoutTrigger.length > 50; // 50 chars seems to be a good limit. |
There was a problem hiding this comment.
For folks reviewing the trigger is a key like / or @ and textWithoutTrigger are all characters after that. So for something like @someusernamewithfiftycharacters it will still properly early return after 50 chars even if folks don't enter a space.
If we're picking something arbitrary, max user_login length is 60 characters. I don't feel strongly on this one.
There was a problem hiding this comment.
Yeah, it was arbitrary. I can adjust it to 60 if we think it's better. I don't feel too happy about this guard, but it's needed for the case described as long as we use this early return approach.
There was a problem hiding this comment.
I'm okay either way on this one 👍
| // Ex: "Some text @marcelo sekkkk" <--- "kkkk" caused a mismatch, but | ||
| // if the user presses backspace here, it will show the completion popup again. | ||
| const matchingWhileBackspacing = | ||
| backspacing && textWithoutTrigger.split( /\s/ ).length <= 3; |
There was a problem hiding this comment.
As suggested by @gwwar it's not very clear what these checks do, so yeah extracting the function + unit tests sounds like a great approach.
The release need to happen soon, I might merge as is though.
|
I would personally go with the bug fix and observe how it works. We need to make this autocomplete feature useful with all the block patterns and variations that are hard to match with only one keyword. The approach proposed is close to what was discussed in other issues/PRs on GitHub. We definitely should have another iteration before WP 5.8 to improve it further. |
|
@fullofcaffeine, thank you for all your work to improve the autocomplete feature. Are you going to work on a follow-up or should we file an issue with all details? |
Thanks! ❤️ No problem! Glad I could help! :)
I don't plan on working on a follow-up just now, I'll let it cool-down for a while, and might resume it in the future if someone else more experienced with the code-base doesn't beat me to it. If you could create a follow-up issue, it'd be great! cc @gwwar |
|
Added a follow up issue in #30969 |
Attempts to fix #30640.
How to test
wp-envinstance in this branch, load the editor by creating a new post or page, open the JS console to check its output;/in a new paragraph). It should work as expected and match blocks with spaces, too.