Skip to content
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.

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

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

Open
andrewhayward opened this issue Jan 18, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@andrewhayward
Copy link

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 andrewhayward added bug Something isn't working needs assessment This needs to be looked at by a team member labels Jan 18, 2024
@andrewhayward
Copy link
Author

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
Copy link

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
Copy link
Member

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.

@ph-fritsche ph-fritsche removed the needs assessment This needs to be looked at by a team member label Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants