@react-aria/overlays - Fix overlay position clamping #3756

filipw01 · 2022-11-16T17:59:44Z

Closes: relates to #1297

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

Open newly created story in UseOverlayPosition story and play with different positions and offsets. More details about the issue that was fixed here #1297

🧢 Your Project:

https://www.chilipiper.com/

kwdowik · 2022-11-16T18:36:08Z

🚀

snowystinger · 2022-11-16T20:20:01Z

GET_BUILD

adobe-bot · 2022-11-16T20:31:42Z

Build successful! 🎉

snowystinger

Thanks for having a go at this!

snowystinger · 2022-11-16T20:34:18Z

packages/@react-aria/overlays/src/calculatePosition.ts

+  // button bottom touching overlay bottom
+  let bottomsTouchPosition = childOffset[crossAxis] + childOffset[crossSize] - overlaySize[crossSize];
+  // button top touching overlay top
+  let topsTouchPosition = childOffset[crossAxis];


I'm a little worried about this, but potentially I'm just not holding the math in my head correctly because I've been testing locally and it appears to be correct.

It used to be that the min/max positions had to do with the center of the target and the top/bottom of the overlay, but now they'd be in relation to the top of the target and bottom of the target with the top and bottom of the overlay respectively?

I tried doing the storybook path=/story/dialogtrigger--crossoffset-examples and it looks right. I also tried it with much larger targets (UNSAFE_style height/width on the buttons) and it also still seems correct. Can you clarify the change a bit more?

Sure, here is how it was calculated before

max = childOffset[crossAxis] + (childOffset[crossSize] / 2) - overlaySize[crossSize]

min = childOffset[crossAxis] + (childOffset[crossSize] / 2)

This is how it is calculated after my changes

topsTouch = childOffset[crossAxis]

bottoms touch = childOffset[crossAxis] + childOffset[crossSize] - overlaySize[crossSize]

And we need to do the check at runtime which position is min and max because of this case

The old solution was glued to the middle of the button if the button was at least 2 times bigger than the overlay

snowystinger · 2022-11-16T20:34:47Z

packages/@react-aria/overlays/stories/UseOverlayPosition.stories.tsx

  .add('document.body container top', () => <Trigger withPortal placement="top" />)
  .add('positioned container bottom', () => <Trigger withPortal={false} placement="bottom" />)
  .add('positioned container top', () => <Trigger withPortal={false} placement="top" />)
+  .add('buttonWidth=500 document.body bottom start', () => <Trigger withPortal buttonWidth={500} placement="bottom start" />)


can you include a screenshot of before and after? just want to know what this is verifying

This is demonstrating the case where button is bigger than the overlay

Before:

After:

filipw01 · 2022-11-17T09:58:23Z

Here is also storybook path=/story/dialogtrigger--crossoffset-examples before and after changes

Before:

After:

snowystinger

Thanks so much for drawing those up, that really helped me understand the issue and what you were doing to solve it. This makes sense to me and does seem like a change we want. I've run into it in another bit of code of I've been working on and this would solve it.

ktabors

Tested all the places where I expected issues and this works great. Thanks!
Love the diagrams!

LFDanLu

LGTM, the calculations check out to me and the stories seem to reflect that

This reverts commit e5d6994.

LFDanLu · 2022-12-14T18:26:36Z

Just a heads up, we have been discussing this new behavior amongst the team and haven't quite settled on what we want as the final behavior (in particular for the case where the overlay is smaller or equal to the trigger's size). We'll be reverting these changes for now so they won't be available in the release going out this month, but we plan on returning to this in the new year.

filipw01 · 2022-12-14T19:02:59Z

Thanks for the heads up. I was expecting the same/similar behavior to popper js, which was a dependency I removed in favor of react aria's positioning. Let me know if I can clarify anything

…#3848) This reverts commit e5d6994.

LFDanLu · 2023-01-04T19:30:27Z

@filipw01 We did some more testing and these are the two cases which we wanted to fix/discuss:

When the trigger is much larger than the overlay. Here the overlay arrow placement breaches the boundaries of the overlay if the crossOffset is set to a high enough value. When the cross offset exceeds that value, the arrow disappears entirely

When the trigger is a similar size to the overlay. In the image below the cross offset is set to large value but the overlay only shifts a small amount due to how the boundary calculations work. Perhaps the overlay should shift in such a way that the its left/right edge is flush with the center arrow

Lemme know if you have any thoughts on the above and/or are interested in digging into these cases further. If not, that is perfectly fine, the team will see if we can address these cases in the coming sprints.

filipw01 · 2023-01-09T13:04:11Z

@LFDanLu I'm still interested, I will take a closer look this week and let you know what I think about these cases

filipw01 · 2023-01-11T12:24:59Z

I think there are three main options here.

