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 issues in ScrollContainer #92138

Merged
merged 1 commit into from
May 31, 2024

Conversation

YeldhamDev
Copy link
Member

  • Fix scrollbars ignoring panel margins.
  • Fix scrollbars and children not updating immediately on theme/RTL changes.

@YeldhamDev YeldhamDev added this to the 4.3 milestone May 20, 2024
@YeldhamDev YeldhamDev requested a review from a team as a code owner May 20, 2024 01:21
@KoBeWi
Copy link
Member

KoBeWi commented May 22, 2024

Fix scrollbars ignoring panel margins.

I'm not sure if this is correct:
image
Panel margins are more useful for the ScrollContainer's contents, not the scrollbars. If you want margins around scrollbars, you can wrap it in a PanelContainer.

@YeldhamDev
Copy link
Member Author

YeldhamDev commented May 23, 2024

Other Controls such as Tree have their scrolls adjust with margins, this is more due to the bigger issue that clipping can only happen at the edge of Controls, and the borders can still be useful if only one scroll is active.

Besides, the current state is even worse:
image

@KoBeWi
Copy link
Member

KoBeWi commented May 27, 2024

The current state is that the panel margin appears nicely around the contents:

godot.windows.editor.dev.x86_64_I68JYK9H5F.mp4

idk what happened in your screenshot.

@YeldhamDev
Copy link
Member Author

Have one of the sides just expand instead of giving it a large minimum size and you will see.

@KoBeWi
Copy link
Member

KoBeWi commented May 27, 2024

I'm still getting the same results. Can you share the scene?

@YeldhamDev
Copy link
Member Author

scroll-bug.zip

@KoBeWi
Copy link
Member

KoBeWi commented May 27, 2024

Ah, so it's because of the borders. If you remove the borders and instead give it a margin, it looks good.
Well, what I posted initially still looks weird, so hard to say which one is better.

@YeldhamDev
Copy link
Member Author

It's only weird if both scrolls are active, and besides, using this buggy behavior the create separation between the scrolls and content is hacky, and should be substituted which proper theme constants.

Also, fixing this allows to create a separation between the scrolls and the borders, which will be handy for themes that have scrolls with shadows that get clipped by the container.

@KoBeWi
Copy link
Member

KoBeWi commented May 27, 2024

By "weird" I mean the space between scrollbar and border. It makes the scroll float above the content.

using this buggy behavior the create separation between the scrolls and content is hacky, and should be substituted which proper theme constants.

Not sure what do you mean here. The gap is caused by your changes. I just added a content margin to the StyleBox.

I checked how your scene looks with this PR and scrollbars look better with the border, but... eh. I think perfectly the contents should be clipped inside the middle part, so it doesn't go under scrollbars and margins. That's not really possible though.

@YeldhamDev
Copy link
Member Author

By "weird" I mean the space between scrollbar and border. It makes the scroll float above the content.

That also happens without a border, so I don't know what you're getting at?

Not sure what do you mean here. The gap is caused by your changes. I just added a content margin to the StyleBox.

Talking about your previous statement:
"Panel margins are more useful for the ScrollContainer's contents, not the scrollbars. If you want margins around scrollbars, you can wrap it in a PanelContainer."

@KoBeWi
Copy link
Member

KoBeWi commented May 28, 2024

Here the scrollbar is at the border. The dark gray area is stylebox margin. It nicely wraps around the content.
image

With your changes, the margin appears behind the scrollbar, creating an ugly gap:

With border, the former case looks worse, while the latter is a bit better.
Also, looking at it again, it's not something you can achieve with a PanelContainer like I thought before.

Anyway, it's not like either case is perfect, so whatever. I'll just do a code review later.

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.

Code looks ok.

@akien-mga akien-mga merged commit 07e6d0c into godotengine:master May 31, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@YeldhamDev YeldhamDev deleted the scrollcontainer_fixes branch May 31, 2024 12:43
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.

None yet

3 participants