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: scroll to correct focused row on keyboard navigation #8003

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Oct 21, 2024

Description

Fixes the regression where keyboard navigation could cause the grid to scroll to and focus a wrong row, especially when some items were expanded.

Fixes #7991

Type of change

  • Bugfix

const targetRowInDom = [...this.$.items.children].find((child) => child.index === index);
if (!targetRowInDom) {
this.scrollToIndex(index);
this._scrollToFlatIndex(index);
Copy link
Contributor Author

@vursen vursen Oct 21, 2024

Choose a reason for hiding this comment

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

note: We forgot to update __ensureFlatIndexInViewport after we changed the signature of scrollToIndex to support scrolling to hierarchical indexes.


// Ensure the correct element is set as focusable after scrolling.
// The virtualizer may use a different element to render the item.
this.__updateItemsFocusable();
Copy link
Contributor Author

@vursen vursen Oct 21, 2024

Choose a reason for hiding this comment

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

note: The previous version didn't update the focusable element if it was a row, which often resulted in a wrong row getting focused.

Comment on lines 103 to 104
// Let's use a count lower than pageSize so we can ignore page + pageSize for now
const itemsOnEachLevel = 5;
const itemsOnEachLevel = 100;
Copy link
Member

@tomivirkki tomivirkki Oct 22, 2024

Choose a reason for hiding this comment

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

nit: This is no longer lower than page size so the comment does not apply anymore (and the data provider should be updated to take page + pageSize into account).

Copy link
Contributor Author

@vursen vursen Oct 23, 2024

Choose a reason for hiding this comment

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

Oh, good catch! I completely forgot to add pagination now that the size has increased. Fixed.

Copy link

sonarcloud bot commented Oct 23, 2024

@vursen vursen merged commit 1de304b into main Oct 23, 2024
9 checks passed
@vursen vursen deleted the fix/scroll-to-correct-item-on-keyboard-navigation branch October 23, 2024 06:22
@vaadin-bot
Copy link
Collaborator

Hi @vursen and @vursen, when i performed cherry-pick to this commit to 24.5, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 1de304b
error: could not apply 1de304b... fix: scroll to correct focused row on keyboard navigation (#8003)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@vaadin-bot
Copy link
Collaborator

Hi @vursen and @vursen, when i performed cherry-pick to this commit to 24.4, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 1de304b
error: could not apply 1de304b... fix: scroll to correct focused row on keyboard navigation (#8003)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[grid] Scrolls to wrong element on Tab or arrow key in hierarchical grids with expanded items
4 participants