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

Jump to bottom

fix(dropdown): prevents navigating dropdown items with Tab key #7527

Merged
merged 7 commits into from
Aug 22, 2023
Merged

fix(dropdown): prevents navigating dropdown items with Tab key #7527

merged 7 commits into from
Aug 22, 2023

Conversation

anveshmekala
Copy link
Contributor

@anveshmekala anveshmekala commented Aug 15, 2023 •
edited

Related Issue: #6610

Summary

  • Prevents navigating the dropdown list using Tab key
  • When Tab key is down with dropdown open the focus shifts on to next focusable element
  • When Shift+Tab key is down with dropdown open the focus shifts back on to trigger element.

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Aug 15, 2023
@anveshmekala anveshmekala marked this pull request as ready for review August 18, 2023 19:16
@anveshmekala anveshmekala requested a review from a team as a code owner August 18, 2023 19:16
@anveshmekala anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Aug 18, 2023
jcfranco
jcfranco reviewed Aug 19, 2023
View reviewed changes
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, @anveshmekala! 😎

packages/calcite-components/src/components/dropdown/dropdown.tsx Outdated Show resolved Hide resolved
packages/calcite-components/src/components/dropdown/dropdown.e2e.ts Outdated
@@ -724,98 +724,169 @@ describe("calcite-dropdown", () => {
await page.waitForChanges();
expect(await dropdownWrapper.isVisible()).toBe(false);
});
describe("keyboard support", () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think tests and describe blocks may need rearranging. This top one is for keyboard support and there's nested ones covering pointer interaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the keyboard support describe block since most of the tests are using click to open the dropdown.

packages/calcite-components/src/components/dropdown/dropdown.tsx Outdated
this.closeCalciteDropdown();
if (this.items.indexOf(target) > -1) {
this.open = false;
this.updateTabIndexOfItems(event);
Copy link
Member

Choose a reason for hiding this comment

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

Should we call this when the active item is updated vs only on Tab?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not required to call when active Item is updated. When the user hits Tab the tabIndex of the current focused item will be set to 0 ( though selected item is different from focused item). When the user opens the dropdown again then the selectedItem is focused using setFocus method.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. I was wondering it the items could be left in an odd state.

@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 21, 2023
jcfranco
jcfranco approved these changes Aug 22, 2023
View reviewed changes
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.

Noice! 👇⌨️👇

packages/calcite-components/src/components/dropdown/dropdown.tsx Outdated
@@ -675,4 +672,10 @@ export class Dropdown
this.el.addEventListener("calciteDropdownOpen", this.toggleOpenEnd);
}
};

