Skip to content

fix(components): make component's own disabled state win accessibilityState merges - #221

Merged
Chibuzor-Nwemambu merged 2 commits into
mainfrom
220-accessibility-state-merge-order
Jul 7, 2026
Merged

fix(components): make component's own disabled state win accessibilityState merges#221
Chibuzor-Nwemambu merged 2 commits into
mainfrom
220-accessibility-state-merge-order

Conversation

@Chibuzor-Nwemambu

@Chibuzor-Nwemambu Chibuzor-Nwemambu commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #220. Fixes #217.

Test plan

…yState merges

Button, IconButton, Input, and TextArea all merge a caller-supplied
accessibilityState with the component's own computed disabled value, but
the caller's value won on conflict. Flipped the spread order so the
component's own disabled is applied last in all four.

disabled drives real behavior (editable, styling), so letting a caller's
accessibilityState disagree with it can mislead assistive technology users
specifically — e.g. a screen reader announcing "disabled" on a field
that's actually fully interactive, or vice versa. Other accessibilityState
keys (selected, expanded, checked, etc.) are unaffected and stay fully
caller-overridable.

TextField and Search both wrap Input and forward accessibilityState
through, so they inherit this fix without any changes of their own.

Fixes #220
@Chibuzor-Nwemambu

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @Chibuzor-Nwemambu's task in 2m 6s —— View job


Code Review

  • Gather context (read diff + surrounding code)
  • Understand the request
  • Analyze the changes for correctness & consistency
  • Post review feedback

I reviewed all four changed files against the surrounding prop-spread ordering. The intent — component's own disabled wins the accessibilityState merge while other keys stay caller-overridable — is sound. Three of the four components implement it correctly, but IconButton has a blocking bug that makes the fix a no-op there.


🔴 Blocker — IconButton.tsx: the fix never takes effect

In IconButton, the explicit accessibilityState prop is placed before the {...pressableProps} spread:

// packages/components/src/components/Button/IconButton.tsx:44-49
accessibilityState={{
    ...pressableProps.accessibilityState,
    disabled: disabled ?? false,
}}
disabled={disabled}
{...pressableProps}   // ← spreads AFTER, clobbering the computed accessibilityState

accessibilityState is not destructured out of props (only name, tone, size, variant, round, disabled, ref are — BaseButtonProps extends PressableProps, so accessibilityState stays in pressableProps). Because {...pressableProps} comes after the explicit prop, a caller-supplied accessibilityState overrides the entire computed object — the disabled merge is discarded completely.

Concretely, <IconButton disabled accessibilityState={{ selected: true }} /> renders accessibilityState={{ selected: true }} — no disabled key at all. That's the opposite of this PR's goal, and arguably worse than before (a genuinely disabled control announces no disabled state to assistive tech).

Note this is the same JSX ordering the file already had; the PR flipped the inner merge order but the outer spread still wins, so the change has no observable effect on IconButton.

Fix: move accessibilityState to after {...pressableProps}, exactly matching Button.tsx:

<Pressable
    ref={ref}
    style={styles.container}
    accessibilityRole={"button"}
    disabled={disabled}
    {...pressableProps}
    accessibilityState={{
        ...pressableProps.accessibilityState,
        disabled: disabled ?? false,
    }}
>

Fix this →


✅ The other three are correct

  • Button.tsx{...pressableProps} comes first, then the explicit accessibilityState, so the merge wins. Inner order (...spread then disabled) is right. ✔
  • Input.tsx{...rest} is spread on line 116, the explicit accessibilityState={{ ...rest.accessibilityState, disabled }} follows on 126–129, so it wins over rest. ✔
  • TextArea.tsxaccessibilityState is destructured into userAccessibilityState (line 44), so the trailing {...rest} (line 162) does not re-introduce it; { ...userAccessibilityState, disabled } stands. ✔

Notes / non-blocking

Everything else (scope limited to disabled, other a11y keys left caller-overridable, TextField/Search inheriting via forwarding) checks out.
· branch 220-accessibility-state-merge-order

…bleProps spread

