Scroll AnchoredOverlay when it's inside a scrolling child of the body

[owenniblock]

Closes https://github.com/github/primer/issues/1421

Adds the code @siddharthkp created for his PR to solve this problem. When you scroll within a parent element that isn't the main body element, the Anchor now updates.

Original functionality

A video showing a panel of buttons, the user opens an overlay showing a cat by pressing one of the buttons. When the user scrolls the panel, the overlay cat remains fixed in place.

https://github.com/primer/react/assets/4696224/81404a1d-3fbf-432b-ae9c-a2cf7618fc62

New functionality

A video showing a panel of buttons, the user opens an overlay showing a cat by pressing one of the buttons. When the user scrolls the panel, the overlay follows the button that was pressed. There's an issue shows where the overlay moves position based on the clipping of the parent rect.

https://github.com/primer/react/assets/4696224/0a162092-d1e1-4bca-bd24-eb7de5c78993

Changelog

New

This adds a new useElementObserver to observe when the rect for an element changes.

Changed

Changes useAnchoredPosition and AnchoredOverlay to use the new hook.

Removed

Rollout strategy

• [ ] Patch release
• [x] Minor release
• [ ] Major release; if selected, include a written rollout or migration plan
• [ ] None; if selected, include a brief description as to why

Testing & Reviewing

I'm not totally happy with this but couldn't see a better way to do it. I removed the instanceof Element because if broke the gatsby SSR build (Element isn't available server side). I've tried to use the solutions suggested by StackOverflow however excluding the library from the build isn't possible (we have to exclude the whole component, but then the Javascript still gets build for every other component so it still fails - I think some work on src/hooks/index.ts might resolve this but that was a rabbit hole I didn't have time to explore) and trying to only load the code client-side was proving to be fiddly and hacky given the structure of the code. In the end, I opted for the simplest solution 🤷

Do we need to worry about the increase to the size of browser.*.js?

I had a look at the weird issue whereby the overlay jumps position when the scroll takes the anchor button into a position where updatePosition returns a different orientation. We can fix this, but it's a much larger lift (and I believe this issue also exists for other overlays which don't suffer from this problem). To fix it, we'll probably need to rework useAnchoredPosition so that updatePosition is only called when the overlay is first opened, subsequent changes should move the overlay based on the original orientation of the overlay (resetting when closed).

Finally: do we want to add this functionality to AutocompleteOverlay and/or SelectPanel?

Merge checklist

• [ ] Added/updated tests
• [ ] Added/updated documentation
• [ ] Added/updated previews (Storybook)
• [ ] Changes are SSR compatible
• [ ] Tested in Chrome
• [ ] Tested in Firefox
• [ ] Tested in Safari
• [ ] Tested in Edge
• [ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: f1837c45e6bd3ee017c2ae02a9dddc916fdb4220

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions[bot]]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	138.77 KB (+22.28% 🔺)
packages/react/dist/browser.umd.js	139.16 KB (+21.9% 🔺)

[owenniblock]

The build error here might be due to the same issue as described on this StackOverflow post.

[owenniblock]

🤔 It looks like the change has surfaced an axe error where there wasn't one previously. It looks like the only axe error on the AnchoredOverlay is due to the sentinel elements being focusable as well as aria-hidden (you can find this error with a test of this page with the overlay open).

Maybe this fix has made the AnchoredOverlay open correctly when initiallyOpen={true}

UPDATE: I was wrong, because it now updates the React state, it needs to run inside act - going to try that.

[owenniblock]

Converting this back to a draft. Might put it down for now and take a look in spare moments next week.

[siddharthkp]

I'm fairly sure it can be fixed in CSS.

@keithamus That would be really cool! That would be the preference, we can treat this PR as the backup plan

My initial exploration was a quite a while ago, so I don't remember if I explored css solution or not (maybe I was only trying to extend getAnchoredPosition without changing it 🤔)

[owenniblock]

Closing, we can always reopen if the CSS investigation doesn't bear fruit.

[siddharthkp]

Hi again!

Does anyone have this on their list? Don't want to completely drop this either.

[owenniblock]

Not on my list @siddharthkp