Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Rework composer autocomplete to be smarter and not trap tab #5659

Merged
merged 12 commits into from
Aug 13, 2021

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Feb 17, 2021

Fixes element-hq/element-web#4872
Fixes element-hq/element-web#11071
Fixes element-hq/element-web#17171
Fixes element-hq/element-web#15646

Notes: Autocomplete has been updated to match modern accessibility standards. Navigate via up/down arrows rather than Tab. Enter or Tab to confirm a suggestion. This should be familiar to Slack & Discord users. You can now use Tab to navigate around the application and do more without touching your mouse. No more accidentally sending half of people's names because the completion didn't fire on Enter!

This PR brings our autocomplete in line with ARIA standards and matches the behaviour of Slack & Discord so should be easier for new users to comprehend.

Before After
Tab Navigate Suggestions Accepts Suggestion
Vertical Arrows Navigate Suggestions Navigate Suggestions
Enter Sends Message Accepts Suggestion

This means that the Tab key no longer keeps you trapped in the composer

Preview: https://6114f8642fe2a3998a4ce3cd--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.


Here's what your changelog entry will look like:

✨ Features


Here's what your changelog entry will look like:

✨ Features

@SimonBrandner

This comment has been minimized.

@pvagner
Copy link
Contributor

pvagner commented May 6, 2021

Unfortunatelly this no longer applies. I was using it for a while and now need to either fix this or revert and get used to the default behaviour.

@t3chguy
Copy link
Member Author

t3chguy commented May 6, 2021

@pvagner what do you mean it no longer applies?

@pvagner
Copy link
Contributor

pvagner commented May 11, 2021

Oh, by saying it no longer applies I wanted to say it needs tweaking as it does not merge cleanly and I was not able to do such changes my-self.

… t3chguy/a11y/composer-list-autocomplete

� Conflicts:
�	src/components/structures/LoggedInView.tsx
�	src/components/views/rooms/BasicMessageComposer.tsx
�	src/editor/autocomplete.ts
@t3chguy
Copy link
Member Author

t3chguy commented May 11, 2021

Ah yes, github doesn't show merge conflicts for draft PRs annoyingly, I've resolved that for you now @pvagner

The merge conflict was quite complex, I gave it a cursory test but let me know if it behaves whatsoever differently, it shouldn't.

Thanks!

@t3chguy t3chguy marked this pull request as ready for review May 18, 2021 11:47
@t3chguy t3chguy requested review from a team May 18, 2021 11:47
… t3chguy/a11y/composer-list-autocomplete

� Conflicts:
�	src/autocomplete/AutocompleteProvider.tsx
�	src/components/views/rooms/Autocomplete.tsx
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

looks good overall

src/components/views/rooms/BasicMessageComposer.tsx Outdated Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

rm -rf tabcomplete

… t3chguy/a11y/composer-list-autocomplete

� Conflicts:
�	src/components/views/rooms/BasicMessageComposer.tsx
�	src/editor/autocomplete.ts
@novocaine novocaine requested review from nadonomy and removed request for a team August 12, 2021 10:08
… t3chguy/a11y/composer-list-autocomplete

� Conflicts:
�	src/components/views/rooms/BasicMessageComposer.tsx
�	src/editor/autocomplete.ts
Copy link
Contributor

@nadonomy nadonomy left a comment

Choose a reason for hiding this comment

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

LGTM.

@novocaine @dbkr In the release notes, I think we should highlight this change as existing users will need to battle existing muscle memory and it's bound to cause friction.

I think we'd want to say something along the lines of "Autocomplete suggestions have been updated to match modern accessibility standards. Navigate via arrows rather than tab. Enter to confirm a suggestion."

@t3chguy t3chguy added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Aug 12, 2021
@t3chguy t3chguy merged commit df28280 into develop Aug 13, 2021
@t3chguy t3chguy deleted the t3chguy/a11y/composer-list-autocomplete branch August 13, 2021 15:55
@novocaine
Copy link
Contributor

LGTM.

@novocaine @dbkr In the release notes, I think we should highlight this change as existing users will need to battle existing muscle memory and it's bound to cause friction.

I think we'd want to say something along the lines of "Autocomplete suggestions have been updated to match modern accessibility standards. Navigate via arrows rather than tab. Enter to confirm a suggestion."

note that thanks to the new changelog automation, @t3chguy was simply able to update the PR description with your copy, and dave and I didn't need to lift a finger

@MTRNord
Copy link
Contributor

MTRNord commented Aug 17, 2021

Can we get this as a setting instead? This is a breaking change without warning and it gets annoying already. :/ Tab is the correct way but this breaks it so I now need to add unneeded spaces to be able to send messages as well as taking 2x to write a message. Also there are 8/10 times the wrong emoji being autocompleted. For example :O will get autocompleted to :v: which is no where near the correct one.

@turt2live
Copy link
Member

Please open an issue to report problems. PR comments are not monitored for triage.

@matrix-org matrix-org locked and limited conversation to collaborators Aug 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
7 participants