Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix autocomplete trigger character detection #55301

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

WunderBart
Copy link
Member

@WunderBart WunderBart commented Oct 12, 2023

What?

Fix trigger character detection when using multiple different triggers inline.

before after
Screen.Recording.2023-10-12.at.17.54.50.mov
Screen.Recording.2023-10-12.at.17.51.44.mov

Closes #55118

How?

The function that finds a completer for given text content performs multiple checks before returning a matching completer. The main issue here is that there are multiple trigger characters, and if they're used side by side, the first one that matches all the criteria is returned. For the given example above, the completers are toggled between the one for @ and the one for + triggers as we type, that's why the popup sometimes disappears, as the matching string is different on each keystroke.
We should not be relying on the order of the triggers array, so to address this, I've added a step where the completer is picked by the closest trigger character to the end of the current text content. After that, all the checks that were there before are performed as well to ensure there are no regressions.

Testing Instructions

Follow the steps to reproduce what has been recorded on the before and after videos above.

  1. checkout trunk and follow the steps described in Mentions autocomplete is unstable #55118 to see the regression.
  2. checkout this branch,
  3. create a new post,
  4. start typing a mention, trigger with @,
  5. hit enter to apply the found mention,
  6. hit space and start typing another mention; this time start with +,
  7. type at least 4 characters of the searched mention,
  8. ensure the results popup is visible on each keystroke.

All the E2E tests should pass.

@WunderBart WunderBart self-assigned this Oct 12, 2023
@github-actions
Copy link

github-actions bot commented Oct 12, 2023

Flaky tests detected in df24fddcdd4300df3669a29204c3b9e397250054.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6498655061
📝 Reported issues:

@WunderBart WunderBart marked this pull request as ready for review October 12, 2023 16:44
@WunderBart WunderBart added the [Type] Bug An existing feature does not function as intended label Oct 12, 2023
@WunderBart WunderBart force-pushed the fix/autocomplete-mentions-stability branch from df24fdd to 301f294 Compare October 12, 2023 19:30
@WunderBart WunderBart force-pushed the fix/autocomplete-mentions-stability branch from 301f294 to 58fb334 Compare October 12, 2023 19:32
@chad1008
Copy link
Contributor

@WunderBart, how are you accessing the + completer for these testing steps? I've been using the localhost:8889 instance from wp-env and activating the Gutenberg Test Autocompleter plugin that creates that completer for the e2e tests that helped reveal the instability. From your testing instructions it sounds like you might be doing it differently?

@WunderBart
Copy link
Member Author

WunderBart commented Oct 16, 2023

@WunderBart, how are you accessing the + completer for these testing steps? I've been using the localhost:8889 instance from wp-env and activating the Gutenberg Test Autocompleter plugin that creates that completer for the e2e tests that helped reveal the instability. From your testing instructions it sounds like you might be doing it differently?

Ah, good question! Here's what I do to use the env from that autocomplete test:

  1. Add await page.pause() right after the test env setup is done, which in this case should be here.

  2. Run that test in headed mode so that it's actually paused:

    npm run test:e2e:playwright -- -g "should insert elements from multiple completers" --headed
    
  3. Wait until the browser opens and the execution is paused.

  4. Go to http://localhost:8889/wp-admin/post-new.php in your browser or Playwright's instance (should be already there when paused).

  5. You should now be able to use the autocomplete data created for that test.

  6. When done testing manually, unpause or close the Playwright test runner.

Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for those steps @WunderBart, I wanted to make sure we were testing the same things! I was actually noticing another odd behavior, but it doesn't look related to this PR at all:

When I first load the editor and start enter a user mention, the autocomplete options appear to re-render on each character, but only for the first time I try it. All subsequent uses of Autocomplete render once and just filter out options as I go. This looks like it happens on trunk and with either of the testing methods I used though, so again, I think it's completely unrelated.

Testing this PR using both your steps and my own approach to activating the secondary + completer, this feels solid to me. The second completer consistently renders the options without dropping them on every other character like trunk currently does, and if I log out the currently selected completer as I go everything looks good 👍

@WunderBart WunderBart merged commit fba216c into trunk Oct 19, 2023
50 checks passed
@WunderBart WunderBart deleted the fix/autocomplete-mentions-stability branch October 19, 2023 13:53
@github-actions github-actions bot added this to the Gutenberg 17.0 milestone Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
2 participants