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

Allow layered ScrollContainers to coexist on input handling #5118

Merged
merged 10 commits into from
Apr 22, 2022

Conversation

frenzibyte
Copy link
Member

This changes drag event handling and scroll event handling in ScrollContainers to be based on their ScrollDirection, to allow for layered scroll containers to coexist.

This however becomes a nuisance on two parts:

  • 20c1d72: In order for scroll containers to coexist, the innermost must not OnMouseDown to let the parenting scroll containers enter the mouse-down/drag input queue, for receiving drag events.

    I'm not entirely sure if there's a reason for that in the first place, checking with git blame threw me back to 4 years ago (General fixes. #316).

  • 7bff9b9: When a scroll container is horizontal, scrolling vertically will also be considered as horizontal, therefore this coexistence change cannot apply when the inner scroll container is horizontal (as done in test).

    In addition, it also results in this kinda weird UX (inner = horizontal, outer = vertical):

    CleanShot.2022-04-22.at.06.47.29.mp4

Copy link
Sponsor Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Haven't tested yet, just reading code changes

// Continue from where we currently are scrolled to.
Target = Current;

return true;
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Will need to check osu!-side to make sure nothing was relying on this behaviour, but I'm not strongly against this change if it works everywhere we expect it to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be interested to see if there's any scroll container overlayed on top of another component at all (unless wrapped in an OverlayContainer of course).

@frenzibyte
Copy link
Member Author

frenzibyte commented Apr 22, 2022

Unsure how to deal with this test:

CleanShot 2022-04-22 at 07 24 20@2x

Feels like an arbitrary choice to drag perfectly diagonally and check whether the scroll container would block the click or not, since it's going to depend on which axis is checked first in the early-guard.

@peppy
Copy link
Sponsor Member

peppy commented Apr 22, 2022

The diagonal in that test does look arbitrary. Changing to a specific direction is probably fine.

@peppy
Copy link
Sponsor Member

peppy commented Apr 22, 2022

Made some minor refactors, but looks and feels good to me. Will wait for a second opinion for merge.

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.

ScrollContainers of different directions should be able to coexist
3 participants