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.

Sign up for GitHub

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

Jump to bottom

Fix sheet losing scroll behaviour #10465

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

Fix sheet losing scroll behaviour #10465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1a2d81
Fix sheet losing scroll behaviour
ryan-rushton May 25, 2024
fbe1dc9
Remove unrelated changes
ryan-rushton May 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/app/dim-ui/Sheet.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ export default function Sheet({
}) {
const sheet = useRef<HTMLDivElement>(null);
const sheetContents = useRef<HTMLDivElement | null>(null);
const childrenContainer = useRef<HTMLDivElement | null>(null);

const [frozenHeight, setFrozenHeight] = useState<number | undefined>(undefined);
const frozenHeightIntervalRef = useRef<NodeJS.Timeout | undefined>(undefined);
Expand Down Expand Up @@ -204,7 +205,7 @@ export default function Sheet({
[dragControls],
);

useFixOverscrollBehavior(sheetContents);
useFixOverscrollBehavior(sheetContents, childrenContainer);

// When drag ends we determine if the sheet should be closed either via the final
// drag velocity or if the sheet has been dragged halfway the down from its height.
Expand Down Expand Up @@ -300,7 +301,9 @@ export default function Sheet({
style={frozenHeight ? { flexBasis: frozenHeight } : undefined}
ref={sheetContents}
>
{_.isFunction(children) ? children({ onClose: triggerClose }) : children}
<div ref={childrenContainer}>
{_.isFunction(children) ? children({ onClose: triggerClose }) : children}
</div>
</div>

{Boolean(footer) && (
Expand Down
7 changes: 4 additions & 3 deletions src/app/dim-ui/useFixOverscrollBehavior.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ import useResizeObserver from '@react-hook/resize-observer';
* https://github.com/w3c/csswg-drafts/issues/3349#issuecomment-492721871 and
* https://bugs.chromium.org/p/chromium/issues/detail?id=813094
*/
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 •
edited
Loading

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 •
edited
Loading

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.

const measured = refs.length === 1 ? refs[0] : refs[1];
useResizeObserver(measured, () => {
const elem = refs[0].current!;
if (elem.scrollHeight > elem.clientHeight) {
// Scrollable contents
elem.style.overflowY = 'auto';
Expand Down
Loading