<InlineAutocomplete>: Add onSelectSuggestion() callback to <InlineAutocomplete /> to allow execution of additional code after an autocomplete item was selected.
#3647
Conversation
🦋 Changeset detectedLatest commit: a5fc2a9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
paxos
changed the title
paxos
changed the title
<InlineAutocomplete>: Add onSelectSuggestion() callback to <InlineAutocomplete /> to allow execution of additional code after an autocomplete item was selected.
There was a problem hiding this comment.
I think this is a good idea and will unlock some useful new design patterns. I just want to highlight a few things to make sure we consider them, as I'll be OOO after tomorrow and won't be able to participate in any further discussion around this PR.
First, the pattern here around autocompletion is explicitly to insert text. I think it's probably OK to perform some side effect when a suggestion is selected as long as it's made clear what will happen, but we still must insert the suggested text or we will break expectations.
I want to point this out because a consumer might, for example, be tempted to set the value of a suggestion to an empty string and only perform a side effect on selection. This would be unexpected for all users but would especially break the experience for screen reader users as the plain-text value of the suggestion is required to generate hidden assistive text. For example, this hidden hint text is announced when the list is open:
If the value of that suggestion was an empty string or didn't correspond to the visual representation of the suggestion, the hint would become nonsense.
Second, the way text is inserted into the input is more complicated than just updating the state of the input. If you just update the state of the input (outside of an onChange handler, which is special-cased by React) you lose the current cursor position (shifting it to the end of the input) and the undo stack will be cleared.
So to allow insertion of text in places other than the end of the input, and to allow users to undo that insertion via Cmd+Z, the component uses this useSyntheticChange hook to simulate the user actually typing in the input (via execCommand).
I think there may be a temptation to use this event handler to update the value of the input to replace / change the inserted text, which would likely update state directly and be problematic.
Neither of these are direct issues with this feature; more with how it might be used. I wonder if we can come up with a way to prevent misuse via the API?
One option to solve the second concern might be to allow returning a string (ie, string | undefined with undefined leaving default behavior) to support customizing what text is inserted. Then the insertion would happen under the hood so it would still use useSyntheticChange. However, this conflicts with the first concern because the value would differ from the actual inserted text. Maybe that's OK as long as the value is still set to something useful -- I don't think anything in the hint text asserts that the inserted text will exactly match the value. I think such a case might be doable but would definitely need extensive accessibility testing.
|
|
||
| /** Called when a suggestion is selected. | ||
| */ | ||
| onSelectSuggestion?: (suggestion: string) => void |
There was a problem hiding this comment.
To keep consistent with the rest of the props, maybe we should define a SelectSuggestionsEvent type and pass that instead of just the suggestion string. It could also include the trigger & the query text. This could give more useful data to the handler. For example:
type SelectSuggestionsEvent = ShowSuggestionsEvent & {
/** The suggestion that was selected. */
suggestion: Suggestion
}There was a problem hiding this comment.
Done. Please check commit 5d1b9fa
Co-authored-by: Ian Sanders <iansan5653@github.com>
|
@iansan5653 Please note that I removed the code that allowed overwriting of the inserted text. This PR now only adds a callback to notify a consumer what autocomplete item was selected. This should resolve most of the accessibility concerns you've mentioned :) |
| * the inserted text. Do not call `setState` in this handler or the user's cursor | ||
| * position / undo history could be lost. | ||
| */ | ||
| onSelectSuggestion?: (event: SelectSuggestionsEvent) => void |
There was a problem hiding this comment.
Could you please update src/drafts/InlineAutocomplete/InlineAutocomplete.docs.json with this new prop?
There was a problem hiding this comment.
Updated the file - was not able to confirm the change is correct.
There was a problem hiding this comment.
This is done and CI is green.
|
Docs updated! |
Adds
onSelectSuggestion()callback to<InlineAutocomplete />to allow execution of additional code after an autocomplete item was selected.Screenshots
Please provide before/after screenshots for any visual changes
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.