Hide the arrow when the overlay is no longer next to the middle of the trigger, like the Arrow example on floating ui's page. I think this is the closest to what was implemented in this PR. With crossOffset, you could move the overlay as far as you want
When the arrow reaches the boundary of the overlay, it starts moving together with the overlay, like the overflow prevention example on popper's page. As an accessibility-focused library, this might be a better option because you never see a detached overlay (if you use the arrow option). With crossOffset, you need at least an arrow width overlap between the trigger and overlay. If there is no arrow enabled, it can work the same as in option 1.
When the arrow reaches the overlay boundary, don't move the overlay. With crossOffset, you need at least half trigger width and half arrow width overlap between the trigger and the overlay. If there is no arrow enabled, it can work the same as in option 1.

There are also possible hybrids like

arrow follows the overlay like in option 2. but when it no longer overlaps, it is hidden like in option 1.
when the arrow reaches the overlay boundary, it is hidden like in option 1.

I don't have a preference, and I don't know how hard it is to implement each option. Let me know what you think about these approaches.

LFDanLu · 2023-01-13T22:14:02Z

@filipw01 Preliminary discussions with the team has us leaning towards option 2. One tricky thing is that we don't have the arrow's information (width, etc) available in calculatePosition. It is specified for the React Spectrum components in the css here, so we may need to move that variable to root and access it somewhere in the overlay position chain. If we don't detect that variable (aka non-RSP), we could have useOverlayPosition/calculatePosition accept a user defined option defining the arrow width.

I'll keep you posted with any further updates

…3756)" (#3848)" This reverts commit 38d44a4.

LFDanLu · 2023-02-02T18:34:27Z

@filipw01 The team has discussed this some more and would like to add an additional detail. We'd still like to go with option 2, but would like the arrow to always point to the center of the overlay if possible.

Let us know if you have any thoughts on the above

filipw01 · 2023-02-15T12:36:13Z

@LFDanLu Just so we're clear. You expect something like this, right?

LFDanLu · 2023-02-15T17:31:08Z

@filipw01 Correct!

filipw01 · 2023-02-22T11:20:49Z

@LFDanLu I opened a new PR with changes to clamping and arrow position, check it out #4097

Fix overlay position clamping

62be226

filipw01 marked this pull request as ready for review November 16, 2022 18:03

filipw01 changed the title ~~Fix overlay position clamping~~ @react-aria/overlays - Fix overlay position clamping Nov 16, 2022

Add tests and update test data

209e353

filipw01 force-pushed the main branch from b956253 to 209e353 Compare November 16, 2022 18:41

Merge branch 'main' into main

77b015e

snowystinger reviewed Nov 16, 2022

View reviewed changes

filipw01 requested a review from snowystinger November 17, 2022 11:52

Merge branch 'main' into main

11a08ca

snowystinger approved these changes Nov 18, 2022

View reviewed changes

ktabors approved these changes Nov 18, 2022

View reviewed changes

Merge branch 'main' into main

57dec70

LFDanLu approved these changes Nov 18, 2022

View reviewed changes

LFDanLu merged commit e5d6994 into adobe:main Nov 18, 2022

LFDanLu added a commit that referenced this pull request Dec 14, 2022

Revert "@react-aria/overlays - Fix overlay position clamping (#3756)"

fb97cb9

This reverts commit e5d6994.

LFDanLu added a commit that referenced this pull request Dec 14, 2022

Revert "@react-aria/overlays - Fix overlay position clamping (#3756)" (…

38d44a4

…#3848) This reverts commit e5d6994.

LFDanLu added a commit that referenced this pull request Jan 18, 2023

Revert "Revert "@react-aria/overlays - Fix overlay position clamping (#…

92a1569

…3756)" (#3848)" This reverts commit 38d44a4.

filipw01 mentioned this pull request Feb 22, 2023

@react-aria/overlays - Fix overlay position clamping and arrow position #4097

Closed

5 tasks

LFDanLu mentioned this pull request Mar 17, 2023

Fix overlay position clamping and arrow position #4220

Merged

5 tasks

@react-aria/overlays - Fix overlay position clamping #3756

@react-aria/overlays - Fix overlay position clamping #3756

Uh oh!

Conversation

filipw01 commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

kwdowik commented Nov 16, 2022

Uh oh!

snowystinger commented Nov 16, 2022

Uh oh!

adobe-bot commented Nov 16, 2022

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

snowystinger Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

filipw01 Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

filipw01 Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

filipw01 Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

snowystinger Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

filipw01 Nov 17, 2022

Choose a reason for hiding this comment

Uh oh!

filipw01 commented Nov 17, 2022

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

ktabors left a comment

Choose a reason for hiding this comment

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

LFDanLu commented Dec 14, 2022

Uh oh!

filipw01 commented Dec 14, 2022

Uh oh!

LFDanLu commented Jan 4, 2023

Uh oh!

filipw01 commented Jan 9, 2023

Uh oh!

filipw01 commented Jan 11, 2023

Uh oh!

LFDanLu commented Jan 13, 2023

Uh oh!

LFDanLu commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filipw01 commented Feb 15, 2023

Uh oh!

LFDanLu commented Feb 15, 2023

Uh oh!

filipw01 commented Feb 22, 2023

Uh oh!

Uh oh!

filipw01 commented Nov 16, 2022 •

edited

Loading

LFDanLu commented Feb 2, 2023 •

edited

Loading