Skip to content

fix: consistently parse tabindex, following HTML 5 spec#4637

Merged
WilcoFiers merged 6 commits intodequelabs:developfrom
ahmedhalac:refactor/#4632-tabindex-regex-parse
Nov 25, 2024
Merged

fix: consistently parse tabindex, following HTML 5 spec#4637
WilcoFiers merged 6 commits intodequelabs:developfrom
ahmedhalac:refactor/#4632-tabindex-regex-parse

Conversation

@ahmedhalac
Copy link
Copy Markdown
Contributor

@ahmedhalac ahmedhalac commented Nov 11, 2024

Ensures tabindex is correctly parsed as an integer using regex instead of parseInt to comply with HTML standards.

Closes #4632

Ensures tabindex is correctly parsed as an integer using regex instead of parseInt to comply with HTML standards.

Related to dequelabs#4632
@ahmedhalac ahmedhalac requested a review from a team as a code owner November 11, 2024 20:59
Copy link
Copy Markdown
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

This is a good start, but I think we can do a little better. Going over the code base, it looks like axe has an awful lot of places where it's parsing tabindex. I think it would be better if we put the code for this in one place. Normally we'd do this in a commons function, but because one of the places is in core, we'll need to do this in core/utils. So how about this:

In lib/core/utils/ create a file parse-tabindex.js with a parseTabindex function that accepts either a string or null. And which returns an integer if the string is a valid tabindex value, or null if not valid or if the value is null. This would need tests in a new test/core/utils/parse-tabindex.js.

This function could then be used in the following files, instead of the current ad-hoc methods:

  • lib/checks/keyboard/focusable-no-name-evaluate.js
  • lib/checks/keyboard/no-focusable-content-evaluate.js
  • lib/checks/keyboard/tabindex-evaluate.js
  • lib/commons/dom/get-tabbable-elements.js
  • lib/commons/dom/inserted-into-focus-order.js
  • lib/commons/dom/is-focusable.js
  • lib/commons/dom/is-in-tab-order.js
  • lib/core/base/context/create-frame-context.js
  • lib/rules/autocomplete-matches.js
  • lib/rules/no-negative-tabindex-matches.js

Add parseTabindex method in lib/core/utils and reused it in multiple places across the codebase

Related to dequelabs#4632
Copy link
Copy Markdown
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

This is looking great! The change required a few additional updates, this is looking really good.

Resolving pull request comments

Related to dequelabs#4637
Copy link
Copy Markdown
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

This is looking great. @ahmedhalac thank you very much for the contribution!

For the books: I reviewed this PR for security.

