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.

Sign up for GitHub

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

feat: virtual list item selection #8318

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
+1,491 −22
Open

feat: virtual list item selection #8318

Changes from 1 commit
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
627689b
feat: virtual list item selection
tomivirkki Nov 22, 2024
02e00f0
test: add an integration test
tomivirkki Nov 25, 2024
1287b6c
test: more tests and fixes
tomivirkki Nov 25, 2024
5273a0a
fix: do not try to select undefined items
tomivirkki Nov 27, 2024
9892d7a
cleanup
tomivirkki Nov 29, 2024
374e690
fix: scroll the focused index to view on space
tomivirkki Dec 2, 2024
57f6dfd
refactor: scroll to focused index
tomivirkki Dec 2, 2024
8033ace
feat: add selected in renderer model
tomivirkki Dec 2, 2024
2516e39
feat: aria semantics
tomivirkki Dec 2, 2024
e33511e
feat: add itemAccessibleNameGenerator
tomivirkki Dec 3, 2024
a549207
fix: remove navigating state on selection mode unset
tomivirkki Dec 3, 2024
734d6e1
refactor: cleanup and test coverage
tomivirkki Dec 3, 2024
3d78d3f
fix: bugfixes
tomivirkki Dec 3, 2024
a9fd9b0
refactor: cleanup
tomivirkki Dec 4, 2024
8e7e837
refactor: cleanup
tomivirkki Dec 5, 2024
2acdbf6
fix an issue with single-selection
tomivirkki Dec 5, 2024
26af05c
refactor: change selection mode type
tomivirkki Dec 5, 2024
e408af5
refactor: cleanup
tomivirkki Dec 5, 2024
a407c4a
refactor: cleanup
tomivirkki Dec 6, 2024
1854b36
fix: do not mark element focused if it does not match focus index
tomivirkki Dec 9, 2024
c626bcb
refactor: avoid excess re-renders on click select
tomivirkki Dec 9, 2024
7e2eb5b
fix: do not rerender when tabbing focusable children of same parent
tomivirkki Dec 9, 2024
d16de3d
docs: clarify the need for focus exit
tomivirkki Dec 9, 2024
5a9e6f9
refactor: remove theme changes for now
tomivirkki Dec 10, 2024
6c8734a
fix: browser-specific fixes
tomivirkki Dec 10, 2024
33833c7
fix types
tomivirkki Dec 10, 2024
7c5e2ec
refactor: cleanup
tomivirkki Dec 10, 2024
272c046
fix: make sure focused index element gets focused on space select
tomivirkki Dec 10, 2024
2c4dae6
refactor: avoid jumpiness on arrow navigation
tomivirkki Dec 10, 2024
24001d4
chore: clean up dev page
tomivirkki Dec 10, 2024
3fbfd1f
refactor: cleanup and fixes
tomivirkki Dec 10, 2024
19ebae1
docs: document new state attributes
tomivirkki Dec 10, 2024
b61a645
fix: mark root element focused in interaction mode
tomivirkki Dec 10, 2024
28ded0e
test: fix a timing issue in an integration test
tomivirkki Dec 10, 2024
275aac1
refactor: cleanup
tomivirkki Dec 11, 2024
993af45
fix: use position absolute for focus-exit to avoid scroll position ch…
tomivirkki Dec 11, 2024
a0bc08a
fix: always update focus index on click
tomivirkki Dec 11, 2024
c4802cf
refactor: use getter for __isSelectable
tomivirkki Dec 11, 2024
a499cbd
fix: do not throw on enter when focus item not rendered
tomivirkki Dec 11, 2024
20854b0
refactor: split out virtual list base mixin
tomivirkki Dec 11, 2024
9b37d4d
refactor __getItemModel
tomivirkki Dec 11, 2024
3a5f2d4
chore: cleanup
tomivirkki Dec 11, 2024
bb503ee
refactor: relocate a11y
tomivirkki Dec 11, 2024
d67dd5b
Merge remote-tracking branch 'origin/main' into feat/virtual-list-sel…
tomivirkki Dec 12, 2024
3b5bda3
Update packages/virtual-list/src/vaadin-virtual-list-selection-mixin.js
tomivirkki Dec 12, 2024
86b695f
test: add explicit typing tests for undefined
tomivirkki Dec 12, 2024
7901b01
fix: set aria-selected asynchronously
tomivirkki Dec 12, 2024
70dbd70
fix: flush virtualizer on arrow navigation
tomivirkki Dec 12, 2024
46369a8
fix: limit the ariaSelected workaround to single-select only
tomivirkki Dec 12, 2024
16ed88c
only have navigating attribute on keyboard navigation
tomivirkki Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Loading
Loading status checks…
refactor: use getter for __isSelectable
@tomivirkki
tomivirkki committed Dec 11, 2024
commit c4802cf18054d2bc2dc279fd5211279a0a4aa090
26 changes: 13 additions & 13 deletions packages/virtual-list/src/vaadin-virtual-list-selection-mixin.js
View file
Copy link
Contributor

