improvement: arrow-key navigation for chat list #4224
Merged
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
WofWca
force-pushed
the
wofwca/a11y-chatlist-arrow-keys-2
branch
2 times, most recently
from
42ec374 to
ec7dd25
Compare
This was referenced Oct 19, 2024
nicodh
approved these changes
Oct 21, 2024
|
Review by testing |
WofWca
force-pushed
the
wofwca/a11y-chatlist-arrow-keys-2
branch
3 times, most recently
from
17803a1 to
b8dccfe
Compare
|
Rebased and adjusted comments. |
WofWca
force-pushed
the
wofwca/a11y-chatlist-arrow-keys-2
branch
from
b8dccfe to
2beb3ea
Compare
WofWca
added a commit
that referenced
this pull request
Oct 21, 2024
This is quite a significant change. I tested a lot of placed where the affected code is used, but still might have missed some regression. And perhaps the behavior is still not ideal in some places, but it's better to assess it in its final form after we add [the roving tabindex functionality (arrow key accessibility)](#4224). Either way this should be an improvement.
WofWca
added a commit
that referenced
this pull request
Oct 21, 2024
This is quite a significant change. I tested a lot of placed where the affected code is used, but still might have missed some regression. And perhaps the behavior is still not ideal in some places, but it's better to assess it in its final form after we add [the roving tabindex functionality (arrow key accessibility)](#4224). Either way I think this is an improvement.
WofWca
added a commit
that referenced
this pull request
Oct 22, 2024
This is quite a significant change. I tested a lot of placed where the affected code is used, but still might have missed some regression. And perhaps the behavior is still not ideal in some places, but it's better to assess it in its final form after we add [the roving tabindex functionality (arrow key accessibility)](#4224). Either way I think this is an improvement.
WofWca
force-pushed
the
wofwca/a11y-chatlist-arrow-keys-2
branch
from
2beb3ea to
a563bda
Compare
|
Rebased |
WofWca
added a commit
that referenced
this pull request
Oct 22, 2024
This is quite a significant change. I tested a lot of placed where the affected code is used, but still might have missed some regression. And perhaps the behavior is still not ideal in some places, but it's better to assess it in its final form after we add [the roving tabindex functionality (arrow key accessibility)](#4224). Either way I think this is an improvement.
WofWca
added a commit
that referenced
this pull request
Oct 24, 2024
This is quite a significant change. I tested a lot of placed where the affected code is used, but still might have missed some regression. And perhaps the behavior is still not ideal in some places, but it's better to assess it in its final form after we add [the roving tabindex functionality (arrow key accessibility)](#4224). Either way I think this is an improvement.
|
@r10s could check the functional aspect of this MR? I want to start adding this everywhere in the app, namely in the accounts list, contact list, for various tabs (e.g. QR reader, emoji picker), in the gallery, and, most importantly, in the messages list. In separate MRs, of course. I want a green light so that I don't waste the efforts in case the entire approach is wrong. @nicodh and @maxphilippov could you also check if this is good code-wise? |
WofWca
force-pushed
the
wofwca/a11y-chatlist-arrow-keys-2
branch
from
a563bda to
2ce97e9
Compare
|
Rebased |
WofWca
added a commit
that referenced
this pull request
Oct 29, 2024
This is quite a significant change. I tested a lot of placed where the affected code is used, but still might have missed some regression. And perhaps the behavior is still not ideal in some places, but it's better to assess it in its final form after we add [the roving tabindex functionality (arrow key accessibility)](#4224). Either way I think this is an improvement.
WofWca
added a commit
that referenced
this pull request
Oct 30, 2024
This should allow us to more easily `setActiveElement` by its ID, without having a reference to `HTMLElement`, e.g. we can more easily set the initial chat list item. This issue was mentioned in #4224: > The initially "active" element is just the first chat item, > and not the currently selected chat.
4 tasks
WofWca
force-pushed
the
wofwca/a11y-chatlist-arrow-keys-2
branch
from
2ce97e9 to
2d9214c
Compare
WofWca
added a commit
that referenced
this pull request
Nov 1, 2024
This is quite a significant change. I tested a lot of placed where the affected code is used, but still might have missed some regression. And perhaps the behavior is still not ideal in some places, but it's better to assess it in its final form after we add [the roving tabindex functionality (arrow key accessibility)](#4224). Either way I think this is an improvement.
WofWca
added a commit
that referenced
this pull request
Nov 1, 2024
This is quite a significant change. I tested a lot of placed where the affected code is used, but still might have missed some regression. And perhaps the behavior is still not ideal in some places, but it's better to assess it in its final form after we add [the roving tabindex functionality (arrow key accessibility)](#4224). Either way I think this is an improvement.
WofWca
force-pushed
the
wofwca/a11y-chatlist-arrow-keys-2
branch
from
2d9214c to
eaf80b5
Compare
Implements the "roving tabindex" approach: https://developer.mozilla.org/en-US/docs/Web/Accessibility/Keyboard-navigable_JavaScript_widgets#technique_1_roving_tabindex. Supersedes #4211. This improves things, but the UX as a whole is not great yet: - The tab order is such that the chat list does not immediately follow the search field, so you have to tab through the other navbar items before you get to the chat list. Same for going back from the chat list to the search bar. - The initially "active" element is just the first chat item, and not the currently selected chat. - Since the chat list is "virtualized", the currently active element might get removed from DOM when the user scrolls, thus we lose track of the item that was last selected. Related: #2784
WofWca
force-pushed
the
wofwca/a11y-chatlist-arrow-keys-2
branch
from
eaf80b5 to
17c8184
Compare
! It's about time.
WofWca
added a commit
that referenced
this pull request
Nov 22, 2024
This should allow us to more easily `setActiveElement` by its ID, without having a reference to `HTMLElement`, e.g. we can more easily set the initial chat list item. This issue was mentioned in #4224: > The initially "active" element is just the first chat item, > and not the currently selected chat.
WofWca
added a commit
that referenced
this pull request
Dec 1, 2024
This should allow us to more easily `setActiveElement` by its ID, without having a reference to `HTMLElement`, e.g. we can more easily set the initial chat list item. This issue was mentioned in #4224: > The initially "active" element is just the first chat item, > and not the currently selected chat.
WofWca
added a commit
that referenced
this pull request
Dec 1, 2024
This should allow us to more easily `setActiveElement` by its ID, without having a reference to `HTMLElement`, e.g. we can more easily set the initial chat list item. This issue was mentioned in #4224: > The initially "active" element is just the first chat item, > and not the currently selected chat.
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.
roving-tabindex.mp4
Implements the "roving tabindex" approach:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/Keyboard-navigable_JavaScript_widgets#technique_1_roving_tabindex.
Supersedes #4211.
This improves things, but the UX as a whole is not great yet:
immediately follow the search field,
so you have to tab through the other navbar items
before you get to the chat list.
Same for going back from the chat list to the search bar.
This can be addressed by HTML semantics: move navbars inside the "chat list" and "chat" sections #4225
and not the currently selected chat.
I can fix it, in a separate MR. I left a TODO.
might get removed from DOM when the user scrolls,
thus we lose track of the item that was last selected.
This is pretty hard to fix. Our virtualization library does not have nice API for this, but luckily IMO this drawback is not that impactful.
Related: #2784
TODO:
If it looks and behaves good in general, I'll proceed with fixing the abovementioned issues, and adding this to other places, such as contact lists and the account list, in separate MRs perhaps.