@WilcoFiers WilcoFiers changed the title refactor(#4632): correct tabindex parsing logic fix: consistently parse tabindex, following HTML 5 spec Nov 20, 2024
@WilcoFiers WilcoFiers merged commit 645a850 into dequelabs:develop Nov 25, 2024
straker pushed a commit that referenced this pull request Mar 4, 2025
Ensures tabindex is correctly parsed as an integer using regex instead
of parseInt to comply with HTML standards.

Closes #4632
@straker straker mentioned this pull request Mar 4, 2025
WilcoFiers added a commit that referenced this pull request Mar 5, 2025
### Bug Fixes

- **aria-allowed-role:** Add math to allowed roles for img element
([#4658](#4658))
([f6dddd9](f6dddd9)),
closes [#4657](#4657)
- **captions:** fix grammar in captions check incomplete message
([#4661](#4661))
([3ef7058](3ef7058))
- consistently parse tabindex, following HTML 5 spec
([#4637](#4637))
([3b0a361](3b0a361)),
closes [#4632](#4632)
- **core:** measure perf for async checks
([#4609](#4609))
([e7dc26e](e7dc26e))
- **locale:** fixed typos in german (DE) locale
([#4631](#4631))
([0740980](0740980))
- **locale:** proofread and updated de.json
([#4643](#4643))
([910cdb2](910cdb2))
- **no-autoplay-audio:** don't timeout for preload=none media elements
([#4684](#4684))
([b7f1ad1](b7f1ad1))
- **target-size:** do not treat focusable tabpanels as targets
([#4702](#4702))
([67d4e4f](67d4e4f)),
closes [#4421](#4421)
[#4701](#4701)
straker added a commit that referenced this pull request Oct 9, 2025
##
[4.11.0](v4.10.3...v4.11.0)
(2025-10-07)

### Features

- add RGAA tags to rules
([#4862](#4862))
([53a925a](53a925a))
- **aria-prohibited-attr:** add support for fallback roles
([#4325](#4325))
([62a19a9](62a19a9))
- **axe.d.ts:** add nodeSerializer typings
([#4551](#4551))
([a2f3a48](a2f3a48)),
closes [#4093](#4093)
- **DqElement:** deprecate fromFrame function
([#4881](#4881))
([374c376](374c376)),
closes [#4093](#4093)
- **DqElement:** Truncate large `html` strings when the element has a
large outerHTML string
([#4796](#4796))
([404a4fb](404a4fb)),
closes [#4544](#4544)
- **get-xpath:** return proper relative selector for id
([#4846](#4846))
([1035f9e](1035f9e)),
closes [#4845](#4845)
- **i18n:** Add Portugal Portuguese translation
([#4725](#4725))
([5b6a65a](5b6a65a))
- incomplete with node on which an error occurred
([#4863](#4863))
([32ed8da](32ed8da))
- **locale:** Added ru locale
([#4565](#4565))
([067b01d](067b01d))
- **tap:** some best practice rules map to RGAA
([#4895](#4895))
([bc33f4c](bc33f4c))
- **td-headers-attr:** report headers attribute referencing other <td>
elements as unsupported
([#4589](#4589))
([ec7c6c8](ec7c6c8)),
closes [#3987](#3987)

### Bug Fixes

- **aria-allowed-role:** add form to allowed roles of form element
([#4588](#4588))
([8aa47ac](8aa47ac)),
closes
[/github.com/dequelabs/axe-core/blob/develop/lib/standards/html-elms.js#L264](https://github.com/dequelabs//github.com/dequelabs/axe-core/blob/develop/lib/standards/html-elms.js/issues/L264)
- **aria-allowed-role:** Add math to allowed roles for img element
([#4658](#4658))
([95b6c18](95b6c18)),
closes [#4657](#4657)
- **autocomplete-valid :** Ignore readonly autocomplete field
([#4721](#4721))
([491f4ec](491f4ec)),
closes [#4708](#4708)
- **autocomplete-valid:** treat values "xon" and "xoff" as
non-WCAG-violations
([#4878](#4878))
([52bc611](52bc611)),
closes [#4877](#4877)
- **axe.d.ts:** add typings for preload options object
([#4543](#4543))
([cfd2974](cfd2974))
- **button-name,input-button-name,input-img-alt:** allow label to give
accessible name
([#4607](#4607))
([a9710d7](a9710d7)),
closes [#4472](#4472)
[#3696](#3696)
[#3696](#3696)
- **captions:** fix grammar in captions check incomplete message
([#4661](#4661))
([11de515](11de515))
- **color-contrast:** do not run on elements with font-size: 0
([#4822](#4822))
([d77c885](d77c885)),
closes [#4820](#4820)
- consistently parse tabindex, following HTML 5 spec
([#4637](#4637))
([645a850](645a850)),
closes [#4632](#4632)
- **core:** measure perf for async checks
([#4609](#4609))
([7e9bacf](7e9bacf))
- fix grammar when using "alternative text" in a sentence
([#4811](#4811))
([237a586](237a586)),
closes [#4394](#4394)
- **get-ancestry:** add nth-child selector for multiple siblings of
shadow root ([#4606](#4606))
([1cdd6c3](1cdd6c3)),
closes [#4563](#4563)
- **get-ancestry:** don't error when there is no parent
([#4617](#4617))
([a005703](a005703))
- **locale:** fix typos in japanese (ja) locale
([#4856](#4856))
([3462cc5](3462cc5))
- **locale:** fixed typos in german (DE) locale
([#4631](#4631))
([b7736de](b7736de))
- **locale:** proofread and updated de.json
([#4643](#4643))
([8060ada](8060ada))
- **meta-viewport:** lower impact to moderate
([#4887](#4887))
([2f32aa5](2f32aa5)),
closes [#4714](#4714)
- **no-autoplay-audio:** don't timeout for preload=none media elements
([#4684](#4684))
([cdc871e](cdc871e))
- **performanceTimer:** throwing in axe catch clause
([#4852](#4852))
([a4ade04](a4ade04)),
closes
[/github.com/dequelabs/axe-core/blob/e7dae4ec48cbfef74de9f833fdcfb178c1002985/lib/core/base/rule.js#L297-L300](https://github.com/dequelabs//github.com/dequelabs/axe-core/blob/e7dae4ec48cbfef74de9f833fdcfb178c1002985/lib/core/base/rule.js/issues/L297-L300)
- **performanceTimer:** work in frames
([#4834](#4834))
([d7dfebc](d7dfebc))
- **rules:** Change "alternate text" to "alternative text"
([#4582](#4582))
([b03ada3](b03ada3))
- **target-size:** do not treat focusable tabpanels as targets
([#4702](#4702))
([60d11f2](60d11f2)),
closes [#4421](#4421)
[#4701](#4701)
- **type:** correct RuleError type
([#4893](#4893))
([d1aa8e2](d1aa8e2))
- **types:** correct raw types
([#4903](#4903))
([3eade11](3eade11))

This PR was opened by a robot 🤖 🎉
rafaelpaniago1 added a commit to masganem/axe-core-mes-20252 that referenced this pull request Nov 7, 2025
Implementação da correção do bug onde isFocusable() usava parseInt()
incorretamente para validar tabindex, não seguindo a spec HTML5.

Solução aplicada:
- Substituído parseInt() por regex /^\s*([-+]?\d+)/
- Seguindo spec: https://html.spec.whatwg.org/#rules-for-parsing-integers
- Arquivo modificado: lib/commons/dom/is-focusable.js

Comparação com solução oficial (PR dequelabs#4637):
- Minha solução: corrige o bug mas apenas em 1 arquivo
- Solução oficial: criou função reutilizável parseTabindex()
  aplicada em 13 arquivos + testes unitários

Análise completa em: ANALISE-ISSUE-4632.md

Referências:
- Issue: dequelabs#4632
- PR oficial: dequelabs#4637
- Commit oficial: 645a850
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

isFocusable is too strict on tabindex

2 participants