Choose a reason for hiding this comment

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

When using multi-select it only announces the number of selected items that are currently rendered. So if three selected items are out of view and you select a new one it will announce that only a single item is selected, while there are actually four. Another issue is that scrolling selected items out of view (so that they are removed from DOM) will announce that they have been removed from the selection, which is not the case.

Only tested this with VoiceOver, but I think this is a general issue with how virtual list works.

Copy link
Member Author

Choose a reason for hiding this comment

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

How do I make VO announce the number of selected items? While navigating, I only saw announcements related to the currently focused item selection state and its position.

Kapture.2024-12-12.at.11.52.15.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

It announces the number when changing the selection.

I also just tested with NVDA, which never announces the current selection, neither for single nor multi listboxes. You only get information about the selection state when going through individual items.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using single selection, when you switch the selection from one item to another one, VoiceOver doesn't announce a selection change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Original file line number Diff line number Diff line change
@@ -96,7 +96,7 @@ export const SelectionMixin = (superClass) =>
}

/** @private */
__isSelectable() {
get __isSelectable() {
return this.selectionMode !== 'none';
}

@@ -114,11 +114,11 @@ export const SelectionMixin = (superClass) =>
el.tabIndex = isFocusable ? 0 : -1;
el.toggleAttribute(
'focused',
this.__isSelectable() && this.__focusIndex === index && el.contains(this.__getActiveElement()),
this.__isSelectable && this.__focusIndex === index && el.contains(this.__getActiveElement()),
);

el.role = this.__isSelectable() ? 'option' : 'listitem';
el.ariaSelected = this.__isSelectable() ? String(this.__isSelected(item)) : null;
el.role = this.__isSelectable ? 'option' : 'listitem';
el.ariaSelected = this.__isSelectable ? String(this.__isSelected(item)) : null;
el.ariaSetSize = String(this.items.length);
el.ariaPosInSet = String(index + 1);

@@ -142,7 +142,7 @@ export const SelectionMixin = (superClass) =>

/** @private */
__updateAria() {
this.role = this.__isSelectable() ? 'listbox' : 'list';
this.role = this.__isSelectable ? 'listbox' : 'list';
this.ariaMultiSelectable = this.selectionMode === 'multi' ? 'true' : null;
}

@@ -153,7 +153,7 @@ export const SelectionMixin = (superClass) =>

/** @private */
__selectionItemsUpdated() {
if (!this.__isSelectable()) {
if (!this.__isSelectable) {
return;
}

@@ -268,10 +268,10 @@ export const SelectionMixin = (superClass) =>

/** @private */
__updateNavigating(navigating) {
const isNavigating = this.__isSelectable() && navigating;
const isNavigating = this.__isSelectable && navigating;
this.toggleAttribute('navigating', isNavigating);

const isInteracting = this.__isSelectable() && !navigating;
const isInteracting = this.__isSelectable && !navigating;
this.toggleAttribute('interacting', isInteracting);

this.__updateFocusable();
@@ -281,7 +281,7 @@ export const SelectionMixin = (superClass) =>
/** @private */
__updateFocusable() {
const isFocusable = !!(this.__isNavigating() && this.items && this.items.length);
if (this.__isSelectable()) {
if (this.__isSelectable) {
this.tabIndex = isFocusable ? 0 : -1;
} else {
this.removeAttribute('tabindex');
@@ -296,7 +296,7 @@ export const SelectionMixin = (superClass) =>

/** @private */
__onKeyDown(e) {
if (e.defaultPrevented || !this.__isSelectable()) {
if (e.defaultPrevented || !this.__isSelectable) {
return;
}

@@ -376,7 +376,7 @@ export const SelectionMixin = (superClass) =>

/** @private */
__onClick(e) {
if (!this.__isSelectable() || !this.__isNavigating()) {
if (!this.__isSelectable || !this.__isNavigating()) {
return;
}
if (this.__getActiveElement() === this) {
@@ -393,7 +393,7 @@ export const SelectionMixin = (superClass) =>

/** @private */
__onFocusIn(e) {
if (!this.__isSelectable()) {
if (!this.__isSelectable) {
return;
}

@@ -418,7 +418,7 @@ export const SelectionMixin = (superClass) =>

/** @private */
__onFocusOut(e) {
if (!this.__isSelectable()) {
if (!this.__isSelectable) {
return;
}
if (!this.contains(e.relatedTarget)) {