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

Allow TextBox to specify type of text being input #6408

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
15 changes: 5 additions & 10 deletions osu.Framework.Tests/Visual/UserInterface/TestSceneTextBox.cs
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,9 @@ public void VariousTextBoxes()
TabbableContentContainer = otherTextBoxes
});

otherTextBoxes.Add(new BasicPasswordTextBox
otherTextBoxes.Add(new BasicTextBox
{
InputProperties = new TextInputProperties(TextInputType.Password, false),
PlaceholderText = @"Password textbox",
Text = "Secret ;)",
Size = new Vector2(500, 30),
Expand Down Expand Up @@ -169,12 +170,13 @@ public void VariousTextBoxes()
[Test]
public void TestNumbersOnly()
{
NumberTextBox numbers = null;
BasicTextBox numbers = null;

AddStep("add number textbox", () =>
{
textBoxes.Add(numbers = new NumberTextBox
textBoxes.Add(numbers = new BasicTextBox
{
InputProperties = new TextInputProperties(TextInputType.Number, false),
PlaceholderText = @"Only numbers",
Size = new Vector2(500, 30),
TabbableContentContainer = textBoxes
Expand Down Expand Up @@ -1042,13 +1044,6 @@ public partial class InsertableTextBox : BasicTextBox
public new void InsertString(string text) => base.InsertString(text);
}

private partial class NumberTextBox : BasicTextBox
{
protected override bool CanAddCharacter(char character) => char.IsAsciiDigit(character);

protected override bool AllowIme => false;
}

private partial class CustomTextBox : BasicTextBox
{
protected override Drawable GetDrawableCharacter(char c) => new ScalingText(c, FontSize);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ public void TestMovingOrExpandingSelectionInvokesEvent()

AddAssert("text input not deactivated", () => textInput.DeactivationQueue.Count == 0);
AddAssert("text input not activated again", () => textInput.ActivationQueue.Count == 0);
AddAssert("text input ensure activated", () => textInput.EnsureActivatedQueue.Dequeue() && textInput.EnsureActivatedQueue.Count == 0);
AddAssert("text input ensure activated", () => textInput.EnsureActivatedQueue.Dequeue() != default && textInput.EnsureActivatedQueue.Count == 0);

AddStep("click deselection", () =>
{
Expand All @@ -217,7 +217,7 @@ public void TestMovingOrExpandingSelectionInvokesEvent()

AddAssert("text input not deactivated", () => textInput.DeactivationQueue.Count == 0);
AddAssert("text input not activated again", () => textInput.ActivationQueue.Count == 0);
AddAssert("text input ensure activated", () => textInput.EnsureActivatedQueue.Dequeue() && textInput.EnsureActivatedQueue.Count == 0);
AddAssert("text input ensure activated", () => textInput.EnsureActivatedQueue.Dequeue() != default && textInput.EnsureActivatedQueue.Count == 0);

AddStep("click-drag selection", () =>
{
Expand Down Expand Up @@ -500,7 +500,7 @@ public void TestChangingFocusDoesNotReactivate(bool allowIme)

AddStep("add second textbox", () => textInputContainer.Add(secondTextBox = new EventQueuesTextBox
{
ImeAllowed = allowIme,
InputProperties = new TextInputProperties(TextInputType.Text, allowIme),
Anchor = Anchor.CentreLeft,
Origin = Anchor.CentreLeft,
CommitOnFocusLost = true,
Expand All @@ -517,7 +517,7 @@ public void TestChangingFocusDoesNotReactivate(bool allowIme)

AddAssert("text input not deactivated", () => textInput.DeactivationQueue.Count == 0);
AddAssert("text input not activated again", () => textInput.ActivationQueue.Count == 0);
AddAssert($"text input ensure activated {(allowIme ? "with" : "without")} IME", () => textInput.EnsureActivatedQueue.Dequeue() == allowIme && textInput.EnsureActivatedQueue.Count == 0);
AddAssert($"text input ensure activated {(allowIme ? "with" : "without")} IME", () => textInput.EnsureActivatedQueue.Dequeue().AllowIme == allowIme && textInput.EnsureActivatedQueue.Count == 0);

AddStep("commit text", () => InputManager.Key(Key.Enter));
AddAssert("text input deactivated", () => textInput.DeactivationQueue.Dequeue());
Expand Down Expand Up @@ -574,10 +574,6 @@ private void testNormalTextInput()

public partial class EventQueuesTextBox : TestSceneTextBox.InsertableTextBox
{
public bool ImeAllowed { get; set; } = true;

protected override bool AllowIme => ImeAllowed;

public readonly Queue<bool> InputErrorQueue = new Queue<bool>();
public readonly Queue<string> UserConsumedTextQueue = new Queue<string>();
public readonly Queue<string> UserRemovedTextQueue = new Queue<string>();
Expand Down
20 changes: 0 additions & 20 deletions osu.Framework/Graphics/UserInterface/BasicPasswordTextBox.cs

This file was deleted.

12 changes: 6 additions & 6 deletions osu.Framework/Graphics/UserInterface/DropdownSearchBar.cs
Original file line number Diff line number Diff line change
Expand Up @@ -197,20 +197,20 @@ public DropdownTextInputSource(TextInputSource platformSource, GameHost host)
platformSource.OnImeResult += TriggerImeResult;
}

protected override void ActivateTextInput(bool allowIme)
protected override void ActivateTextInput(TextInputProperties properties)
{
base.ActivateTextInput(allowIme);
base.ActivateTextInput(properties);

if (allowTextInput)
platformSource.Activate(allowIme, imeRectangle ?? RectangleF.Empty);
platformSource.Activate(properties, imeRectangle ?? RectangleF.Empty);
}

protected override void EnsureTextInputActivated(bool allowIme)
protected override void EnsureTextInputActivated(TextInputProperties properties)
{
base.EnsureTextInputActivated(allowIme);
base.EnsureTextInputActivated(properties);

if (allowTextInput)
platformSource.EnsureActivated(allowIme, imeRectangle);
platformSource.EnsureActivated(properties, imeRectangle);
}

protected override void DeactivateTextInput()
Expand Down
51 changes: 33 additions & 18 deletions osu.Framework/Graphics/UserInterface/TextBox.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,18 @@

namespace osu.Framework.Graphics.UserInterface
{
public abstract partial class TextBox : TabbableContainer, IHasCurrentValue<string>, IKeyBindingHandler<PlatformAction>
public abstract partial class TextBox : TabbableContainer, IHasCurrentValue<string>, IKeyBindingHandler<PlatformAction>, ICanSuppressKeyEventLogging
{
protected FillFlowContainer TextFlow { get; private set; }
protected Container TextContainer { get; private set; }

public override bool HandleNonPositionalInput => HasFocus;

/// <summary>
/// A character displayed whenever the type of text input set by <see cref="TextInputProperties.Type"/> is hidden.
/// </summary>
protected virtual char MaskCharacter => '*';

/// <summary>
/// Padding to be used within the TextContainer. Requires special handling due to the sideways scrolling of text content.
/// </summary>
Expand All @@ -50,12 +55,14 @@ public abstract partial class TextBox : TabbableContainer, IHasCurrentValue<stri
/// <summary>
/// Whether clipboard copying functionality is allowed.
/// </summary>
protected virtual bool AllowClipboardExport => true;
protected virtual bool AllowClipboardExport => !InputProperties.Type.IsPassword();

/// <summary>
/// Whether seeking to word boundaries is allowed.
/// </summary>
protected virtual bool AllowWordNavigation => true;
protected virtual bool AllowWordNavigation => !InputProperties.Type.IsPassword();

bool ICanSuppressKeyEventLogging.SuppressKeyEventLogging => InputProperties.Type.IsPassword();

/// <summary>
/// Represents the left/right selection coordinates of the word double clicked on when dragging.
Expand All @@ -67,17 +74,13 @@ public abstract partial class TextBox : TabbableContainer, IHasCurrentValue<stri
/// </summary>
public virtual bool HandleLeftRightArrows => true;

[Obsolete($"Use {nameof(InputProperties)} instead.")] // can be removed 20250506
protected virtual bool AllowIme => true;

/// <summary>
/// Whether to allow IME input when this text box has input focus.
/// A set of properties to consider when interacting with this <see cref="TextBox"/>.
/// </summary>
/// <remarks>
/// This is just a hint to the native implementation, some might respect this,
/// while others will ignore and always have the IME (dis)allowed.
/// </remarks>
/// <example>
/// Useful for situations where IME input is not wanted, such as for passwords, numbers, or romanised text.
/// </example>
protected virtual bool AllowIme => true;
public TextInputProperties InputProperties { get; init; }

/// <summary>
/// Check if a character can be added to this TextBox.
Expand All @@ -87,9 +90,14 @@ public abstract partial class TextBox : TabbableContainer, IHasCurrentValue<stri
protected virtual bool CanAddCharacter(char character) => true;

/// <summary>
/// Private helper for <see cref="CanAddCharacter"/>, additionally requiring that the character is not a control character.
/// Private helper for <see cref="CanAddCharacter"/>, additionally requiring that the character is not a control character and obeys <see cref="TextInputProperties.Type"/>.
/// </summary>
private bool canAddCharacter(char character) => !char.IsControl(character) && CanAddCharacter(character);
private bool canAddCharacter(char character)
{
return !char.IsControl(character)
&& (!InputProperties.Type.IsNumerical() || char.IsAsciiDigit(character))
&& CanAddCharacter(character);
}

private bool readOnly;

Expand Down Expand Up @@ -158,6 +166,10 @@ public bool ReadOnly

protected TextBox()
{
#pragma warning disable CS0618 // Type or member is obsolete
InputProperties = new TextInputProperties(TextInputType.Text, AllowIme);
#pragma warning restore CS0618 // Type or member is obsolete

Masking = true;

Children = new Drawable[]
Expand Down Expand Up @@ -789,6 +801,9 @@ private string removeCharacters(int number = 1)

protected virtual Drawable AddCharacterToFlow(char c)
{
if (InputProperties.Type.IsPassword())
c = MaskCharacter;

// Remove all characters to the right and store them in a local list,
// such that their depth can be updated.
List<Drawable> charsRight = new List<Drawable>();
Expand Down Expand Up @@ -1339,7 +1354,7 @@ protected override void OnFocusLost(FocusLostEvent e)
protected override bool OnClick(ClickEvent e)
{
if (!ReadOnly && textInputBound)
textInput.EnsureActivated(AllowIme);
textInput.EnsureActivated(InputProperties);

return !ReadOnly;
}
Expand All @@ -1366,17 +1381,17 @@ private void bindInput([CanBeNull] TextBox previous)

if (textInputBound)
{
textInput.EnsureActivated(AllowIme);
textInput.EnsureActivated(InputProperties);
return;
}

// TextBox has special handling of text input activation when focus is changed directly from one TextBox to another.
// We don't deactivate and activate, but instead keep text input active during the focus handoff, so that virtual keyboards on phones don't flicker.

if (previous?.textInput == textInput)
textInput.EnsureActivated(AllowIme, ScreenSpaceDrawQuad.AABBFloat);
textInput.EnsureActivated(InputProperties, ScreenSpaceDrawQuad.AABBFloat);
else
textInput.Activate(AllowIme, ScreenSpaceDrawQuad.AABBFloat);
textInput.Activate(InputProperties, ScreenSpaceDrawQuad.AABBFloat);

textInput.OnTextInput += handleTextInput;
textInput.OnImeComposition += handleImeComposition;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,14 @@
namespace osu.Framework.Input
{
/// <summary>
/// Marker interface which suppresses logging of keyboard input events.
/// An interface which suppresses logging of keyboard input events.
/// Useful for password fields, where user input should not be logged.
/// </summary>
public interface ISuppressKeyEventLogging
public interface ICanSuppressKeyEventLogging
{
/// <summary>
/// Whether key event logging should be suppressed for this drawable.
/// </summary>
bool SuppressKeyEventLogging { get; }
}
}
5 changes: 4 additions & 1 deletion osu.Framework/Input/InputManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1003,7 +1003,10 @@ protected virtual bool PropagateBlockableEvent(SlimReadOnlyListWrapper<Drawable>

if (shouldLog(e))
{
string detail = d is ISuppressKeyEventLogging ? e.GetType().ReadableName() : e.ToString();
string detail = d is ICanSuppressKeyEventLogging kd && kd.SuppressKeyEventLogging
? e.GetType().ReadableName()
: e.ToString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this actually... suppress? Because in debug, when I run the game, and type into a password text box, key events are still logged. And that's because they're being logged in a completely different location, namely

if (handledBy != null)
Logger.Log($"{e} handled by {handledBy}.", LoggingTarget.Runtime, LogLevel.Debug);

So does this really do anything? Or did it just break at some point and nobody noticed?

Copy link
Member Author

@frenzibyte frenzibyte Nov 9, 2024

Choose a reason for hiding this comment

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

Seems like it did regress, since key input used to be logged in this location until it was moved to KeyEventManager. I'll correct it in this PR.


Logger.Log($"{detail} handled by {d}.", LoggingTarget.Runtime, LogLevel.Debug);
}

Expand Down
8 changes: 4 additions & 4 deletions osu.Framework/Input/SDLWindowTextInput.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,16 @@ private void handleTextEditing(string? text, int selectionStart, int selectionLe
TriggerImeComposition(text, selectionStart, selectionLength);
}

protected override void ActivateTextInput(bool allowIme)
protected override void ActivateTextInput(TextInputProperties properties)
{
window.TextInput += handleTextInput;
window.TextEditing += handleTextEditing;
window.StartTextInput(allowIme);
window.StartTextInput(properties);
}

protected override void EnsureTextInputActivated(bool allowIme)
protected override void EnsureTextInputActivated(TextInputProperties properties)
{
window.StartTextInput(allowIme);
window.StartTextInput(properties);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't appear to work correctly on android. When switching from a normal textbox to a password textbox immediately, IME remains available. Conversely, when switching from a password textbox to a normal textbox immediately, IME remains unavailable.

Record_2024-11-08-11-57-01.mp4

I wonder what this looks like on iOS. (I own an iOS device now, but no mac, so I still can't check myself.)

Whether this is an SDL bug, or ours, is up for debate I guess. I can say that the naive solution, i.e.

diff --git a/osu.Framework/Input/SDLWindowTextInput.cs b/osu.Framework/Input/SDLWindowTextInput.cs
index 79fa1d848..c67bf7f77 100644
--- a/osu.Framework/Input/SDLWindowTextInput.cs
+++ b/osu.Framework/Input/SDLWindowTextInput.cs
@@ -10,6 +10,8 @@ internal class SDLWindowTextInput : TextInputSource
     {
         private readonly ISDLWindow window;
 
+        private TextInputProperties? startedInputProperties;
+
         public SDLWindowTextInput(ISDLWindow window)
         {
             this.window = window;
@@ -42,10 +44,14 @@ protected override void ActivateTextInput(TextInputProperties properties)
             window.TextInput += handleTextInput;
             window.TextEditing += handleTextEditing;
             window.StartTextInput(properties);
+            startedInputProperties = properties;
         }
 
         protected override void EnsureTextInputActivated(TextInputProperties properties)
         {
+            if (startedInputProperties != null && startedInputProperties != properties)
+                window.StopTextInput();
+
             window.StartTextInput(properties);
         }
 
@@ -54,6 +60,7 @@ protected override void DeactivateTextInput()
             window.TextInput -= handleTextInput;
             window.TextEditing -= handleTextEditing;
             window.StopTextInput();
+            startedInputProperties = null;
         }
 
         public override void SetImeRectangle(RectangleF rectangle)

does not appear to help.

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'm seeing roughly the same issue on iOS, I will have to look into libsdl-org/SDL#11406 (comment) again first to tell whether this is an issue on SDL's side or ours. Not sure about Android though, someone else will have to investigate this (libsdl-org/SDL#11406 (comment) may be relevant).

Copy link
Member Author

Choose a reason for hiding this comment

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

After further investigation on the iOS issue, SDL doesn't handle updating text input properties appropriately on iOS, regardless of whether text input is active or not. I've fixed this in the PR linked above (libsdl-org/SDL@f4d81f2) for now.

Copy link
Collaborator

@bdach bdach Nov 11, 2024

Choose a reason for hiding this comment

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

...so i guess at this point it's either up to me to fix android in sdl, or to accept it's broken over there?

i feel like my android devices might mysteriously stop working soon.

Copy link
Member

Choose a reason for hiding this comment

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

I can try to take a stab at fixing this in SDL. On first glance, it doesn't seem to be an android-specific issue as SDL will ignore requests to show the keyboard if it's already active.

Copy link
Member

Choose a reason for hiding this comment

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

}

protected override void DeactivateTextInput()
Expand Down
21 changes: 21 additions & 0 deletions osu.Framework/Input/TextInputProperties.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

namespace osu.Framework.Input
{
/// <summary>
/// Represents a number of properties to consider during a text input session.
/// </summary>
/// <param name="Type">The type of text being inputted.</param>
bdach marked this conversation as resolved.
Show resolved Hide resolved
/// <param name="AllowIme">
/// <para>
/// Whether IME should be allowed during this text input session.
/// Useful for situations where IME input is not wanted, such as for passwords, numbers, or romanised text.
/// </para>
/// <para>
/// Note that this is just a hint to the native implementation, some might respect this,
/// while others will ignore and always have the IME (dis)allowed.
/// </para>
/// </param>
public record struct TextInputProperties(TextInputType Type, bool AllowIme);
}
Loading
Loading