private updateTabIndexOfItems(event: CustomEvent<ItemKeyboardEvent>): void {
Copy link
Member

Choose a reason for hiding this comment

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

To make it more general and decouple the event from this, can you update the signature to pass the item directly instead of the event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. updated.

@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 22, 2023
@anveshmekala anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 22, 2023
geospatialem
geospatialem approved these changes Aug 22, 2023
View reviewed changes
Copy link
Member

@geospatialem geospatialem left a comment

Choose a reason for hiding this comment

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

Awesome! 💪🏻 Great a11y update - works well with keyboard navigation and with JAWS active.

@anveshmekala anveshmekala merged commit 3ea658d into main Aug 22, 2023
15 checks passed
@anveshmekala anveshmekala deleted the anveshmekala/6610-dropdown-remove-list-navigation-with-tab branch August 22, 2023 21:43
@github-actions github-actions bot added this to the 2023 August Priorities milestone Aug 22, 2023
@calcite-admin calcite-admin mentioned this pull request Aug 22, 2023
Merged
benelan pushed a commit that referenced this pull request Sep 1, 2023
@calcite-admin @github-actions
chore: release main (#7571)
784cc25 
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-components: 1.7.0</summary>

##
[1.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@1.6.1...@esri/calcite-components@1.7.0)
(2023-09-01)


### Features

* **action-bar, action-pad, action-group:** Add label properties for
group context
([#7415](#7415))
([b34f36d](b34f36d))
* **combobox:** Add single-persist selection mode
([#7583](#7583))
([dab06a3](dab06a3))
* **flow:** Add support for custom flow-item elements
([#7608](#7608))
([197adfe](197adfe))
* **input-date-picker:** Normalize year to current century for user
typed values only
([#7638](#7638))
([a1db718](a1db718))
* **input-number:** Add integer property
([#7646](#7646))
([cd66a6d](cd66a6d))
* **input-time-picker:** Support fractional seconds
([#7532](#7532))
([c2bf34b](c2bf34b))
* **meter:** Add Meter component
([#7401](#7401))
([47163ed](47163ed))
* **sheet:** Add Sheet component
([#7561](#7561))
([f12a393](f12a393))
* **sheet:** Update default widths
([#7617](#7617))
([47d2c0b](47d2c0b))
* **shell:** Add "Sheets" Slot
([#7579](#7579))
([e798765](e798765))
* **table:** Add Table and related components
([#7607](#7607))
([b067e72](b067e72))


### Bug Fixes

* **accordion, accordion-item:** Improve a11y
([#7560](#7560))
([b5170b6](b5170b6))
* Add drag styles for improved UX
([#7644](#7644))
([afbb764](afbb764))
* **block, block-section:** Improve a11y
([#7557](#7557))
([1f44f6b](1f44f6b))
* **chip-group:** Add existence checks
([#7586](#7586))
([5ca64f1](5ca64f1))
* **color-picker:** Update value when alphaChannel is toggled
([#7563](#7563))
([1f753dd](1f753dd))
* **combobox:** Prevent deselecting items via keyboard in single-persist
mode
([#7634](#7634))
([4f5f8b0](4f5f8b0))
* **combobox:** Update combobox height to follow design spec
([#7558](#7558))
([ec08845](ec08845))
* **date-picker:** Set start of week to monday in zh-CN
([#7578](#7578))
([7e385cb](7e385cb))
* **dropdown:** Prevents navigating dropdown items with Tab key
([#7527](#7527))
([3ea658d](3ea658d))
* Ensure label only focuses the first labelable child
([#7553](#7553))
([426159c](426159c))
* **flow:** Catch error when beforeBack promise is rejected
([#7601](#7601))
([cb70671](cb70671))
* **input-date-picker, input-time-picker:** Do not show dropdown
affordance when read-only
([#7559](#7559))
([5a3f19c](5a3f19c))
* **input, input-number:** Correctly sanitize numbers when pasting
string with 'e'
([#7648](#7648))
([b8bc11c](b8bc11c))
* **list, sortable-list, value-list:** Emit calciteListOrderChange when
dragging between lists
([#7614](#7614))
([4653581](4653581))
* **list:** Fixes dragging nested list items
([#7555](#7555))
([c25f7b3](c25f7b3))
* **list:** Stop emitting calciteListChange when a list-item is disabled
or closed.
([#7624](#7624))
([7008463](7008463))
* **loader:** Tweak loading animations to work in Safari
([#7564](#7564))
([2103654](2103654))
* **modal:** Catch error when beforeClose promise is rejected
([#7600](#7600))
([70405d0](70405d0))
* **modal:** Handle removal of open attribute and prevent multiple
beforeClose calls
([#7470](#7470))
([f31588f](f31588f))
* **rating:** Adds focus outline on click
([#7341](#7341))
([af30073](af30073))
* **segmented-control:** Refresh items when added dynamically
([#7567](#7567))
([2e36eb3](2e36eb3))
* **split-button:** Update divider and borders to follow design spec
([#7568](#7568))
([8df59ab](8df59ab))
* **tree-item:** Move focus outline to item label area
([#7581](#7581))
([1327cef](1327cef))
* **tree-item:** Updates state when selection changes programmatically
for `selection-mode='ancestors'`
([#7572](#7572))
([40758c5](40758c5))
* **tree:** Improve keyboard navigation
([#7618](#7618))
([826a5cb](826a5cb))
</details>

<details><summary>@esri/calcite-components-react: 1.7.0</summary>

##
[1.7.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@1.6.1...@esri/calcite-components-react@1.7.0)
(2023-09-01)


### Bug Fixes

* Make sure components are defined in environments like in codesandbox
([#7632](#7632))
([7005cce](7005cce))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^1.7.0-next.22 to ^1.7.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

@geospatialem geospatialem geospatialem approved these changes

Assignees

@anveshmekala anveshmekala

Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
2023 August Priorities
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@anveshmekala @jcfranco @geospatialem