testing-library / user-event

🐕 Simulate user events
https://testing-library.com/user-event
MIT License
2.18k stars 245 forks source link

The `keydown.Tab` behaviour handler assumes that the event target will always be the origin for determining focus #1188

Open andrewhayward opened 8 months ago

andrewhayward commented 8 months ago

Reproduction example

https://codesandbox.io/p/devbox/lgshc5

Prerequisites

  1. Add an onKeyDown handler to a focusable node
  2. Inside the handler, change the focused node to something else
  3. Include a simulated tab keypress in a test (using user.tab() or a lower level method)

Expected behavior

The Tab keydown behaviour handler should use the currently focused element as the source for determining the tab destination.

Actual behavior

The Tab keydown behaviour handler uses the event target as the source for determining the tab destination.

User-event version

14.5.2

Environment

No response

Additional context

This is niche behaviour, but is used to create focus traps, etc, to constrain tabbing. This may well be causing #820.

andrewhayward commented 8 months ago

Having done a bit more digging, it seems like the entire dispatch/behaviour lifecycle is open to the same fragility. It's highly unlikely that the active element is going to change in most cases, but by constructing the behaviorImplementation callback before dispatching the event, there's no guarantee that the context won't have changed by the time its called.

I may well be missing something obvious, but would it be worth not passing the target initially, and instead passing it as an argument to the behaviour callback?

const behaviorImplementation = preventDefault
  ? () => {}
  : (behavior[type] as BehaviorPlugin<EventType> | undefined)?.(
      event,
      this,
    )

...

behaviourImplementation(currentTarget);

The original target would still be available through event, if it was needed.

GorenDaniel commented 5 months ago

It reproduces with the focus-trap component of Material-UI, can be used for testing. https://mui.com/base-ui/react-focus-trap/

ph-fritsche commented 3 months ago

Thanks for the report.

The default behavior is indeed determined by the event target. This is especially true for events that trigger secondary events as default behavior (e.g. click for a keydown). Moving the focus in an event handler (or even between the user input and the event dispatch in a real browser) should not lead to triggering default behavior on another element than the one rendered as focused. (event.target is not protected and might be overwritten by an event handler just like the focus might be moved. Therefore we pass it to our plugins instead of relying on the property.)

It's interesting that the default behavior for Tab – moving the focus – is then performed relative to the active element after the event handlers are executed instead of relative to the initial target.