Skip to content

Fix menu items being clickable during fade out #6563

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

Merged
merged 1 commit into from
Apr 9, 2025
Merged

Fix menu items being clickable during fade out #6563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 35 additions & 8 deletions osu.Framework.Tests/Visual/UserInterface/TestSceneClosableMenu.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using osu.Framework.Graphics.UserInterface;
using osu.Framework.Input.Events;
using osu.Framework.Testing;
using osuTK;
using osuTK.Input;

namespace osu.Framework.Tests.Visual.UserInterface
Expand All @@ -34,14 +35,7 @@ public void SetUpSteps()
new MenuItem("Sub-item #2", () => { }),
}
},
new MenuItem("Item #2")
{
Items = new[]
{
new MenuItem("Sub-item #1"),
new MenuItem("Sub-item #2", () => { }),
}
},
new MenuItem("Item #2"),
}
});
}
Expand Down Expand Up @@ -131,6 +125,30 @@ public void TestMenuBlocksInputUnderneathIt()
AddAssert("mouse handler not activated", () => !actionReceived);
}

[Test]
public void TestItemsNotClickableDuringFadeOut()
{
bool item1Clicked = false;
bool item2Clicked = false;
bool topLevelItemClicked = false;

AddStep("set item actions", () =>
{
Menus.GetSubMenu(0).Items[0].Items[0].Action.Value = () => item1Clicked = true;
Menus.GetSubMenu(0).Items[0].Items[1].Action.Value = () => item2Clicked = true;
Menus.GetSubMenu(0).Items[1].Action.Value = () => topLevelItemClicked = true;
});

AddStep("click item", () => ClickItem(0, 0));
AddStep("click item", () => ClickItem(1, 0));
AddAssert("menu item 1 activated", () => item1Clicked);
AddStep("click item", () => ClickItem(1, 1));
AddAssert("menu item 2 not activated", () => !item2Clicked);

AddStep("click top level item", () => ClickItem(0, 1));
AddAssert("top level item not activated", () => !topLevelItemClicked);
}

private partial class MouseHandlingLayer : Drawable
{
public Action Action { get; set; }
Expand Down Expand Up @@ -161,6 +179,15 @@ protected override bool OnKeyDown(KeyDownEvent e)
return PressBlocked = base.OnKeyDown(e);
}

protected override void UpdateSize(Vector2 newSize)
{
Width = newSize.X;

// I don't know why menu size is reset to zero on closing, but let's just ignore it to make things work.
if (newSize.Y > 0)
this.ResizeHeightTo(newSize.Y, 300, Easing.OutQuint);
}

protected override void AnimateOpen() => this.FadeIn(500);

protected override void AnimateClose() => this.FadeOut(5000); // Ensure escape is pressed while menu is still fading
Expand Down
6 changes: 4 additions & 2 deletions osu.Framework/Graphics/UserInterface/Menu.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ public abstract partial class Menu : CompositeDrawable, IStateful<MenuState>
private readonly Container<Menu> submenuContainer;
private readonly LayoutValue positionLayout = new LayoutValue(Invalidation.DrawInfo | Invalidation.RequiredParentSizeToFit);

public override bool PropagatePositionalInputSubTree => State == MenuState.Open;
public override bool HandlePositionalInput => State == MenuState.Open;
Comment on lines +86 to +87
Copy link
Collaborator

Choose a reason for hiding this comment

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

While setting these will indeed prevent the other menu items from being clicked, it will also allow mouse input to fall through the context menu to whatever is behind it.

In the test you modified here, that can be exercised by

diff --git a/osu.Framework.Tests/Visual/UserInterface/TestSceneClosableMenu.cs b/osu.Framework.Tests/Visual/UserInterface/TestSceneClosableMenu.cs
index 9d7959e8b..33657ab3a 100644
--- a/osu.Framework.Tests/Visual/UserInterface/TestSceneClosableMenu.cs
+++ b/osu.Framework.Tests/Visual/UserInterface/TestSceneClosableMenu.cs
@@ -131,6 +131,7 @@ public void TestItemsNotClickableDuringFadeOut()
             bool item1Clicked = false;
             bool item2Clicked = false;
             bool topLevelItemClicked = false;
+            bool backgroundClickReceived = false;
 
             AddStep("set item actions", () =>
             {
@@ -138,15 +139,23 @@ public void TestItemsNotClickableDuringFadeOut()
                 Menus.GetSubMenu(0).Items[0].Items[1].Action.Value = () => item2Clicked = true;
                 Menus.GetSubMenu(0).Items[1].Action.Value = () => topLevelItemClicked = true;
             });
+            AddStep("add mouse handler", () => Add(new MouseHandlingLayer
+            {
+                Action = () => backgroundClickReceived = true,
+                Depth = 1,
+            }));
 
             AddStep("click item", () => ClickItem(0, 0));
             AddStep("click item", () => ClickItem(1, 0));
             AddAssert("menu item 1 activated", () => item1Clicked);
+
             AddStep("click item", () => ClickItem(1, 1));
             AddAssert("menu item 2 not activated", () => !item2Clicked);
+            AddAssert("background did not receive click", () => !backgroundClickReceived);
 
             AddStep("click top level item", () => ClickItem(0, 1));
             AddAssert("top level item not activated", () => !topLevelItemClicked);
+            AddAssert("background did not receive click", () => !backgroundClickReceived);
         }
 
         private partial class MouseHandlingLayer : Drawable

Do you see this as a problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't. At the point the menu is "closed", we can assume it's transitioning away from existence. From a user's perspective, they should be able to interact behind it (else they will have to learn that their clicks may be blocked for n milliseconds after dismissing a menu).

So I'd argue what is in this PR is more correct than trying to block during that duration.

public override bool HandleNonPositionalInput => State == MenuState.Open;

/// <summary>
/// Constructs a menu.
/// </summary>
Expand Down Expand Up @@ -627,8 +631,6 @@ private void menuItemHovered(DrawableMenuItem item)
}
}

public override bool HandleNonPositionalInput => State == MenuState.Open;

protected override bool OnKeyDown(KeyDownEvent e)
{
if (e.Key == Key.Escape && !TopLevelMenu)
Expand Down
Loading