react icon indicating copy to clipboard operation
react copied to clipboard

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

Open siddharthkp opened this issue 3 years ago โ€ข 8 comments

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)
  • Fixes https://github.com/primer/react/issues/2184
see video

https://user-images.githubusercontent.com/1863771/187722377-b98399fd-61ce-4065-b061-207e0674e5cf.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:

  • [x] 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

siddharthkp avatar Jul 28 '22 13:07 siddharthkp

๐Ÿฆ‹ Changeset detected

Latest commit: 3505214d355fc129c3f98ca1107b7e3bab738279

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

changeset-bot[bot] avatar Jul 28 '22 13:07 changeset-bot[bot]

size-limit report ๐Ÿ“ฆ

Path Size
dist/browser.esm.js 101.69 KB (+33.25% ๐Ÿ”บ)
dist/browser.umd.js 102.1 KB (+32.72% ๐Ÿ”บ)

github-actions[bot] avatar Jul 28 '22 13:07 github-actions[bot]

๐Ÿ‘‹ Hi @siddharthkp - Would you cc @primer/accessibility-reviewers when this is out of Draft? ๐Ÿ™

inkblotty avatar Sep 08 '22 18:09 inkblotty

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

siddharthkp avatar Sep 12 '22 09:09 siddharthkp

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:

https://user-images.githubusercontent.com/3104489/189673468-5aec1f77-cec5-40a9-9cee-e4d9466040c0.mov

smockle avatar Sep 12 '22 15:09 smockle

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:

https://user-images.githubusercontent.com/3104489/189692010-67f98bba-4f1f-473c-9099-8ae086eb3aaa.mov

smockle avatar Sep 12 '22 15:09 smockle

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.

https://user-images.githubusercontent.com/1863771/189927290-f14caf6b-46dc-4983-8501-ddd7a888f28d.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.

siddharthkp avatar Sep 13 '22 14:09 siddharthkp

๐Ÿ‘‹๐Ÿป @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?

lesliecdubs avatar Oct 10 '22 23:10 lesliecdubs

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 avatar Oct 11 '22 13:10 siddharthkp

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.

github-actions[bot] avatar Jan 13 '23 17:01 github-actions[bot]