Skip to content

Repository navigation

Ensure buttons don't overlap when resized mid-footer content change #39125

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.

Sign up for GitHub

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

Open
LiquidPL wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Ensure buttons don't overlap when resized mid-footer content change #39125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dcbb644
Add test case demonstrating the issue
LiquidPL Nov 2, 2026
8d2d575
Ensure buttons don't overlap when resized mid-footer content change
LiquidPL Nov 2, 2026
5b196aa
Move spacing to a constant
LiquidPL Nov 3, 2026
2a29f36
Clarify some things in comments
LiquidPL Nov 3, 2026
700de60
Merge branch 'master' into song-select-footer-ctrl-enter
LiquidPL Nov 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions osu.Game.Tests/Visual/UserInterface/TestSceneScreenFooter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,40 @@ public void TestButtonResizedAfterFooterIsDisplayed()
AddUntilStep("other buttons returned", () => ScreenFooter.ChildrenOfType<ScreenFooterButton>().Skip(1).All(b => b.ChildrenOfType<Container>().First().Y == 0));
}

[Test]
public void TestSceneButtonResizedWhenChangingScreens()
{
TestScreen screen = null!;

AddStep("push screen", () =>
{
LoadScreen(screen = new TestScreen
{
FooterContent =
{
Value = ScreenFooterContent.Argon(() => new[]
{
new ScreenFooterButton
{
AccentColour = Dependencies.Get<OsuColour>().Orange1,
Icon = FontAwesome.Solid.Toolbox,
Text = "One",
},
new ScreenFooterButton { Text = "Two", Action = () => { } },
new ScreenFooterButton { Text = "Three", Action = () => { } },
}),
},
});
});
AddUntilStep("wait until screen is loaded", () => screen.IsCurrentScreen(), () => Is.True);

AddStep("resize button and immediately push second screen", () =>
{
ScreenFooter.ChildrenOfType<ScreenFooterButton>().First().ResizeWidthTo(300, 300, Easing.OutQuint);
LoadScreen(new TestScreen());
});
}

