SelectPanel not scrolling with anchoring element

[reneexeener]

Describe the bug
SelectPanel not scrolling with anchoring element when parent containers have different overflow properties. This also happens with ActionMenu.

To Reproduce
In memex list view:

1. Go to an existing memex project (list view)
2. Click on any downward carets to open the SelectPanel
3. Start scrolling the table
4. SelectPanel does not scroll with the anchoring element that is the table cell

In bulk add side panel:

1. Go to an existing memex project and click on the + icon in the omnibar at the bottom of the page
2. Click on the repository selector to open the SelectPanel
3. Start scrolling the table
4. SelectPanel does not scroll with the repository selector button

In Project settings (implements ActionMenu):

1. Make sure you have admin permissions and go to an existing project
2. Click on the three dots on the top right corner -> Settings -> go to bottom of the page
3. Click on the button next to 'Visibility'
4. Start scrolling the page
5. ActionMenu does not scroll with the button

Expected behavior
SelectPanel and ActionMenu should scroll correctly with their anchoring elements.

Screenshots

Desktop (please complete the following information):

• OS: macOS Montery 12.4
• Browser: Chrome
• Version 103.0.5060.114

Additional context
After doing some deductions with @siddharthkp, we suspect that the scrolling problem is caused by wrapping SelectPanel or ActionMenu in nested containers with different overflow properties, which causes the getClippingRect function to find the wrong parent node (code).

[iansan5653]

I've looked into this a bit before because we see the same thing with InlineAutocomplete suggestions when you scroll the Markdown editor textarea in the side panel:

Screen.Recording.2022-07-25.at.12.35.37.PM.mov

The cause is that all of our overlays render into a Portal at the document root, so the portals all are be positioned relative to the document root. In this case, the table grid is scrolling but the document is not. The only way to fix this without changing Primer is to put a portal inside the grid and render all in-grid overlays into that portal. That would be a nightmare to maintain and we'd have to do it for every single scrollable container in the app. It's actually impossible for the Markdown editor since you can't put a portal inside of a textarea element.

Here's an example:

On the docs page, the panels do scroll with the page because the entire document scrolling:

Screen.Recording.2022-07-25.at.12.17.39.PM.mov

If we put the anchor inside a scrolling container though, the menu does not scroll with its container, though it will still scroll with the document:

Screen.Recording.2022-07-25.at.12.19.09.PM.mov

---

I think the only way to fix this would be to attach a scroll event listener to every ancestor of the anchor element and then recalculate the overlay position when any of those events fire. This seems expensive but maybe we could optimize it by bypassing React to avoid renders (ie, directly set the overlay element's style.top/style.left attributes).

[github-actions]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

[siddharthkp]

Adding it back to Unprioritized backlog

[unnatijadhav12]

can we hide scrollbar while the dropdown panel gets opened?

[github-actions]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

[siddharthkp]

• Related: https://github.com/github/primer/issues/1421

[DavidMeu]

Also related: https://github.com/github/feature-delivery-team/issues/837
(We are still trying to figure out a workaround for that so any suggestions are welcome 🙏 )

[JeffreyKozik]

👋 We are experiencing the same problem for our "Include repositories by property" dialog for rulesets: https://github.com/github/repos/issues/11324#issuecomment-2091524716

[siddharthkp]

Update: If we imitate the anchor positioning spec, it's expected that the popover moves with the anchor

Demo 1: https://anchor-polyfill.netlify.app/#anchor-positioning-popover (click Apply polyfill in the header first)
Demo 2: https://anchor-polyfill.netlify.app/#position-fallback

[lesliecdubs]

@siddharthkp does #2184 (comment) mean that the current behavior is expected? If so, what can we do to alleviate the issue when the behavior appears broken (e.g., menu doesn't scroll with anchoring element)?

@camertron shared that the PVC implementation of this component makes the page inert, which means the page should not scroll when the menu is open.

[siddharthkp]

@siddharthkp does #2184 (comment) mean that the current behavior is expected?

Oh, not at all. I meant if, in the future, we adopt the anchored position spec or the polyfill before it's in all browsers, that is how it would work. We do not have that today.

@camertron shared that the PVC implementation of this component makes the page inert, which means the page should not scroll when the menu is open

That is definitely an option. If it's good with the a11y team, it's good with me!

[siddharthkp]

@maximedegreve, can I get your thoughts on this as well:

the PVC implementation of this component makes the page inert, which means the page should not scroll when the menu is open

[maximedegreve]

When the dialog is anchored it should always remain anchored to its trigger. In this case there should also be no backdrop and the page should remain scrollable. I think having to click first the background to close is acceptable but scrolling should always be enabled nevertheless especially since in some instances the SelectPanel might be displayed slightly below the viewports. Whatever we do we should ensure both the PVC and React version behave exactly the same to not cause frustration to our users.

Example:

CleanShot.2024-09-02.at.11.43.53.mp4

The above is not valid anymore if the SelectPanel is displayed as a modal variant. In this case there is a backdrop and page scrolling is locked.

[lesliecdubs]

@camertron I wanted to loop you in here as well since there is feedback for the PVC implementation of SelectPanel: that the page should not be inert (e.g., page should continue to scroll while menu is open).

[camertron]

I think we're still struggling to reach consensus on this. Here's feedback from @dipree:

1. No backdrop
2. The panel should not scroll with page, it’s not a common pattern.
3. Yes, trap focus. This has been specifically mentioned by screenreader users during testing; a focus trap makes it much easier to examine the panel. Also ack that we shouldn’t change the current behavior with regards to click-to-close. ... In Rails it requires to click to close the panel (expected) and in React the user can interact with the page still. For example, if the panel is open, and the user wants to click a link, it requires two clicks, one to close the panel, one to interact. In React it currently will do both things with one action, close the panel (apply changes) and interact with the page. It should be like in Rails.

See: https://github.slack.com/archives/GACAW0NPM/p1724278674795749