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

Fix sheet losing scroll behaviour #10465

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ryan-rushton
Copy link
Contributor

@ryan-rushton ryan-rushton commented May 25, 2024

Overview

The fundamental issue here is that once we set overflow-y hidden on the container, the useResizeObserver hook we use no longer detects resize events on that contents node we use in the sheet.

This fixes the issue by splitting the boundary of what gets measured by useResizeObserver and the node we are setting overflow settings on. Specifically, we measure the node one depth lower in the tree than where we are setting the overflow behaviour.

Examples

Before

Recording.2024-05-26.095208.mp4

After

Recording.2024-05-26.095258.mp4

Fixes #10346

export function useFixOverscrollBehavior(ref: React.RefObject<HTMLElement>) {
useResizeObserver(ref, (entry) => {
const elem = entry.target as HTMLElement;
export function useFixOverscrollBehavior(...refs: React.RefObject<HTMLElement>[]) {
Copy link
Contributor Author

@ryan-rushton ryan-rushton May 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will move these to named params in an object with docs if we are happy with this approach. I didn't want to go do that refactor until we are sure of the solution, as I am still undecided how much I like it. It feels like there should be a solution which doesn't require more refs and nesting of divs but I am not sure what that is atm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not sure either. The big question is whether this still solves the original problem, which is to allow for overscroll within a sheet without it scrolling the background.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah so on my windows PC where I reproduced the issue, using the filter help, I can conform that I can scroll the entirety of the sheet and we never see the inventory screen scroll either during or when hitting the ends. When I get a chance I will test it on mac and on my iphone to ensure touch is behaving as expected.

Copy link
Contributor Author

@ryan-rushton ryan-rushton May 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, what is expected behaviour with a background scrolling element? Because I see this on both prod and this branch.

Recording.2024-05-26.131031.mp4

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is that attempting to scroll on the sheet contents should not chain to scrolling the body. This is mostly targeted at touch scrolling, but it seems like my fix doesn't even work there, now. So who knows. Maybe we should go back to body-scroll-lock libraries, if there's still a maintained one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me have a play around to see if I can get something that works everywhere. Mobile is the most important bit I think due to how disruptive this can be to the touch experience but desktop is also a nice to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm this does in fact break mobile as i can now the background behind the filter help when there is no help results.

I did a bit of a dig around and the state of overscroll behaviour and overscroll chaining is a mess and has been for a while.

@delphiactual
Copy link
Contributor

This does fix the issue I reported.

Copy link
Contributor

@bhollis bhollis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it fixes this issue without regressing the original functionality of useFixOverscrollBehavior, I'm into it.

export function useFixOverscrollBehavior(ref: React.RefObject<HTMLElement>) {
useResizeObserver(ref, (entry) => {
const elem = entry.target as HTMLElement;
export function useFixOverscrollBehavior(...refs: React.RefObject<HTMLElement>[]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not sure either. The big question is whether this still solves the original problem, which is to allow for overscroll within a sheet without it scrolling the background.

@ryan-rushton
Copy link
Contributor Author

Ok, I will do some more testing on mac a bit later

@bhollis
Copy link
Contributor

bhollis commented Jun 28, 2024

@liamdebeasi you might want to poke at this one, too. We can always go back to using a body-scroll-lock library but those had their own problems...

@ryan-rushton
Copy link
Contributor Author

@liamdebeasi and @bhollis feel free to have at it. I have been trying to figure out how to fix this using the tools we have at the moment but I am not entirely sure it is possible. As I commented on a comment here, I was digging into the state of scroll chaining in browsers and its a nightmare. I think we might be forced to choose a least bad solution

@liamdebeasi
Copy link
Contributor

@bhollis What were some of the problems with body-scroll-lock? I’m out of the office at the moment, but I’ll try to take a look at this sometime over the next week.

@bhollis
Copy link
Contributor

bhollis commented Jun 29, 2024

There were two big problems - one is that it mostly works by setting overflow: hidden on the body to prevent it scrolling, and that made the page jump up and do a bunch of weird things. The other was that it had a sort of reference-counting thing going on that meant it would sometimes fail to unlock (or unlock too early). Lastly it just wasn't maintained anymore, and it wasn't clear that modern browsers needed the same fixes it applied.

@ryan-rushton
Copy link
Contributor Author

Yeah I did a bit of digging into body-scroll-lock libraries and most of them are unmaintained these days. The most maintained one seems to be https://github.com/tuax/tua-body-scroll-lock so I would probably go with that if we go back.

@liamdebeasi
Copy link
Contributor

I spent a couple days looking into this and found a potential solution. This is a slightly modified technique that I used in Ionic to better control scrolling.

In terms of what would need to change:

  1. Set overflow: hidden on body when the first sheet is opened, and remove it when the last sheet is dismissed.
    This should avoid the scroll chaining on Chromium browsers similar to what the 3rd party libraries have been doing.

  2. Force rubber band scrolling on iOS
    WebKit (particularly on iOS) is a bit tricky to develop a solution for since it does not respect overflow: hidden on the body. To work around this, we need to force rubber band scrolling so that swiping on a non-scrollable sheet causes the rubber band effect to kick in (similar to native applications) and thus prevents scrolling from chaining underneath to the body. Attached is a video sample:

(Note: I changed the height of the sheet so you can see that the content underneath it does not scroll)

Simulator.Screen.Recording.-.iPhone.13.-.2024-07-09.at.22.11.31.mp4

This approach does require some refactors of the sheet component. In addition to implementing the CSS that triggers the rubber band effect
you'd likely need to do some platform detection to make sure it only activates on iOS. Otherwise, you'll get all sheets with a minimum of 1px scrolling which may look strange. You'd also need to change some of the position styles, which could increase the scope of the work since other styles may need to change to account for it.

Additionally, the presence of the rubber band effect seems to interfere with swiping on the sheet content to close the sheet. You'd likely need to modify some of that gesture too.


In the end, it really depends on how much effort you want to put into building and maintaining a workaround until this issue is fixed at the browser level. This approach, while performant and dependency-free, does require a fair amount of upfront work. I will note that the rubber band effect in the sheet does seem like a nice UX improvement and makes DIM feel more like a mobile app since traditional native apps have this effect too.

Happy to discuss more!

@bhollis
Copy link
Contributor

bhollis commented Jul 10, 2024

Thanks for the research! The sticking point here is really the swipe-to-dismiss behavior on sheets. In emulating native sheet components, I really want the sheet itself to drag (and eventually close) once the scrollable interior is scrolled to the top. This works perfectly in many native apps, and appears to be nearly impossible to get right using the web platform (or at least, I haven't been able to make it work consistently without other problems cropping up).

@liamdebeasi
Copy link
Contributor

Makes sense. Let me do some more digging and see if there's a good way to preserve the native sheet behavior.

@liamdebeasi
Copy link
Contributor

So I think one of the main issues here is that Framer's gesture util uses the Pointer Events API which can cause pointercancel to be fired when scrolling occurs.

With how it is in master, there's no rubber band scrolling on the sheet content, so the gesture works even when pulling down on content.

With my proposed change, pulling down on the sheet content causes a scroll, so pointercancel kicks in thus aborting the gesture. I'm not sure if there's a good way of allowing the gesture to go through while preventing scrolling using the Pointer Events API.


Ionic uses a combination of mouse events and touch events for gestures, neither of which seem to have the same cancel behavior for scrolling. One option could be to avoid pointer events and use mouse/touch events instead. https://docs-demo.ionic.io/ shows generally how the sheet would work with the gesture (Load this on an iPhone, click "Modal", then click "Open Sheet Modal". You can expand the sheet then try to swipe down to close it). What is your appetite for trying a more custom gesture implementation?

Alternatively, I can ask around with some browser vendor contacts and see if there's a recommended way of doing this with the Pointer Events API.

@bhollis
Copy link
Contributor

bhollis commented Jul 18, 2024

That's interesting, and it explains why it used to work better but got worse - as Pointer Events stabilized, I believe Framer switched entirely to them (and so did I for all of our custom gesture handling). I'm open to using mouse/touch instead (with docs explaining why!), though it's sad to see that pointer events cannot be the mythical solution to all gesture handling.

@liamdebeasi
Copy link
Contributor

Sounds good! I'll take a stab at getting this working as I have time.

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

Successfully merging this pull request may close these issues.

Sheets can lose their scroll bar (Filters Help)
4 participants