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 ScrollBar grabber mouse input ignores scroll content margins. #98035

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Oct 9, 2024

Fixes this, the grab area now will respect the left and top content margins.

image

Also ignoring the focus style content margins and using only the scroll style's one, to prevent the grabber from changing it's position when focused.

Before:

before.mp4

After:

after.mp4

@WhalesState WhalesState requested a review from a team as a code owner October 9, 2024 20:04
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Grab area seems to be still a bit off (red area works, the rest is still click-through):

Screenshot 2024-10-10 at 09 18 01

@AThousandShips AThousandShips added this to the 4.4 milestone Oct 10, 2024
@AThousandShips AThousandShips added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Oct 10, 2024
@WhalesState
Copy link
Contributor Author

WhalesState commented Oct 10, 2024

Grab area seems to be still a bit off (red area works, the rest is still click-through):

The MRP that he has added includes expand margin for both left and right sides in the grabber, if you check it again, you will see that it's actually centered but the first and last 8 pixels can't be clicked. please test with this MRP after removing the expand margins test-scrollbar-grabber-main.zip and try to add some content margins for the scrolls.

The issue was explained in this comment #98004 (comment), The ScrollBar doesn't take the content margins into account for mouse input.

@WhalesState WhalesState requested a review from bruvzg October 11, 2024 23:35
@WhalesState WhalesState marked this pull request as draft October 18, 2024 20:20
@WhalesState WhalesState deleted the scroll-bar-grapper-area branch October 25, 2024 12:02
@AThousandShips AThousandShips added archived and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Oct 25, 2024
@AThousandShips AThousandShips removed this from the 4.4 milestone Oct 25, 2024
@Mickeon

This comment was marked as outdated.

@Zireael07

This comment was marked as outdated.

@mounirtohami mounirtohami restored the scroll-bar-grapper-area branch October 26, 2024 22:13
@mounirtohami mounirtohami deleted the scroll-bar-grapper-area branch October 26, 2024 22:14
@WhalesState WhalesState restored the scroll-bar-grapper-area branch October 26, 2024 22:17
@WhalesState WhalesState reopened this Oct 26, 2024
@WhalesState WhalesState marked this pull request as ready for review October 26, 2024 22:17
@KoBeWi KoBeWi added this to the 4.4 milestone Oct 26, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Nov 24, 2024

I don't see any difference with and without this PR. The original MRP is still broken, the new MRP works properly without the fix. It would be nice to see some project that works incorrectly on master, but is fixed by this PR.

@WhalesState
Copy link
Contributor Author

WhalesState commented Nov 26, 2024

Here it is, this will show the issue when you try to drag the scroll-bar, the scroll style just have some margins, test with the current godot version and with this PR MRP.

scroll-bar-issue.mp4

@KoBeWi
Copy link
Member

KoBeWi commented Nov 26, 2024

I confirmed that this fixes a bug, but not the linked issues.

Though there are some changes that I'm not sure about. Removing bg makes the margins of focus StyleBox ignored.

@WhalesState
Copy link
Contributor Author

WhalesState commented Nov 26, 2024

I confirmed that this fixes a bug, but not the linked issues.

Though there are some changes that I'm not sure about. Removing bg makes the margins of focus StyleBox ignored.

You can define the margins inside an empty stylebox, just test again in the current godot's dev branch with a focus style that has some content margins and you will see the grabber runs into a new position after you grab it. also we are using the scroll style by default inside input, so if i revert this change i will have to respect the focus style's margin inside the input function.

@WhalesState
Copy link
Contributor Author

WhalesState commented Nov 26, 2024

I don't see any difference with and without this PR. The original MRP is still broken, the new MRP works properly without the fix. It would be nice to see some project that works incorrectly on master, but is fixed by this PR.

This issue was mentioned in the linked issue, also i have helped him to fix the original issue. so we can close the issue after merging this PR.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Makes sense.

@WhalesState WhalesState force-pushed the scroll-bar-grapper-area branch from e068036 to 8834e9b Compare November 27, 2024 00:37
@WhalesState WhalesState force-pushed the scroll-bar-grapper-area branch from 8834e9b to 2041d8c Compare November 27, 2024 01:31
@Repiteo Repiteo merged commit f128f38 into godotengine:master Nov 27, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 27, 2024

Thanks!

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.

ScrollBar StyleBoxTexture Grabber only half usable?
7 participants