Skip to content

feat: add button-tab-index-required rule #11719

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

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

anveshmekala
Copy link
Contributor

@anveshmekala anveshmekala commented Mar 7, 2025

Related Issue: #11941

Adds a new custom ESLint rule.

Summary

button-tab-index-required

This rule will catch any button elements without tabIndex attribute. tabIndex attribute is required for button to be focusable when clicked in Safari browser.

Config

No config is needed

Usage

{ "@esri/calcite-components/button-tab-index-required": "error" }

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Mar 7, 2025
@anveshmekala anveshmekala force-pushed the anveshmekala/eslint-rule-native-button-tab-index-required branch from ee75ac7 to b22d93d Compare March 7, 2025 23:56
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Mar 15, 2025
@anveshmekala anveshmekala removed the Stale Issues or pull requests that have not had recent activity. label Mar 27, 2025
Copy link
Contributor

github-actions bot commented Apr 5, 2025

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Apr 5, 2025
@anveshmekala anveshmekala removed the Stale Issues or pull requests that have not had recent activity. label Apr 11, 2025
@anveshmekala anveshmekala marked this pull request as ready for review April 11, 2025 21:20
@anveshmekala anveshmekala requested a review from jcfranco as a code owner April 11, 2025 21:20
@anveshmekala anveshmekala changed the title feat: add native-button-tab-index-required rule feat: add button-tab-index-required rule Apr 11, 2025
@anveshmekala anveshmekala marked this pull request as draft April 11, 2025 21:23
@anveshmekala anveshmekala marked this pull request as ready for review April 11, 2025 21:33
@anveshmekala anveshmekala added the skip visual snapshots Pull requests that do not need visual regression testing. label Apr 11, 2025
@driskull
Copy link
Member

@anveshmekala I'm not finding any info about Safari requiring a tabindex on buttons.

I also can't see any difference in a codepen: https://codepen.io/driskull/pen/OPJKRbw

Are you sure the issue wasn't related to focus trapping and incorrectly closing the input-date-picker?

@driskull
Copy link
Member

I think there may be a larger issue with the component, even clicking on the background of the open date picker shouldn't close it. This is an inconsistency between Safari and Chrome and adding the tabindex to the button doesn't fix that part.

@driskull
Copy link
Member

driskull commented Apr 11, 2025

The issue seems to be with the this.listen("blur", this.blurHandler); blur handler, not the tabindex on button. When clicking on the floating UI dialog its calling blur on the host which is closing it. It doesn't do that in Chrome. So something in that logic needs to change. My guess is the click is bubbling up and causing the blur on the host for some reason. Some kind of composedPath check on the event might prevent this.

Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

I'm not sure this is necessary but I'll defer to @jcfranco.

I don't think safari needs tabindex set and can't seem to find any information to back that up.

@anveshmekala
Copy link
Contributor Author

@driskull The inconsistency is mentioned here.

While debugging #11544, i stumbled up on it. IIRC, safari wasn't focusing the button by default. But the Codepen you shared is consistent across browsers.

@driskull
Copy link
Member

The root of the issue is the blur event causing the component to close in Safari the button being focused does hide the issue but the root of the issue persists and clicking not on a button within the datepicker shouldn't close it either.

@anveshmekala
Copy link
Contributor Author

The root of the issue is the blur event causing the component to close in Safari the button being focused does hide the issue but the root of the issue persists and clicking not on a button within the datepicker shouldn't close it either.

True & Agree. We can freeze this one & fix the root cause. Can you add a bug for the issue with input-date-picker.

@anveshmekala anveshmekala marked this pull request as draft April 11, 2025 23:02
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. skip visual snapshots Pull requests that do not need visual regression testing. Stale Issues or pull requests that have not had recent activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants