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(combobox): listbox item focused with home or end now scrolls into view #4304

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Mar 24, 2022

Related Issue: #4265

Summary

This discussion prompted several changes for Combobox to be consistent with the Dropdown behavior:

  • when the combobox is focused but closed, End/Home (fn arrow left/right) scrolls the entire page to the very top/bottom and won’t keep the focused combobox in view
  • Page up/down (fn arrow up/down) scroll up and down as well
  • while it’s open it'll scroll the listbox to focused item into view on Home/End or ArrowUp/Down
  • when you open the listbox, the focus jumps to the first item
  • in the situation where the listbox is open and overflows the page also scroll on these keystrokes to bring the combobox item into view.

Includes adjustments to related failing tests.

@Elijbet Elijbet requested a review from a team as a code owner March 24, 2022 22:28
@github-actions github-actions bot added this to the Sprint 03/14 - 03/25 milestone Mar 24, 2022
@Elijbet Elijbet marked this pull request as draft March 24, 2022 22:28
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Mar 24, 2022
@Elijbet Elijbet marked this pull request as ready for review March 30, 2022 20:27
@Elijbet Elijbet marked this pull request as draft March 31, 2022 18:02
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Looking good!

@Elijbet Elijbet marked this pull request as ready for review April 11, 2022 16:48
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Aside from a few comments, this LGTM! 🎉

@@ -514,7 +516,7 @@ describe("calcite-combobox", () => {
await page.waitForChanges();
expect(await page.evaluate(() => document.activeElement.id)).toBe("myCombobox");

await page.keyboard.press("ArrowDown");
await page.keyboard.press("Space");
Copy link
Member

Choose a reason for hiding this comment

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

Do we also need a test for ArrowDown opening the menu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💡 There is a test below this one that does that.

@Elijbet Elijbet merged commit 8b09553 into master Apr 11, 2022
@Elijbet Elijbet deleted the elijbet/4265-fix-combobox-home/end-bring-first/last-item-into-focus-scroll-into-view branch April 11, 2022 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants