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.

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

Jump to bottom

Remove overrides of PopIn/PopOut in FocusedOverlayContainer #5834

Merged
merged 5 commits into from
Jun 12, 2023
Merged

Remove overrides of PopIn/PopOut in FocusedOverlayContainer #5834

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e5ef8c7
FocusedOverlayContainer: Don't override PopIn/Out methods
jai-x Jun 11, 2023
bc34a03
FocusedOverlayContainer: Remove redundant PopIn/Out calls in tests
jai-x Jun 11, 2023
04e4f20
Popover: Inherit from OverlayContainer and manage own focus
jai-x Jun 11, 2023
8addc18
Merge branch 'master' into focused-overlay-pop-in-out
bdach Jun 12, 2023
af7bdba
Merge branch 'master' into focused-overlay-pop-in-out
peppy Jun 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions osu.Framework.Tests/Visual/Drawables/TestSceneFocus.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,11 @@ public FocusOverlay()

protected override void PopIn()
{
base.PopIn();
stateText.Text = State.ToString();
}

protected override void PopOut()
{
base.PopOut();
stateText.Text = State.ToString();
}

Expand Down
4 changes: 0 additions & 4 deletions osu.Framework.Tests/Visual/UserInterface/TestSceneFocusedOverlayContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,16 +203,12 @@ protected override void PopIn()
{
this.FadeIn(1000, Easing.OutQuint);
this.ScaleTo(1, 1000, Easing.OutElastic);

base.PopIn();
}

protected override void PopOut()
{
this.FadeOut(1000, Easing.OutQuint);
this.ScaleTo(0.4f, 1000, Easing.OutQuint);

base.PopOut();
}
}
}
Expand Down
22 changes: 15 additions & 7 deletions osu.Framework/Graphics/Containers/FocusedOverlayContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using osu.Framework.Bindables;

namespace osu.Framework.Graphics.Containers
{
/// <summary>
Expand All @@ -12,15 +14,21 @@ public abstract partial class FocusedOverlayContainer : OverlayContainer

public override bool AcceptsFocus => State.Value == Visibility.Visible;

protected override void PopIn()
protected override void UpdateState(ValueChangedEvent<Visibility> state)
{
Schedule(() => GetContainingInputManager().TriggerFocusContention(this));
}
base.UpdateState(state);

protected override void PopOut()
{
if (HasFocus)
GetContainingInputManager().ChangeFocus(null);
switch (state.NewValue)
{
case Visibility.Hidden:
if (HasFocus)
GetContainingInputManager().ChangeFocus(null);
break;

case Visibility.Visible:
Schedule(() => GetContainingInputManager().TriggerFocusContention(this));
break;
}
}
}
}
6 changes: 5 additions & 1 deletion osu.Framework/Graphics/UserInterface/Popover.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,18 @@ namespace osu.Framework.Graphics.UserInterface
/// It typically is activated by another control and includes an arrow pointing to the location from which it emerged.
/// (loosely paraphrasing: https://developer.apple.com/design/human-interface-guidelines/ios/views/popovers/)
/// </summary>
public abstract partial class Popover : FocusedOverlayContainer
public abstract partial class Popover : OverlayContainer
{
protected override bool BlockPositionalInput => true;

public override bool ReceivePositionalInputAt(Vector2 screenSpacePos) => Body.ReceivePositionalInputAt(screenSpacePos) || Arrow.ReceivePositionalInputAt(screenSpacePos);

public override bool HandleNonPositionalInput => State.Value == Visibility.Visible;

public override bool RequestsFocus => State.Value == Visibility.Visible;

public override bool AcceptsFocus => State.Value == Visibility.Visible;

Comment on lines +22 to +33
Copy link
Member Author

@jai-x jai-x Jun 11, 2023 •
edited

Choose a reason for hiding this comment

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

This is a bit of an odd change.

Since PopOver was overriding PopIn/PopOut and FocusedOverlayContainer used those methods for automatic getting focus, it meant that PopOver would never automatically gain focus when shown.

Since moving the focus gain to UpdateState in FocusedOverlayContainer, this changed the focus behaviour of PopOver and broke tests.

The solution here is to not inherit from FocusedOverlayContainer and allow PopOver to manage it's own focus state.

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