Expose arrowBoundaryOffset and internalize arrowSize calculation on RAC <Popover /> and <Tooltip />

[sookmax]

Closes #5917

Hello, I thought it would be nicer, in terms of DX, to add a prop with which users can adjust their arrow position, rather than exposing OverlayArrowContext as suggested in #5917 (comment).

The way this PR implements the feature relies on the fact that calculatePosition returns only one of either arrowOffsetLeft or arrowOffsetTop (not both) with a valid value. So I added a defensive test case in calculatePosition.test.ts as well.

Frankly, it's a little unintuitive which way is positive/negative when setting the props.offset, but I believe it's easy enough for the user to figure out the direction of the offset by changing it a few times.

Or maybe there's yet a better way to do this. I'm open to any suggestion from you guys!

Thanks.

✅ 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:

Stories

http://localhost:9003/?path=/story/react-aria-components--tooltip-arrow-offest-example&providerSwitcher-express=false&strict=true

Docs

• http://localhost:1234/react-aria/Popover.html#overlayarrow
• http://localhost:1234/react-aria/Tooltip.html#overlayarrow

🧢 Your Project:

[snowystinger]

Hey! We had a chance to talk about this one today. Instead of adding a prop to the OverlayArrow, we'd like to expose arrowOffsetBoundary on Popover, which already exists as a prop on usePopover
(you can see how we use it in React Spectrum's implementation here

react-spectrum/packages/@react-spectrum/overlays/src/Popover.tsx

Line 116 in a8ec966

arrowBoundaryOffset: borderRadius

)
and can see the prop description here https://react-spectrum.adobe.com/react-aria/usePopover.html#usepopover

This will allow the value to get into calculatePosition reducing the work a user has to do.

In addition, we think that we could get a ref to the OverlayArrow through

react-spectrum/packages/react-aria-components/src/Popover.tsx

Line 167 in a8ec966

<OverlayArrowContext.Provider value={{...arrowProps, placement}}>

which would allow us to automatically measure the size of the arrow, negating the need to also expose that prop from Popover.

[thorbde]

@snowystinger You mentioned exposing the prop in Popover, would it also be exposed in Tooltip?

[sookmax]

@snowystinger Thanks for the feedback! I revised the fix according to your feedback to both <Popover> and <Tooltip>. <Popover> actually has already been exposing arrowBoundaryOffset so I just added a story showcasing the usage of that and also removed props.arrowSize.

The usage of Pick<> here will properly be reflected in the docs once #5986 gets merged.

Let me know if I missed anything!

@thorbde I added the same fix to the tooltip as well!

[sookmax]

hm not sure why the test packages/@react-aria/interactions/test/useFocusVisible.test.js is failing on CI. It passes fine in my local env though 😭.

https://app.circleci.com/pipelines/github/adobe/react-spectrum/19178/workflows/a98494f0-8486-468b-b927-28aac3a1c025/jobs/278905

[snowystinger]

Looks like it just intermittently failed, that's a new one, I'll keep an eye out for it.
Thanks for the prompt changes, I'll review later today hopefully

[snowystinger]

GET_BUILD

[rspbot]

Build successful! 🎉

• View the storybook
• View the storybook-17
• View the storybook-16
• View the documentation

[snowystinger]

GET_BUILD

[rspbot]

Build successful! 🎉

• View the storybook
• View the storybook-17
• View the storybook-16
• View the documentation

[rspbot]

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

[snowystinger]

It looks like we need to do a little more clamping work as I can get them to separate, and it also affects the overall placement when I set the offsetboundary to something extreme. I think we can handle it in a follow up

it's probably not a common case. most of the time this number should be the border radius of the popover, in which case the most it'll ever be is 50% of the height where we don't need to worry about clamping yet.

Thanks for the stories and PR!

[LFDanLu]

LGTM, I agree with @snowystinger, we can handle potentially clamping the value in a followup if at all. I figure since the user is in control of the value that they can judge a good value to use for now (I figure it will usually be a small number just to partially offset/limit the arrow position from the edge)