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 scroll container min size calculation #96305

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Fix scroll container min size calculation #96305

merged 1 commit into from
Sep 12, 2024

Conversation

Jordyfel
Copy link
Contributor

@Jordyfel Jordyfel commented Aug 29, 2024

The code responsible for accounting for the ScrollContainer scroll bars for it's minimum size calculation was broken, so that they would always be added, even if the scrollbar wasn't shown.

To test, create a scroll container and add a control with a minimum size (like a label with text). While vertical or horizontal scrollbars are on auto, try to resize the scroll container to be smaller than the contained control and observe minimum size not working properly on master

@Jordyfel Jordyfel requested a review from a team as a code owner August 29, 2024 23:20
@YeldhamDev YeldhamDev added this to the 4.4 milestone Aug 29, 2024
@Chaosus
Copy link
Member

Chaosus commented Aug 30, 2024

To test, create a scroll container and add a control with a minimum size (like a label with text).

It would be nicer if you have attach a small video to demonstrate this bug fix.

@Jordyfel
Copy link
Contributor Author

Jordyfel commented Aug 30, 2024

Horizontal scroll auto, vertical scroll disabled, pushed to vertical min size with resize gizmo.

Before:
image
image

After:
image
image

I should also note that I assumed this is desired (and a bug since it's not working) from looking at the code

@KoBeWi
Copy link
Member

KoBeWi commented Sep 8, 2024

There is this annoying behavior with ScrollContainer where toggling scrollbar visibility will resize the content:

godot.windows.editor.dev.x86_64_6WrNaIKxfv.mp4

Maybe the intended behavior was to actually use the extra margin for content.
Or at least it could be used like that.

@Jordyfel
Copy link
Contributor Author

Jordyfel commented Sep 8, 2024

I fail to understand what is happening here. Could you upload the scene?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 8, 2024

It's a simple ScrollContainer with 2 controls:
image
When you toggle panel visibility, the ScrollBar will appear/disappear, changing content's width.

scroll_container.tscn.txt

@Jordyfel
Copy link
Contributor Author

Jordyfel commented Sep 8, 2024

I see. The alternative would be to always reserve space for the scrollbar, even when it's not visible, unless I'm missing a better way.

I don't really prefer it, I think that situations where the scrollbar is never visible will be way more that those in which the amount of elements changes just enough to show or hide it right when the user is looking at it.

Looked at some web app scrollbars (fb messenger right bar, reddit "recent posts" right bar) and they behave like that - if you resize chrome vertically the scrollbar will show and hide nudging the contents, but in practice the number of elements will almost always stay static while using the page

@KoBeWi
Copy link
Member

KoBeWi commented Sep 8, 2024

I run into this issue a lot in custom plugins or editors. It easily happens when the scrollable content is dynamic. You can even see it in the inspector:

godot.windows.editor.dev.x86_64_W9vn0uyWdc.mp4

Though I guess empty space reserved for scrollbar might look bad, so not sure what's the proper solution for that.

@Jordyfel
Copy link
Contributor Author

Jordyfel commented Sep 8, 2024

Maybe use always show for cases like this where it will change often, but change it's behavior when the contents are smaller.

Currently, always show will draw a huge scrollbar over the entire thing when the contents are smaller when the scrollbar. It would be better to instead draw the lower contrast background line only. This way the only thing that will change, when the contents become larger and smaller than the scroll container, is the scrollbar appearing and disappearing.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 10, 2024

I was just reminded that a proposal exists:
godotengine/godot-proposals#2121
I think it could be a new SCROLL_MODE. The problem is probably unrelated to this PR.

@akien-mga akien-mga merged commit 33dd105 into godotengine:master Sep 12, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Jordyfel Jordyfel deleted the scroll-container-min-size branch September 12, 2024 07:36
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.

5 participants