Claude's review on this PR caught that IconButton's fix was a no-op:
{...pressableProps} was spread AFTER the explicit accessibilityState prop
in JSX, so a caller-supplied accessibilityState (which lands in
pressableProps, since it isn't destructured out) silently replaced the
entire computed accessibilityState object, disabled key and all. Moved
accessibilityState to after the spread, matching Button.tsx exactly.
@Chibuzor-Nwemambu

Copy link
Copy Markdown
Collaborator Author

Good catch — verified and fixed in 952f426. `IconButton` had `accessibilityState` before `{...pressableProps}` while `Button` had it after; moved it to match `Button.tsx` exactly, so `{...pressableProps}` now spreads first and the explicit `accessibilityState` (with its own inner disabled-wins merge) applies last and actually takes effect.

Noted your point on test coverage too — when the regression tests go into PR #216's `Button.test.tsx` (queued until this PR merges), I'll make sure the `Button.Icon`/`IconButton` case specifically covers a caller passing `accessibilityState` with a conflicting `disabled` key, not just the plain `Button` case — that's exactly the scenario this bug would have slipped through on.

@Chibuzor-Nwemambu
Chibuzor-Nwemambu merged commit ed5d78d into main Jul 7, 2026
7 checks passed
@Chibuzor-Nwemambu
Chibuzor-Nwemambu deleted the 220-accessibility-state-merge-order branch July 7, 2026 10:45
@github-actions github-actions Bot mentioned this pull request Jul 7, 2026
Chibuzor-Nwemambu added a commit that referenced this pull request Jul 7, 2026
…utton/Button.Icon

Covers #221's fix: a caller-supplied accessibilityState merges with the
component's own computed disabled value, with disabled winning on
conflict. The Button.Icon case specifically targets the bug #221's review
caught — verified this exact test fails against that pre-fix IconButton
code (disabled: true, caller's conflicting value winning) and passes
against the fix.
Chibuzor-Nwemambu added a commit that referenced this pull request Jul 7, 2026
…nput/TextArea

Covers #221's fix (now merged): a caller-supplied accessibilityState
merges with the component's own computed disabled value, with disabled
winning on conflict. One consolidated test per component rather than
separate #217/#220 tests, since they're one fix now.

Verified each test fails against its respective pre-#221 source (Input's
original no-merge-at-all code, TextArea's original caller-wins-on-conflict
code) and passes against the fix.
Chibuzor-Nwemambu added a commit that referenced this pull request Jul 7, 2026
Batch 3 of issue #214's component test rollout — 20 tests across 3 suites.

Duplicates the shared test infra from batch-1/2 (test-utils render helper,
@expo/vector-icons mock, @types/react-test-renderer) since this branch was
created independently, off main post-#221, before either batch-1 or
batch-2 merged.

Narrows jest.config.cjs's untested-component checklist the same way
batch-1/2 did: removes SelectionControls (now tested) and reapplies the
permanent exclusions (Dialog/ErrorBoundary/Icon/Label/Portal/Scrim/
_internal/Paper/PressableHighlight).

Radio and Checkbox tests intentionally exclude the "no onPress provided"
scenario for now — see follow-up commit for why.
Chibuzor-Nwemambu added a commit that referenced this pull request Jul 8, 2026
…phy (#216)

## Summary
- Batch 1 of #214's component test rollout: adds behavior tests for
Button (+ Button.Icon), Badge, Divider, Link, and Typography (+
Typography.Header) — 38 tests total (rebalanced from an initial 42 →
briefly 70 → 36 addressing PR review on test quality, then +2 for the
accessibilityState merge regression tests below).
- Adds shared test infra reused by all future batches: `test-utils.tsx`
(render wrapped in `EDSProvider`), an `@expo/vector-icons` mock in
`jest.setup.ts` (fixes act() warnings from async font-loading state,
while preserving real static properties like `.font` so `useEDS.ts`
isn't silently broken), and `@types/react-test-renderer` (fixes RNTL
query results resolving to TS's `error` type).
- Narrows `jest.config.cjs`'s untested-component checklist: removes
Button/Badge/Divider/Link/Typography (now tested), and also drops
Dialog/ErrorBoundary/Icon/Label/Portal/Scrim/`_internal` (internal
pieces covered indirectly via consumers) and Paper/PressableHighlight
(slated for removal/replacement, not worth testing now).
- Adds regression tests for #221 (merged) covering
`Button`/`Button.Icon`'s `accessibilityState` merge — the `Button.Icon`
case specifically targets the JSX-ordering bug that PR #221's review
caught, verified to fail against that pre-fix code.

Contributes to #214 — does not close it (batches 2, SelectionControls,
and the new-component-workflow update are still outstanding).

## Test plan
- [x] `pnpm test` — 38/38 passing across 5 suites
- [x] `pnpm lint` — clean
- [x] `pnpm check-types` — clean
- [x] `pnpm build` — clean, confirmed `test-utils.tsx` doesn't leak into
`dist/`
Chibuzor-Nwemambu added a commit that referenced this pull request Jul 8, 2026
…DSProvider (#218)

## Summary
- Batch 2 of #214's component test rollout: behavior tests for Input,
TextField, TextArea, Search, and EDSProvider — 48 tests total, plus the
shared test infra (`test-utils.tsx`, `@expo/vector-icons` mock,
`@types/react-test-renderer`) duplicated from batch-1's branch, since
this branch was deliberately created independent of batch-1 (branched
off `main` post-#215, not off `214-batch-1-tests`).
- Narrows `jest.config.cjs`'s untested-component checklist the same way
batch-1 did.
- Adds regression tests for #221 (merged) covering `Input`/`TextArea`'s
`accessibilityState` merge — one consolidated test per component (not
separate #217/#220 tests, since they're one fix now), each verified to
fail against its respective pre-#221 source before the fix.

**Note**: this PR previously included an isolated fix for #217
(`Input.tsx`'s `accessibilityState` clobbering caller-supplied values)
plus its regression test. Both were reverted — the fix turned out to be
incomplete (it stopped caller values from being clobbered, but still let
a caller's `disabled` win over the component's own computed value on
conflict). That's now fixed properly, in one place, by #221, which
superseded and closed #217 as well.

Contributes to #214 — does not close it (SelectionControls and the
new-component-workflow update are still outstanding).

## Test plan
- [x] `pnpm test` — 48/48 passing across 5 suites
- [x] `pnpm lint` — clean
- [x] `pnpm check-types` — clean
- [x] `pnpm build` — clean
Chibuzor-Nwemambu added a commit that referenced this pull request Jul 9, 2026
…224)

## Summary
- `Divider.tsx` spread `{...rest}` after its `accessible={false}` /
`importantForAccessibility="no"` defaults, so a caller passing either
prop explicitly could override Divider's "hidden from assistive
technology" behavior.
- Reordered so the component's own defaults always win, since Divider is
purely decorative and no legitimate use case was found for making it
independently accessible/focusable — same reasoning as the `disabled`
merge fix in #221 for Button/IconButton/Input/TextArea.
- Added a regression test asserting
`accessible`/`importantForAccessibility` can't be overridden via props.

Fixes #219

## Test plan
- [x] `pnpm jest Divider` — 4/4 pass
- [x] `pnpm lint` — clean
- [x] `pnpm check-types` — clean
- [x] `pnpm prettier --check` — clean
Chibuzor-Nwemambu added a commit that referenced this pull request Jul 9, 2026
## Summary
- Batch 3 of #214's component test rollout: behavior tests for Switch,
Radio, and Checkbox (SelectionControls) — 22 tests total, plus the
shared test infra (`test-utils.tsx`, `@expo/vector-icons` mock,
`@types/react-test-renderer`) duplicated from batch-1/2's branches,
since this branch was created independently, off `main` post-#221,
before either batch-1 or batch-2 merged.
- Narrows `jest.config.cjs`'s untested-component checklist the same way
batch-1/2 did.
- Along the way, investigated a suspected accessibilityState bug in
`Radio`/`Checkbox` (same category as #220 — component's real disabled
state not reflected in `accessibilityState`). Turned out **not** to be a
real bug: React Native's `Pressable` already auto-overrides
`accessibilityState.disabled` from its own `disabled` prop internally,
so the apparent mismatch never manifested at the rendered level. A fix
was written, found to be unnecessary once a regression test for it
passed against both the pre- and post-fix code, and reverted (commits
`c04a643`/`3798816`). The underlying test coverage for the "no
`onPress`" case was kept (`40e5559`) since it's still valid, correct
behavior worth covering.

Contributes to #214 — does not close it (the workflow-docs update, batch
4, is still outstanding).

## Test plan
- [x] `pnpm test` — 22/22 passing across 3 suites
- [x] `pnpm lint` — clean
- [x] `pnpm check-types` — clean
- [x] `pnpm build` — clean
pomfrida added a commit that referenced this pull request Aug 4, 2026
🤖 I have created a release *beep* *boop*
---


<details><summary>eds-mobile-components: 0.3.1</summary>

##
[0.3.1](eds-mobile-components-v0.3.0...eds-mobile-components-v0.3.1)
(2026-07-09)


### Bug Fixes

* **components:** Divider accessibility defaults can't be overridden
([#224](#224))
([9873283](9873283))
* **components:** make component's own disabled state win
accessibilityState merges
([#221](#221))
([ed5d78d](ed5d78d))
</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] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Frida Erdal <31915755+pomfrida@users.noreply.github.com>
Chibuzor-Nwemambu pushed a commit to equinor/design-system that referenced this pull request Aug 7, 2026
🤖 I have created a release *beep* *boop*
---


<details><summary>eds-mobile-components: 0.3.1</summary>

##
[0.3.1](equinor/design-system-mobile@eds-mobile-components-v0.3.0...eds-mobile-components-v0.3.1)
(2026-07-09)


### Bug Fixes

* **components:** Divider accessibility defaults can't be overridden
([#224](equinor/design-system-mobile#224))
([9873283](equinor/design-system-mobile@9873283))
* **components:** make component's own disabled state win
accessibilityState merges
([#221](equinor/design-system-mobile#221))
([ed5d78d](equinor/design-system-mobile@ed5d78d))
</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] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Frida Erdal <31915755+pomfrida@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants