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

react-aria list focus conflicting with wrapping code #3785

Closed
mitchellwarr opened this issue Nov 23, 2022 · 6 comments
Closed

react-aria list focus conflicting with wrapping code #3785

mitchellwarr opened this issue Nov 23, 2022 · 6 comments
Labels
good first issue Good for newcomers

Comments

@mitchellwarr
Copy link

🐛 Bug Report

Inside the useListBox hook for react-aria, it will focus on elements for you.
However that focusing uses a querySelector that finds [data-key="key"]inside the list area.

This is the line, or one of them:

let element = scrollRef.current.querySelector(`[data-key="${manager.focusedKey}"]`) as HTMLElement;

🤔 Expected Behavior

The hook should find the element to focus on

😯 Current Behavior

If there is any other element inside this area with data-key, then it conflicts and focuses on which ever one it found first inside the DOM element.

💁 Possible Solution

A more specific search to query would be good.

In my case though I am using other react-aria items inside the list area due to keeping it all in one scroll space. But I think I will need to move things and wrap everything in a manual virtualised scroll space to fix my issue

🔦 Context

Just trying to build a list and use data-key, data-label, data-component, data-disabled, and other of these for testing purposes.

🌍 Your Environment

Software Version(s)
react-aria 3.21.0
@snowystinger
Copy link
Member

We could consider namespacing it or allowing people to specify a pattern.

Any chance you'd be willing to make a PR for it? Maybe just namespacing it to start, data-react-aria-key or something to that effect. No rush on it, we're about to go on break, so we won't get to discuss this until next week at the earliest.

@mitchellwarr
Copy link
Author

namespacing sounds good. though i suspect that will require touching a few different spaces since its the Item component that sets this (i think) and that gets used in combination of a bunch of hooks

@snowystinger
Copy link
Member

Yeah, it's definitely in a few places. Fortunately, it's easy to find with a project string search. I count about 7-8 files it's in project-wide. I think namespacing is easier as well. Trying to centralize naming it seems difficult.

@LFDanLu LFDanLu added the good first issue Good for newcomers label Nov 29, 2022
@LFDanLu
Copy link
Member

LFDanLu commented Dec 1, 2022

@mitchellwarr just wanna double check the use case here: are you running into an issue with nested lists or just that you have items in a list that you are applying a data-key attribute to? If it is nested lists, I think we'll run into an issue where the same keys may exist in both lists and thus duplicate data-key attributes could exist even if we update it to be data-react-aria-key

@mitchellwarr
Copy link
Author

mitchellwarr commented Dec 1, 2022

I have two issues. Theres one where we are using our own data-key attributes, and another where we are layering two kinds of list.

We have a scrollable space like so

|---------------|
| item1, item2  |
| item4        ||
|  ----------- ||
| X   item1    ||
| X   item2     |
|     item3     |
| X   item4     |

If that makes sense. So those item tags have data-keys for our needs and the list items have react-arias data-keys. The tags also happen to done using the aria menu implementation, meaning theyre wrapped in react-stately Item and thus are using the react-aria data-key no matter what

If i were to move the tags outside the listRef area, then i would need to wrap it in a different scrollRef anyway, which would require doing virtualised scrolling from what i can tell. So i've fixed our implementation by just doing all the focus and scrolling ourselves.

If you want to close this issue it will no longer be affecting me. But I think namespacing it is still a good idea.

@snowystinger
Copy link
Member

I believe we've done the namespacing in feat: nested collection support
Please see if this fixes the issue for you. I'm going to close for now, we can reopen if it's still a problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants