Warn when NaN is passed as an aria-* attribute value#36340
Open
starboyvarun wants to merge 1 commit intofacebook:mainfrom
Open
Warn when NaN is passed as an aria-* attribute value#36340starboyvarun wants to merge 1 commit intofacebook:mainfrom
starboyvarun wants to merge 1 commit intofacebook:mainfrom
Conversation
aria-* attributes bypass the existing NaN check in ReactDOMUnknownPropertyHook because they return early before that check is reached. When a NaN number reaches the DOM it gets stringified to "NaN", which is invalid for every ARIA attribute type and is almost always a developer mistake (e.g. a missing guard around a computed numeric value). This adds the same NaN check directly inside ReactDOMInvalidARIAHook so that any valid aria-* attribute receiving a NaN value produces a clear warning in development, matching the message already used for non-aria attributes.
liufang88789-ui
left a comment
There was a problem hiding this comment.
Nice fix. The NaN → "NaN" stringification in the DOM is an insidious silent bug, especially since computed numeric values (like derived state from findIndex returning -1) flow into aria-* props frequently.
The placement is right — checking in validateProperty before the return, and mirroring the existing error message pattern keeps it consistent with the rest of the hook. Passing props[key] as the third argument is minimal and clean.
One edge case worth considering: what about Infinity? It has the same stringification problem ("Infinity" is equally invalid for aria attributes), though it's less common in practice. Not necessarily a blocker for this PR, just something to keep in mind.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
aria-* attributes currently bypass the NaN check in
ReactDOMUnknownPropertyHookbecause they return early (line 97-99 of that file) before the check is reached. When aNaNnumber flows through, the DOM stringifies it to the literal string"NaN", which is invalid for every ARIA attribute type and silently breaks accessibility.This is a common pitfall when a computed numeric value is derived from user data or state that can be undefined/null:
Changes:
ReactDOMInvalidARIAHook.js— extendvalidatePropertyto accept the propvalueand add aNaNcheck for valid, correctly-cased aria-* attributes. Mirrors the warning message already used byReactDOMUnknownPropertyHookfor non-aria attributes.ReactDOMInvalidARIAHook-test.js— three new test cases: NaN on a numeric aria attribute (aria-valuenow), NaN on a string-type aria attribute (aria-label), and no false-positive for valid integers.Test plan
yarn test ReactDOMInvalidARIAHook— all tests pass (new + existing)validatePropertysignature changes (internal)