Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AnchoredOverlay + useAnchoredPosition: close overlay when menu goes out of view #2200

Closed
wants to merge 16 commits into from

Conversation

siddharthkp
Copy link
Contributor

@siddharthkp siddharthkp commented Jul 28, 2022

Changes

  1. Adds an optional argument to useAnchoredPosition to observe changes to anchor element: observe
  2. AnchoredPosition now uses useAnchoredPosition with observe which means overlay re-positions based on changes to anchor's position (scroll, resize, etc)
  3. AnchoredOverlay closes when anchor goes out of view (includes ActionMenu and SelectPanel, but not Autocomplete)

video:

overlay-goes-outside.mov

Notes for reviewer:

  1. Have kept the change in primer/react and not upstreamed to primer/behaviours because I'm not sure about the long term tradeoffs yet.
  2. Using reach/observe-rect which calls getBoundingRect on every requestAnimationFrame. Added 2 performance optimisations:
    2.1 Observer is only initiated when overlay is open
    2.2 Callback is debounced (16ms) to make sure we're not updating state too fast
  3. Considered alternate approach of adding scroll listeners on every parent up the tree, but decided against it because it attaches events on application elements

Checklist:

  • Check if new dependency works across build systems (library has support for cjs, esm and umd)
  • Is this an okay way to load lodash?
  • Check if there is a performance regression because of observer (using debounce but need to test with memex)
  • Check if this approach is accessible (a11y discussion on slack for github staff)
  • Check if the type changes make sense for OnCloseGesture (maybe remove them from this PR)
  • Check if Autocomplete should also add observe with useAnchoredPosition

@changeset-bot
Copy link

changeset-bot bot commented Jul 28, 2022

🦋 Changeset detected

Latest commit: 3505214

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

@siddharthkp siddharthkp added the skip changeset This change does not need a changelog label Jul 28, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 28, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 101.69 KB (+33.25% 🔺)
dist/browser.umd.js 102.1 KB (+32.72% 🔺)

@siddharthkp siddharthkp temporarily deployed to github-pages July 28, 2022 13:17 Inactive
@siddharthkp siddharthkp temporarily deployed to github-pages July 28, 2022 14:05 Inactive
@siddharthkp siddharthkp changed the title Add repro for #2184 Add repro and potential fix for #2184 Jul 28, 2022
@siddharthkp siddharthkp self-assigned this Jul 29, 2022
@siddharthkp siddharthkp added behaviors react bug Something isn't working labels Jul 29, 2022
@siddharthkp siddharthkp temporarily deployed to github-pages August 25, 2022 13:44 Inactive
@siddharthkp siddharthkp temporarily deployed to github-pages August 26, 2022 11:07 Inactive
@siddharthkp siddharthkp temporarily deployed to github-pages August 26, 2022 13:20 Inactive
@siddharthkp siddharthkp changed the title Add repro and potential fix for #2184 useAnchoredPosition: Floating element should follow anchor out of bounds Aug 26, 2022
@siddharthkp siddharthkp marked this pull request as ready for review August 26, 2022 14:37
@siddharthkp siddharthkp requested a review from a team as a code owner August 26, 2022 14:37
@siddharthkp siddharthkp marked this pull request as draft August 26, 2022 14:37
@siddharthkp siddharthkp removed the request for review from mperrotti August 26, 2022 14:37
@siddharthkp siddharthkp temporarily deployed to github-pages August 29, 2022 15:05 Inactive
@mperrotti mperrotti self-requested a review August 29, 2022 15:44
@siddharthkp siddharthkp removed bug Something isn't working skip changeset This change does not need a changelog behaviors labels Aug 31, 2022
@siddharthkp siddharthkp changed the title useAnchoredPosition: Floating element should follow anchor out of bounds AnchoredOverlay + useAnchoredPosition: close overlay when menu goes out of view Aug 31, 2022
@owenniblock
Copy link
Contributor

@inkblotty
Copy link
Contributor

👋 Hi @siddharthkp - Would you cc @primer/accessibility-reviewers when this is out of Draft? 🙏

@siddharthkp siddharthkp marked this pull request as ready for review September 12, 2022 09:31
@siddharthkp
Copy link
Contributor Author

siddharthkp commented Sep 12, 2022

cc @primer/accessibility-reviewers for review :)

Context: In the tree view in memex, when you interact with a table cell, it opens a select panel and we allow users to scroll the table while select panel is open.

While scrolling, when the table cell that opened the select panel goes out of view, i'd like to close the dialog automatically, but I'm not sure what are the accessibility concerns of this approach

if you'd like to see a running example in memex, it's live here: https://github-9b5cf0a362-12535706.drafts.github.io/orgs/app/projects/1

@smockle
Copy link
Member

smockle commented Sep 12, 2022

cc @primer/accessibility-reviewers for review 🙂

@siddharthkp I’m not sure this is an “accessibility” issue per se, but I noticed some unexpected behavior related to scroll-bouncing while I was testing this, so I made a screen recording. I’m using Safari on macOS. When I overscroll the list of rows, I briefly see several empty rows, then scroll position snaps/bounces back. Sometimes, this means a cell can briefly go offscreen but then quickly return on-screen. When that cell has an open menu, the menu is closed, even if the cell is again visible after the scrolling+bouncing is complete.

Caption: Screen recording of an open menu closing after its associated cell bounces back into view post-scrolling:

Menu.closing.when.cell.is.brought.back.into.view.post-bounce.mov

@smockle
Copy link
Member

smockle commented Sep 12, 2022

Other menus on github.com remain open as the page is scrolled, even if they are scrolled out of view. Here is the issue comment menu, for example (screen recording below). I’d expect all menus to behave consistently.

Caption: Screen recording showing that the issue comment three-dot menu remains open even after it scrolls out-of-view:

Issue.comment.three-dot.menu.remains.open.if.it.scrolls.out.of.view.mov

@siddharthkp
Copy link
Contributor Author

siddharthkp commented Sep 13, 2022

Other menus on github.com remain open as the page is scrolled, even if they are scrolled out of view. Here is the issue comment menu, for example (screen recording below). I’d expect all menus to behave consistently.

Yes! that was my concern as well. The suggestion to close it came after my first iteration where it stays open:

Description: There is a sticky header above the table so the select panel awkwardly floats on when the table cell is "tucked under" the header.

wutt.mov

(in the video, ignore the select panel jumping around, that's not the recommendation)

One solution would be to "tuck in" the select panel under sticky header as well. From an implementation point of view, that might be tricky or finicky because the select panel needs to render in a portal "above" the table but below the header.

@lesliecdubs
Copy link
Member

👋🏻 @siddharthkp just doing some triage on the PRC board and this one has been in "ready for review" for a while. Is this still active?

@siddharthkp
Copy link
Contributor Author

siddharthkp commented Oct 11, 2022

Is this still active?

We still need it 😅 but I haven't touched this in few weeks since floating it for a11y feedback. Will have to schedule this to a different time, going to convert to draft for now

@siddharthkp siddharthkp marked this pull request as draft October 11, 2022 13:04
@github-actions
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SelectPanel not scrolling with anchoring element
6 participants