Update useLayoutEffect dependencies to ensure TabLine resizes

[tywayne]

Adds two additional dependencies to the useLayoutEffect inside of TabLine that will ensure it's width is updated properly when the text inside the tab item changes.

Closes #2004

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

I attempted to write a new test to check that the inline style for width is updated correctly, however because the value is calculated using element offsets, those values always end up at 0 in a non-browser environment. So I added a new Storybook story instead to visualize the bugfix.

Scenario 1

• View new Storybook story for Tabs -> Tab 1 controlled TabList item
• Use the text field to adjust the first tab text
• Validate the TabLine grows/shrinks as expected

Scenario 2

• View new Storybook story for Tabs -> Tab 1 controlled TabList item
• Select Tab 2
• Use the text field to adjust the first tab text
• Validate the TabLine for Tab 2 moves horizontally to maintain alignment beneath Tab 2

[snowystinger]

I think this may not update in all cases, only if we get lucky that the size update is tied to a state update. I think what we really want to do is attach a resizeObserver

An example of this changing outside of a state update is a font change

[tywayne]

Ahh, yep. I do remember seeing a comment on the issue about it not updating after a font change or FOUT type of thing, I just forgot to solve for it.

I'll take a look at the useResizeObserver util and se what I can do 👍

[tywayne]

@snowystinger Okay, this should be more resilient now. Moved the main functionality into a callback, and pass that to an observer as well as keeping a useLayoutEffect to catch the regular state changes.

[snowystinger]

Thanks, will look again soon. We're in the midst of release planning and some other things at work, so apologies if it takes a few days

[LFDanLu]

Apologies for the delay. Tested locally, behavior looks good to me, the small comment below isn't a blocking one.

My only concern was orientation: 'vertical' and whether the tab label would wrap in specific cases, but that case doesn't seem to happen with our current Tabs implementation.

[LFDanLu]

Think we can get rid of some of these extra deps since they are already in the callback's dep array

Suggested change

	}, [onResize, direction, scale, selectedKey, orientation, selectedTab?.offsetLeft]);
	}, [onResize, scale, selectedKey, selectedTab?.offsetLeft]);

[tywayne]

I don't think so—from my understanding, the useCallback deps gives us a memoized callback function with non-stale values. And the useLayoutEffect deps ensure we call that resize function any time its deps change.

[snowystinger]

@LFDanLu is correct, the onResize function will change anytime its dep array changes (as you said, it's memoized based on that dep array), which will mean that it changes in this dependency array and cause the useLayoutEffect to run

however, I have a different concern. we still have offsetLeft in this dependency array, which means we're getting lucky and render happens to be running after some change in the browser rendered dom. we should never have a dom rendered measurement as a direct reference in our dependency arrays, it's only safe to read dom values during effects. Here's an overly simplified version of why, https://codesandbox.io/s/rough-butterfly-98ny0r?file=/src/App.js

Instead, we need to be figuring out what actually changed (a resize event?) and hooking up to that to get offsetLeft into state. This will ensure a re-render and therefore the line will be placed correctly. We may need to move the state up a component in order to accomplish this. I haven't looked into it, just noticed it when I scanned the review.

[tywayne]

Ah, yep! Not sure how I looked right over onResize being in the second dep array, haha.

I think I'm following your other concern. Can take another look sometime soon, though likely not until after holiday.

[snowystinger]

That's fine, no rush. I've been trying to find time to look into it as well. Thanks for all the work you've done so far

[snowystinger]

Can probably take similar approach to font loading here e8829a8#diff-6fa8be0fd81806f4ca7cebb681f725674ae8cbebd3187fea8fd8939cac53fca7R120

[snowystinger]

I finally got some time to look at this. I managed to remove the need for our resize observer.

This doesn't address the font, but a user can do that in their implementation by waiting to render until the font is loaded. I do want to provide a utility to help with this in the future, but I think it can be handled separately from this PR.

[LFDanLu]

Works well enough, but I wonder if it would instead be sufficient to trigger an arbitrary state change when children changes and provide that state to the context in place of tabLineState. It would cause a couple of extra onResize calls down in TabLine I guess if children changed but the exact sizing of each resulting tab didn't...

[snowystinger]

Right, I thought of doing that, but this is the thing we actually care about changing. And as you point out, some children may change without changing the dimensions, so it saves us some potential extra renders.