private void contentHidden()
{
AddUntilStep("content hidden from footer", () => ScreenFooter.ChildrenOfType<TestShearedOverlayContainer.TestFooterContent>().SingleOrDefault()?.IsPresent != true);
Expand Down
60 changes: 43 additions & 17 deletions osu.Game/Screens/Footer/ScreenFooter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public partial class ScreenFooter : OverlayContainer

public const int HEIGHT = 50;

private static readonly Vector2 spacing = new Vector2(7, 0);
private const int padding = 60;
private const float delay_per_button = 30;
private const double transition_duration = 500;
Expand All @@ -55,8 +56,12 @@ public partial class ScreenFooter : OverlayContainer
private FillFlowContainer<ScreenFooterButton> leftButtonsFlow = null!;
private FillFlowContainer<ShearedButton> rightButtonsFlow = null!;
private Container overlayContentContainer = null!;
private Container<ScreenFooterButton> hiddenLeftButtonsContainer = null!;
private Container<ShearedButton> hiddenRightButtonsContainer = null!;

// Note: because these are fill flows, they will relayout the buttons moved to them
// if handled in a wrong way (for example, by inserting them into the container
// in a different order than they were placed in the original container).
private FillFlowContainer<ScreenFooterButton> hiddenLeftButtonsContainer = null!;
private FillFlowContainer<ShearedButton> hiddenRightButtonsContainer = null!;
Comment on lines +63 to +64

@bdach bdach Nov 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Of note these changes rely on the children being transferred from their original placements in exactly the same order. Any SetLayoutPosition() call on any children here probably breaks things and makes buttons move about during the hide in an undesirable manner.

This was not previously the case because of "clever" parlor trickery (fill flow does layout by setting positions on their children directly, which means said positions transfer even when the drawables are moved to a plain Container. That's also what causes the bug this is fixing, too.)

Probably no action required here, but something worth keeping notes on (and maybe adding an inline comment on).

@LiquidPL LiquidPL Nov 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FWIW, I was considering proposing a refactor of the footer code in such a fashion that effectively each update of the footer content bindable would have its own set of containers, instead of all of them sharing a single one. Something along the lines of:

  1. ScreenFooter.SetButtons() is called.
  2. ScreenFooter starts transforms for disappearing the currently present buttons in the footer, and schedules the disposal of the containers they were stored in for after all the transforms have completed. At this point nothing in ScreenFooter or elsewhere should have a reason to touch said buttons or containers.
  3. ScreenFooter creates a new set of containers, inserts them into the drawable, and proceeds with inserting the new buttons into said containers and animating their appearance as usual.
  4. The previous set of buttons disappears away, the containers get disposed alongside their contents, and we're left with only the new footer content in the footer.

This approach would effectively sidestep having to manually move the buttons into the hidden containers when changing footer content, and prevent any issues stemming from any mistakes made during such a move.

Just to be clear, I'm not trying to suggest this be implemented right now, I just wanted to ask if it makes sense, and if there's possibly some hidden complexity somewhere that would make this infeasible. Because if there is not, I think this would make the entire footer somewhat more pleasant to understand and work with.

@bdach bdach Nov 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell your proposal only talks about SetButtons() but not about RegisterActiveOverlayContainer(). The latter case is more annoying interesting because not all buttons get offscreened in that case so you can't really just ditch the old buttons - and also they need to be restored once the overlay is dismissed.

Am I missing something?

@LiquidPL LiquidPL Nov 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The way I see it, RegisterActiveOverlayContainer would stay the same as it'd be after this PR is merged. Once an overlay is shown the relevant buttons are offscreened but they still remain in the container for that screen specific footer content. Buttons would get fully offscreened and expired only when the footer content bindable changes.

The way I see things, we don't expect the base footer content to be changed while an overlay is present (aside from screen changes I suppose) so I think this should be fine.

Either way, this is a thing to be dealt with in the future, so feel free to ignore this and just focus on this PR instead.


private LogoTrackingContainer logoTrackingContainer = null!;
private IDisposable? logoTracking;
Expand Down Expand Up @@ -116,7 +121,7 @@ private void load()
Origin = Anchor.BottomLeft,
Y = ScreenFooterButton.CORNER_RADIUS,
Direction = FillDirection.Horizontal,
Spacing = new Vector2(7, 0),
Spacing = spacing,
AutoSizeAxes = Axes.Both,
},
overlayContentContainer = new Container
Expand All @@ -135,7 +140,7 @@ private void load()
Origin = Anchor.BottomRight,
Y = -OsuGame.SCREEN_EDGE_MARGIN,
Direction = FillDirection.Horizontal,
Spacing = new Vector2(7, 0),
Spacing = spacing,
AutoSizeAxes = Axes.Both,
Padding = new MarginPadding { Right = OsuGame.SCREEN_EDGE_MARGIN },
},
Expand All @@ -146,21 +151,25 @@ private void load()
Origin = Anchor.BottomLeft,
Action = onBackPressed,
},
hiddenLeftButtonsContainer = new Container<ScreenFooterButton>
hiddenLeftButtonsContainer = new FillFlowContainer<ScreenFooterButton>
{
Name = "Hidden left footer buttons",
Margin = new MarginPadding { Left = OsuGame.SCREEN_EDGE_MARGIN + ScreenBackButton.BUTTON_WIDTH + padding },
Y = ScreenFooterButton.CORNER_RADIUS,
Anchor = Anchor.BottomLeft,
Origin = Anchor.BottomLeft,
Direction = FillDirection.Horizontal,
Spacing = spacing,
AutoSizeAxes = Axes.Both,
},
hiddenRightButtonsContainer = new Container<ShearedButton>
hiddenRightButtonsContainer = new FillFlowContainer<ShearedButton>
{
Name = "Hidden right footer buttons",
Margin = new MarginPadding { Bottom = OsuGame.SCREEN_EDGE_MARGIN, Right = OsuGame.SCREEN_EDGE_MARGIN },
Anchor = Anchor.BottomRight,
Origin = Anchor.BottomRight,
Direction = FillDirection.Horizontal,
Spacing = spacing,
AutoSizeAxes = Axes.Both,
},
(logoTrackingContainer = new LogoTrackingContainer
Expand Down Expand Up @@ -302,6 +311,8 @@ public void SetButtons(IReadOnlyList<ScreenFooterButton> leftButtons, IReadOnlyL

private readonly List<ScreenFooterButton> temporarilyHiddenLeftButtons = new List<ScreenFooterButton>();

private ScheduledDelegate? autosizeRestoreDelegate;

public IDisposable RegisterActiveOverlayContainer(ShearedOverlayContainer overlay, out VisibilityContainer? overlayContent)
{
if (ActiveOverlay != null)
Expand All @@ -312,6 +323,24 @@ public IDisposable RegisterActiveOverlayContainer(ShearedOverlayContainer overla

ActiveOverlay = overlay;

// On a screen with two overlays, if the user were to hide the first, and then
// appear the second one before the transforms for reappearing the buttons were
// to finish, the register overlay handler for the second overlay would run before
// the clear handler for the second one.
// Specifically, this would mean that the autosize restore delegate for overlay #1
// could potentially run after the content for the overlay #2 has been displayed,
// resetting the autosize bypass for all buttons, and especially those that should
// be currently hidden.
//
// To avoid that, we immediately fire the autosize restore delegate if it happens
// to exist at this point in time. It has to be forced to run instead of being cancelled,
// as the set of the buttons whose autosize will be restored by it is not necessarily
// a subset of the set of buttons whose autosize will be bypassed below (i.e. a visible
// button would have its autosize bypassed when it shouldn't be, causing the overlay
// content to overlap it).
if (autosizeRestoreDelegate?.Completed == false)
autosizeRestoreDelegate.RunTask();

Debug.Assert(temporarilyHiddenLeftButtons.Count == 0);

var targetButton = leftButtonsFlow.SingleOrDefault(b => b.Overlay == overlay);
Expand All @@ -324,8 +353,13 @@ public IDisposable RegisterActiveOverlayContainer(ShearedOverlayContainer overla
{
var button = temporarilyHiddenLeftButtons[i];
button.State.Value = Visibility.Hidden;
leftButtonsFlow.Remove(button, false);
hiddenLeftButtonsContainer.Add(button);

// Temporarily bypass autosize on the X axis to prevent the buttons taking
// up space in the flow while they are replaced by the overlay content.
//
// Without this, there would be a bunch of empty space between
// the overlay content and the buttons that remain visible.
button.BypassAutoSizeAxes = Axes.X;

makeButtonDisappearToBottom(button, 0, 0, false);
}
Expand Down Expand Up @@ -369,14 +403,6 @@ private void clearActiveOverlayContainer()
{
var button = temporarilyHiddenLeftButtons[i];
button.State.Value = Visibility.Visible;
hiddenLeftButtonsContainer.Remove(button, false);

// temporarily bypass autosize on the X axis to prevent the buttons taking space
// immediately upon being moved back to the flow.
// this prevents the overlay content jumping to the right during its fade-out.
button.BypassAutoSizeAxes = Axes.X;
leftButtonsFlow.Add(button);

makeButtonAppearFromBottom(button, 0);
}

Expand All @@ -390,7 +416,7 @@ private void clearActiveOverlayContainer()

updateColourScheme(OverlayColourScheme.Aquamarine.GetHue());

Scheduler.AddDelayed(() =>
autosizeRestoreDelegate = Scheduler.AddDelayed(() =>
{
// overlay content is done displaying, re-enable autosize on all active buttons
foreach (var button in leftButtonsFlow)
Expand Down
Loading