[react-aria-components] Tabs' onSelectionChange prop doesn't fire correctly when using tabs as links

[neefrehman]

Provide a general summary of the issue here

After doing a Tabs implementation where we are using each Tab as a link, per the docs, I've noticed that onSelectionChange doesn't fire correctly.

🤔 Expected Behavior?

• onSelectionChange should fire with the current key
• onSelectionChange should fire on keyboard navigation between tabs

😯 Current Behavior

• onSelectionChange is firing with the previous key
• onSelectionChange is not firing on keyboard navigation

💁 Possible Solution

No response

🔦 Context

I've been adding analytics functionality to our implementation of Tabs, that should track each tab change a user makes. To make it easier for our product/analytics people to use the results in their dashboards and reports, the tracking should include the tab's text value, as opposed to it's key. To do this without using useTabList—because we would like to keep the rest of what Tabs offers for 'free'—I am having to use a sub-component that can access TabListState, which we can then use to create the tracking function and pass it back to Tabs via state.

const UpdateTabTrackingFunction = ({ setTrackTabChange }) => {
  const tabListState = useContext(TabListStateContext);
  useEffect(() => {
    setTrackTabChange(() => (key) => {
      track('TabChange', tabListState?.collection?.getItem(key)?.textValue);
    });
  }, [tabListState?.collection]); // eslint-disable-line react-hooks/exhaustive-deps
  return null;
};

const CustomTabs = (props: ComponentPropsWithoutRef<typeof Tabs>) => {
  const [trackTabChange, setTrackTabChange] = useState(null);

  const onSelectionChange = (key) => {
    props.onSelectionChange?.(key);
    trackTabChange?.(key);
  };

  return (
    <Tabs {...props} onSelectionChange={onSelectionChange}>
      <>
        <UpdateTabTrackingFunction setTrackTabChange={setTrackTabChange} />
        {props.children}
      </>
    </Tabs>
  );
};

It's a little ugly, but does the job! I should also note that it would be really nice to have access to the full tab item in onSelectionChange, which would help us avoid this situation.

When testing this with our tabs that use links, I noticed the reported behaviour.

🖥️ Steps to Reproduce

Minimal reproduction: https://codesandbox.io/p/sandbox/react-aria-components-tabs-onselectionchange-bug-cv6wh2?file=%2Fapp%2Fpage.js%3A7%2C21

Version

1.0.0-beta-1

What browsers are you seeing the problem on?

Firefox, Chrome, Safari

If other, please specify.

No response

What operating system are you using?

MacOS

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

[snowystinger]

I'm not sure why, but the codesandbox won't open for me.
Could you try an id other than 'all'? like 'foo' or anything else, I'm curious if we're just running into a special word which we use in collections to signify selecting all.

[neefrehman]

@snowystinger sorry about that, I had the default port misconfigured. If you try again now it should work.

Thanks also for the suggestion! I've just tried a UUID for the id but there is no change.

[snowystinger]

Thanks, it's still not loading for me. However, I revisited our docs for Tabs and Links because I remembered Links don't participate in selection for things like Menus, so I figured it was similar here. We have an example of how to synchronize the selected key and the link events https://react-spectrum.adobe.com/react-spectrum/Tabs.html#links

Can you give this a try and see if it works for you?

[neefrehman]

@snowystinger that's curious! is the browser in the sandbox running port 3000? Potentially this link will work to access the output: https://cv6wh2-3000.csb.app/

I've tried things as they are are in the example you linked, and sadly the outcome is the same. I just used search parameters in the repro's case to make the surface area narrower. Just to be clear as well, the synchronisation between the URL and selectedKey is working totally fine. It's onSelectionChange that seems to be misbehaving.

[snowystinger]

I think we might need to add a little clarification to our docs. As I mentioned earlier, links in collections do not participate in selection changes. So this is working as intended.

Just to get a little more information though to inform what we add to the docs. If you can currently handle the synchronization, what do you need onSelectionChange for?

[neefrehman]

@snowystinger ah I see what you we're saying earlier now, sorry that I didn't clock your meaning initially.

So this is working as intended.

Based on what you are saying, I still think that there may be a bug. It sounds like onSelectionChange shouldn't fire at all if it doesn't take part in selection changes, but based on my observations it still is firing, just only on click events and with the previous key. I initially read into this observation as there being a bug, and that it should have the same behaviour as non-link based Tabs.

Just to get a little more information though to inform what we add to the docs. If you can currently handle the synchronization, what do you need onSelectionChange for?

We are using onSelectionChange for product analytics purposes, so we can track user journeys properly, on pages that use tabs. In our app there are multiple ways to get to a specific tab, as we use deeplinks extensively, so knowing which interaction took a user to a tab is valuable. There are more details in the 'Context' section of the PR description, but let me know if anything needs to be clearer.

[snowystinger]

Thanks, firing with the previous key does indeed seem a bit unexpected. I'll add our bug label.
On another note, I was able to get the codesandbox running. I think I just had a network problem earlier. So I was able to observe the behavior as well.

As for analytics, you could probably listen for some pointer/keyboard events with capturing to determine if they came from the tabs. Or attach a URL fragment?

[neefrehman]

Thanks @snowystinger!

you could probably listen for some pointer/keyboard events with capturing to determine if they came from the tabs.

I guess we'll do this. Though, we were hoping to not have to re-implement in userland part of what (we thought) the library offers us.

[snowystinger]

I looked into this a bit more. There are two places I think need some updating
We should be updating the selection here https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/selection/src/useSelectableCollection.ts#L144

And we should also find a way to work around this restriction from combobox https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/selection/src/useSelectableItem.ts#L135