feat: warn when firing events on disabled elements - #1934
Conversation
931ca0e to
0949859
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
A test-local logger.warn spy is restored in a way that can leak mocks if the test fails before cleanup.
Pull request overview
Adds a configurable warning to help diagnose fireEvent calls that silently do nothing when the target element is disabled, leveraging existing disabled-state computation and the shared logger.
Changes:
- Emit a
logger.warnwhenfireEventfinds no handler and the nearest touch responder is disabled (opt-out viaconfigure({ disabledEventWarning: false })). - Add
disabledEventWarningto global config (defaulttrue) and update documentation. - Extend
fire-eventand config tests to cover the warning behavior and default config assertions.
File summaries
| File | Description |
|---|---|
| website/docs/14.x/docs/api/misc/config.mdx | Documents the new disabledEventWarning configuration option and opt-out usage. |
| src/fire-event.ts | Adds disabled-target detection and a warning when fireEvent results in no handler due to disabled state. |
| src/config.ts | Introduces disabledEventWarning in Config, default config, and configure() plumbing. |
| src/tests/fire-event.test.tsx | Adds tests validating warning behavior and config opt-out; suppresses warning output in relevant suites. |
| src/tests/config.test.ts | Updates default-config equality assertion to include disabledEventWarning: true. |
Review details
Suppressed comments (1)
src/tests/fire-event.test.tsx:907
logger.warnis spied and restored manually at the end of the test. If an assertion throws before the restore line, the spy will leak into later tests and can hide/alter warning expectations.
Wrap the test body in a try/finally (or move the spy to a beforeEach/afterEach) so the restore always runs.
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});
function TestChildTouchableComponent({
onPress,
someProp,
}: {
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Firing an event on a disabled element (e.g. a `Pressable` with
`disabled={true}`) silently triggers no handler, which is confusing when
debugging tests. Emit a warning in that case, reusing `computeAriaDisabled`
for detection and the existing `logger`.
- Gated on no handler being found, so events that bubble to an enabled
parent do not warn.
- Scoped to disabled state only; `pointerEvents="none"` and `TextInput`
editability are intentionally excluded to avoid false positives.
- Opt-out via `configure({ disabledEventWarning: false })`; on by default.
Closes callstack#1718 (fireEvent scope; userEvent is a follow-up).
0949859 to
42ca90d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new TextInput exclusion in the warning logic is broader than the PR description’s intent and may incorrectly suppress warnings for truly disabled TextInput elements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| // `TextInput` editability (`editable={false}`) is a separate concern from | ||
| // disabled state, so we don't warn about it here to avoid false positives. | ||
| if (isHostTextInput(target)) { | ||
| return; | ||
| } |
|
Heads-up from my side: after opening this I realized #1726 already resolves #1717/#1718 with a fuller design — a unified I'd rather help land your approach. Happy to do whichever is most useful:
Just let me know what you'd prefer. If you'd welcome the help on #1726, I'm glad to put the work in. |
Summary
Closes #1718.
Firing an event on a disabled element (e.g. a
Pressablewithdisabled={true}) currently triggers no handler silently, which is confusing when debugging why a test "does nothing". This adds a warning in that case, opt-out viaconfigure({ disabledEventWarning: false }).Implementation
computeAriaDisabledhelper (the one behindtoBeDisabled) and emits through the existinglogger.pointerEvents="none"does not warn (it isn't a disabled state).TextInputeditability (editable={false}) is intentionally excluded — it's a distinct concept from disabled state, and warning onchangeText/focusthere produced false positives.disabledEventWarning: boolean, on by default.Scope
This PR covers
fireEvent.userEventuses a separate dispatch path (user-event/press) with its own disabled handling, so I've left it out here — happy to follow up with a matching change foruserEventif you're good with this approach.Open question
Per the discussion on #1718: I've defaulted the warning to on (opt-out), matching the leaning in the thread. Happy to switch it to opt-in for the first release if you'd prefer.
Testing
fire-event.test.tsx: warns on disabled element, does not warn when bubbling to an enabled parent, does not warn forpointerEvents="none", and is silenced byconfigure({ disabledEventWarning: false }).configure()default-config assertion and documented the option inconfig.mdx.yarn typecheck,yarn lint,yarn format:check, and the fullyarn testsuite (817 tests) all pass.