Skip to content

fix(a11y): color contrast, skip link, focus rings, and aria attributes #421

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

Open
gabitoesmiapodo wants to merge 3 commits into
base: test/demo-smoke
Choose a base branch
from
Open

fix(a11y): color contrast, skip link, focus rings, and aria attributes #421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf8852d
fix(a11y): fix color contrast, skip link, focus rings, and aria attri…
gabitoesmiapodo Mar 23, 2026
1ae1991
fix(a11y): address review feedback on accessibility fixes
gabitoesmiapodo Mar 23, 2026
6a2f37e
fix(a11y): guard parseUnits in BigNumberInput sync effect against throws
gabitoesmiapodo Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/components/sharedComponents/BigNumberInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
type RefObject,
useEffect,
useRef,
useState,
} from 'react'
import { formatUnits, maxUint256, parseUnits } from 'viem'
export type RenderInputProps = Omit<InputProps, 'onChange'> & {
Expand Down Expand Up @@ -66,14 +67,23 @@ export const BigNumberInput: FC<BigNumberInputProps> = ({
value,
}: BigNumberInputProps) => {
const inputRef = useRef<HTMLInputElement>(null)
const [hasError, setHasError] = useState(false)

// update inputValue when value changes
useEffect(() => {
Comment on lines 72 to 73
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

In the useEffect that syncs the DOM input value, the parseUnits(...) call inside the effect can throw if the input currently contains an unparseable intermediate string (possible because updateValue returns early on parse errors). Since this effect runs on external value updates, an external update while the user has invalid text could crash rendering; wrap the parseUnits call in a try/catch or guard the string before parsing.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@gabitoesmiapodo gabitoesmiapodo Mar 24, 2026

Choose a reason for hiding this comment

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

Fixed: wrapped parseUnits in a try/catch; on failure a sentinel value of BigInt(-1) forces the DOM value to be overwritten, ensuring the external value prop always wins when the current input is unparseable.

const current = inputRef.current
if (!current) {
return
}
const currentInputValue = parseUnits(current.value.replace(/,/g, '') || '0', decimals)
// The input may contain an intermediate/unparseable string while the user is
// typing; guard against a parseUnits throw so an external value update never
// crashes the effect.
let currentInputValue: bigint
try {
currentInputValue = parseUnits(current.value.replace(/,/g, '') || '0', decimals)
} catch {
currentInputValue = BigInt(-1) // sentinel: force the DOM value to be overwritten
}

if (currentInputValue !== value) {
current.value = formatUnits(value, decimals)
Expand All @@ -91,6 +101,7 @@ export const BigNumberInput: FC<BigNumberInputProps> = ({
const { value } = typeof event === 'string' ? { value: event } : event.currentTarget

if (value === '') {
setHasError(false)
onChange(BigInt(0))
return
}
Expand Down Expand Up @@ -125,12 +136,16 @@ export const BigNumberInput: FC<BigNumberInputProps> = ({
}] and value is: ${value}`
console.warn(message)
onError?.({ value, message })
setHasError(true)
} else {
setHasError(false)
}
Comment on lines 136 to 142
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

hasError is only updated inside the min/max range check. Other early-return paths in updateValue (e.g. empty string, parseUnits failure, decimals-length guard) don’t clear hasError, so aria-invalid can remain true after the user fixes/clears the input. Consider resetting hasError (and optionally clearing onError) on those early returns to keep validity state accurate.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@gabitoesmiapodo gabitoesmiapodo Mar 23, 2026

Choose a reason for hiding this comment

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

Fixed: setHasError(false) is now called on the empty-string early return path.


onChange(newValue)
}

const inputProps = {
'aria-invalid': (hasError || undefined) as true | undefined,
disabled,
onChange: updateValue,
placeholder,
Expand Down
7 changes: 6 additions & 1 deletion src/components/sharedComponents/Hash.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,12 @@ const Hash: FC<HashProps> = ({
aria-label="Copy"
/>
)}
{explorerURL && <ExternalLink href={explorerURL} />}
{explorerURL && (
<ExternalLink
aria-label="View on explorer"
href={explorerURL}
/>
)}
</Flex>
)
}
Expand Down
5 changes: 4 additions & 1 deletion src/components/sharedComponents/TokenInput/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,10 @@ const TokenInput: FC<Props> = ({
showBalance={showBalance}
showTopTokens={showTopTokens}
>
<CloseButton onClick={() => setIsOpen(false)} />
<CloseButton
aria-label="Close"
onClick={() => setIsOpen(false)}
/>
</TokenSelect>
</Dialog.Content>
</Dialog.Positioner>
Expand Down
9 changes: 7 additions & 2 deletions src/components/sharedComponents/ui/CopyButton/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,15 @@ export const CopyButton: FC<Props> = ({
height="fit-content"
justifyContent="center"
lineHeight="1"
outline="none"
padding="0"
_focusVisible={{
outline: '2px solid',
outlineColor: 'primary.default',
outlineOffset: '2px',
borderRadius: '2px',
}}
textDecoration="none"
transition="background-color {durations.moderate}, border-color {durations.moderate}, color {durations.moderate"
transition="background-color {durations.moderate}, border-color {durations.moderate}, color {durations.moderate}"
userSelect="none"
whiteSpace="nowrap"
width="fit-content"
Expand Down
9 changes: 7 additions & 2 deletions src/components/sharedComponents/ui/ExternalLink/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,14 @@ export const ExternalLinkButton: FC<LinkProps> = ({
fontWeight="400"
height="fit-content"
justifyContent="center"
outline="none"
padding="0"
transition="background-color {durations.moderate}, border-color {durations.moderate}, color {durations.moderate"
_focusVisible={{
outline: '2px solid',
outlineColor: 'primary.default',
outlineOffset: '2px',
borderRadius: '2px',
}}
transition="background-color {durations.moderate}, border-color {durations.moderate}, color {durations.moderate}"
whiteSpace="nowrap"
width="fit-content"
_hover={{
Expand Down
7 changes: 6 additions & 1 deletion src/components/sharedComponents/ui/Modal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,12 @@ export const Modal: FC<Props> = ({ css, children, title, onClose, text, ...restP
>
{title}
</Heading>
{onClose && <CloseButton onClick={() => onClose()} />}
{onClose && (
<CloseButton
aria-label="Close"
onClick={() => onClose()}
/>
)}
{children ? children : 'No contents'}
<Text
borderRadius="md"
Expand Down
10 changes: 5 additions & 5 deletions src/components/ui/provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,23 +57,23 @@ export function Provider(props: ColorModeProviderProps) {
default: {
value: {
_light: '#800',
_dark: '#800',
_dark: '#ff6666',
},
},
},
ok: {
default: {
value: {
_light: '#080',
_dark: '#080',
_light: '#006600',
_dark: '#66ee66',
},
},
},
warning: {
default: {
value: {
_light: '#cc0',
_dark: '#cc0',
_light: '#996600',
_dark: '#e6b800',
},
},
},
Expand Down
17 changes: 16 additions & 1 deletion src/routes/__root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Toaster } from '@/src/components/ui/toaster'
import { TransactionNotificationProvider } from '@/src/providers/TransactionNotificationProvider'
import { Web3Provider } from '@/src/providers/Web3Provider'
import { printAppInfo } from '@/src/utils/printAppInfo'
import { Flex } from '@chakra-ui/react'
import { Flex, chakra } from '@chakra-ui/react'
import { Outlet, createRootRoute } from '@tanstack/react-router'
import { Analytics } from '@vercel/analytics/react'
import { useEffect } from 'react'
Expand All @@ -30,11 +30,26 @@ function Root() {
minH="100vh"
w="100%"
>
<chakra.a
bg="bg.default"
color="text.default"
href="#main-content"
left={0}
p={2}
position="absolute"
top="-100%"
zIndex="tooltip"
_focusVisible={{ top: 0 }}
>
Skip to main content
</chakra.a>
<Header />
<Flex
as="main"
direction="column"
flexGrow="1"
id="main-content"
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

Skip link target is a <main> element, which is not focusable by default. Following href="#main-content" may scroll but leave keyboard focus on the skip link, reducing its usefulness. Add tabIndex={-1} (or otherwise ensure the target can receive focus) so activation moves focus into main content.

Suggested change
id="main-content"
id="main-content"
tabIndex={-1}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@gabitoesmiapodo gabitoesmiapodo Mar 23, 2026

Choose a reason for hiding this comment

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

Fixed: added tabIndex={-1} to the <Flex as="main"> element.

tabIndex={-1}
>
<Outlet />
</Flex>
Expand Down
Loading