Skip to content

Fix keyboard navigation scrolling in dropdowns #26358

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

Merged
timja merged 3 commits into from
Mar 4, 2026
+1 −13
Merged

Fix keyboard navigation scrolling in dropdowns #26358

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47cc730
Fix keyboard navigation scrolling in dropdowns
SalmanDeveloperz Feb 24, 2026
7d2da53
Fix lint
timja Feb 24, 2026
c06b5ad
Use nearest scroll behavior for smoother navigation
SalmanDeveloperz Feb 25, 2026
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
14 changes: 1 addition & 13 deletions src/main/js/util/keyboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,22 +82,10 @@ export default function makeKeyboardNavigable(

function scrollAndSelect(selectedItem, selectedClass, items) {
if (selectedItem) {
if (!isInViewport(selectedItem)) {
selectedItem.scrollIntoView(false);
}
selectedItem.scrollIntoView({ block: "center", behavior: "smooth" });
Copy link
Member

@timja timja Feb 24, 2026

Choose a reason for hiding this comment

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

Whilst this does fix the bug (which is the priority) it is scrolling too early and its a bit jumpy.

Could you investigate if the isInViewport function could be fixed instead? as I assume that was intended to only scroll when needed. (cc @mawinter69 as the author of that change)

the block: center does seem better. When it was anchored to the bottom which is what false does it was good when going down but when going back up it was a bit strange feeling.

Copy link
Contributor Author

@SalmanDeveloperz SalmanDeveloperz Feb 24, 2026

Choose a reason for hiding this comment

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

Good point. I tested with block: 'nearest' initially, which only scrolls when needed, but it didn't show section headers when wrapping from bottom to top (e.g. selecting "System" didn't show the "Actions" and "System Configuration" header above it).
image

The isInViewport function checks if the element is in the browser window, not the dropdown container. Items scrolled out of view in the dropdown still pass the check because they're technically in the window, so scrolling never triggers.

I can fix isInViewport to check visibility within the dropdown bounds instead. That would keep the "only scroll when needed" behavior while fixing the bug. Want me to go that route?

Copy link
Member

@timja timja Feb 24, 2026

Choose a reason for hiding this comment

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

I can fix isInViewport to check visibility within the dropdown bounds instead. That would keep the "only scroll when needed" behavior while fixing the bug. Want me to go that route?

Give it a go, would be better if it was less jarring, but if we can't get something nicer at least this works.

Copy link
Contributor Author

@SalmanDeveloperz SalmanDeveloperz Feb 24, 2026

Choose a reason for hiding this comment

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

Will update to check dropdown container visibility. Testing now !

Copy link
Contributor Author

@SalmanDeveloperz SalmanDeveloperz Feb 25, 2026

Choose a reason for hiding this comment

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

Could you investigate if the isInViewport function could be fixed instead? as I assume that was intended to only scroll when needed. (cc @mawinter69 as the author of that change)

Tried fixing isInViewport to check dropdown container visibility but ran into issues, the dropdown doesn't have a max-height in the DOM, so it sizes to fit all items and visibility checks always pass.

The "nearest" approach scrolls smoothly and only when needed.
Minor edge case: section headers don't always show when wrapping bottom to top, but navigation works fine.

Video attached.

dropdown.with.nearest.fix.mp4

Worth committing or should try something else?

Copy link
Member

@timja timja Feb 25, 2026

Choose a reason for hiding this comment

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

yeah lets go for nearest

Copy link
Contributor Author

@SalmanDeveloperz SalmanDeveloperz Feb 25, 2026

Choose a reason for hiding this comment

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

Done!

selectedItem.classList.add(selectedClass);
if (items.includes(document.activeElement)) {
selectedItem.focus();
}
}
}

function isInViewport(element) {
const rect = element.getBoundingClientRect();
return (
rect.top >= 0 &&
rect.left >= 0 &&
rect.bottom <= window.innerHeight &&
rect.right <= window.innerWidth
);
}
Loading