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 PopupMenu doesn't respect its ScrollContainer's margins #87462

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Jan 22, 2024

Fixes #87461

Removed the MarginContainer and applied the PopupMenu panel style to the ScrollContainer.

This results in the same old behavior, instead of applying the panel's content margins to the margin container and drawing the panel style manually.

Removing the include of the margin container header file has resulted in errors in 8 other files which was depending on it, so I have included the header in each one of them.

@WhalesState WhalesState requested a review from a team as a code owner January 22, 2024 05:36
@Rubonnek Rubonnek added bug topic:gui cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release enhancement labels Jan 22, 2024
@Rubonnek Rubonnek added this to the 4.3 milestone Jan 22, 2024
@YuriSizov
Copy link
Contributor

Why take the stylebox specifically into account, and not just the minimum size of the node itself, which should already include the stylebox?

@WhalesState
Copy link
Contributor Author

WhalesState commented Jan 22, 2024

Why take the stylebox specifically into account, and not just the minimum size of the node itself, which should already include the stylebox?

I didn't expect this question! but the answer is in the underlying line, why it's taking in mind the VScrollBar width ? because it's already calculated in the ScrollContainer minsize when it appears!
I don't know the answer! but yes it seems that it ignores the ScrollContainer's child minsize. but maybe because the ScrollContainer doesn't calculate minsize properly until we disable both vertical and horizontal scrolling.

I don't know what's the best way to fix this issue, but my project needs some margins by default for the ScrollContainer and I'm using a theme, so it do override the PopupMenu's ScrollContainer margins too and all the popups appears with the VerticalSlider visible, this was the only issue since everything else is fine.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 25, 2024

I think the reason is that PopupMenu already has its own panel style and it's not expected that ScrollContainer will have another one.
Either the minimum size should be max of internal panel size and ScrollContainer minimum size or (better), the internal ScrollContainer should have an override for its panel style to force-disable it, because it's redundant.

@WhalesState
Copy link
Contributor Author

WhalesState commented Jan 30, 2024

or (better), the internal ScrollContainer should have an override for its panel style to force-disable it, because it's redundant

I'm trying to do this without creating a new StyleBoxEmpty in every instance. but I don't know how. all I need is just one static empty style box. if creating a new one in every instance is fine just tell me.

The best fix is to override the ScrollContainer's panel style box with the PopupMenu panel instead of drawing it manually. also this may result in removing the MarginContainer from the node since it can be replaced with the panel content margins.

@WhalesState WhalesState marked this pull request as ready for review January 30, 2024 09:15
@WhalesState WhalesState requested review from a team as code owners January 30, 2024 09:15
scene/gui/color_picker.h Outdated Show resolved Hide resolved
KoBeWi
KoBeWi previously approved these changes Feb 16, 2024
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.

Left some comments, otherwise looks fine.

@akien-mga akien-mga changed the title Fix PopupMenu doesn't respect it's ScrollContainer's margins Fix PopupMenu doesn't respect its ScrollContainer's margins Feb 16, 2024
@WhalesState
Copy link
Contributor Author

WhalesState commented Feb 17, 2024

while testing i have found some bugs related to items and size and it took me a long time to make sure that the popup menu will work fine when changing the item's v_separation and the panel_style's content margins.

I have noticed that the ScrollContainer is no longer drawing the scroll over it's children, and it adds the margins automatically for the Scroll Bar, so I have removed the extra space used for the Vertical Scroll Bar since it will be added automatically when it's visible.

After applying the stylebox directly to the ScrollContainer and testing with large v_separation value to force the scroll to appear, i have noticed some issues and i have changed the calculactions to take into account the new items size with v_separation and the correct mouse hover while taking into account the panel's top margin.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 17, 2024

Thanks for reminding, I actually forgot to test the scrollbar 🤦‍♂️
And it made me find 2 regressions:

  • the ScrollBar can't be clicked. Clicking it will activate an item under it (can be tested with language list in editor settings)
  • when opening menu at the edge of the screen, the scrollbar goes outside the screen (can be tested with resource list in the inspector)

@KoBeWi KoBeWi dismissed their stale review February 17, 2024 12:19

suddenly regressions

@WhalesState
Copy link
Contributor Author

WhalesState commented Feb 18, 2024

I have found that this issue presents in all github windows builds, but it doesn't exist when building locally without d3d12=yes so it may be an issue related to d3d 12.

Edit: my branch wasn't rebased and the issue appeared after rebasing.

  • the ScrollBar can't be clicked. Clicking it will activate an item under it (can be tested with language list in editor settings)

Fixed:

  • when opening menu at the edge of the screen, the scrollbar goes outside the screen (can be tested with resource list in the inspector)

Fixed another two issues:

  • when dragging the menu slider and releasing the mouse over an item, the item gets selected and the menu hides. on the next menu popup the slider dragging continues after popup when the mouse enters the slider's area because it didn't recieve the mouse release event.

  • when dragging the scroll bar's slider and hovering over a submenu item, the submenu appears and the scroll bar continues scrolling when mouse enters after the submenu hides.

@WhalesState
Copy link
Contributor Author

WhalesState commented Feb 20, 2024

Please have a look on this #88392 (comment)

@KoBeWi
Copy link
Member

KoBeWi commented Feb 20, 2024

Needs rebase after #86952 was reverted.

@akien-mga akien-mga merged commit 4042dca into godotengine:master Feb 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@WhalesState WhalesState deleted the popup-menu branch February 24, 2024 05:45
RadiantUwU added a commit to RadiantUwU/godot that referenced this pull request Jun 18, 2024
RadiantUwU added a commit to RadiantUwU/godot that referenced this pull request Jun 25, 2024
RadiantUwU added a commit to RadiantUwU/godot that referenced this pull request Jul 8, 2024
RadiantUwU added a commit to RadiantUwU/godot that referenced this pull request Jul 27, 2024
RadiantUwU added a commit to RadiantUwU/godot that referenced this pull request Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release enhancement topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PopupMenu doesn't respect the content margins of it's ScrollContainer's StyleBox
5 participants