Improves some text field behavior.

[deniz1a]

This patch improves text field behavior in game lobby, settings menu and asset browser.

[pchote]

This is a generic utility function, so shouldn't make any assumptions about the UI state. You could instead pass in a defaultFocus widget, and then give that focus if it is non-null.

[pchote]

I think the idea behind this is good, but the implementation introduces some unsettling coupling. I think a better approach would be to add a FocusPriority integer to widgets, and then have YieldFocus walk the tree and automatically focus the highest priority (> 0) widget that is visible.

[deniz1a]

OK I implemented keyboard focus management as a stack. When a widget takes focus, it adds itself on top of the stack. When it yields focus, it removes itself from the stack so the one below it becomes the current focus.

This also allows for automatic focus yielding at widget removal since ForceYieldKeyboardFocus() method is called when a widget is removed. When the map chooser window for example is closed, its focus is removed from the stack and the chat text field of the lobby automatically gets the focus. There is no need to manually yield focus at widget removal or window closure.

[phrohdoh]

This isn't a widget anymore.

[deniz1a]

I fixed the issues Phrohdoh pointed out.

[deniz1a]

Why are YieldKeyboardFocus() and TakeKeyboardFocus() virtual? Shouldn't only TryYieldKeyboardFocus() be virtual so a subclass can add conditions for yielding focus?

[Mailaender]

This patch improves text field behavior in game lobby, settings menu and asset browser.

Can you elaborate on that? What gets improved/changed?

[deniz1a]

Here's what this PR does:

• Changes keyboard focus management from a single variable to a stack. When a widget takes keyboard focus, it is added to the stack. When it yields focus, it is removed from stack and the widget below it gets the focus.
• Asset browser: The filename filter text field gets keyboard focus by default. When the escape key is pressed, text field resets (becomes empty) and the result list gets updated.
• Game lobby: When escape key is pressed, chat text field resets. Player name text field resets to unmodified name and keyboard focus is yielded back to chat field.
• Settings menu Display tab: When escape is pressed, frame limit and player name text fields reset to unmodified value and yield focus. When enter is pressed, leading and trailing white spaces are removed from player name input and the value is checked for zero length. If it is zero, text field resets to unmodified value.

[deniz1a]

Rebased.

[deniz1a]

Shouldn't these be called WindowStack and KeyboardFocusStack? Since List is a different type of collection.

[deniz1a]

I changed the focus management from stack to widget priority. @pchote was right, it's much better and simpler now.

[Mailaender]

Not really sure what has changed now. Pressing ESC seems to do the same as before and TAB cycling does nothing. I thought that was the addition here.

[deniz1a]

Pressing Esc switches text field focus? It should only remove focus and not give it to another text field... And this doesn't implement tab cycling.

I just tested it and when you press Esc in settings text field, it only removes focus. So the issue you mentioned there should be fixed. Does this not work like this for you?

[Mailaender]

I see. ESC seems to do nothing there in current bleed so this works as promised. ✅

[pchote]

This still doesn't look right: you've removed the tracking of the currently focused keyboard widget, and added tracking of the current priority (which is a different thing)...? At best this appears to walk the whole widget tree each key press to find the current widget, but at worst there may be cases where this directs input to the wrong widget.

[pchote]

Reviewers please note that this is a significant change to the core input plumbing, and it must be reviewed based on design as well as function. "works as promised" isn't sufficient criteria for merging if it hurts performance or generally muddies the engine code.

[deniz1a]

Yes, for each key press, this walks the whole widget tree but only those that are visible. Currently focused widget is the one that has the highest priority.

I think if the priorities are set correctly and widgets that are not visible have correct IsVisible() value, this wouldn't direct input to wrong widget. Since TakeKeyboardFocus() calls are not necessary now, priority is the only thing that determines focus. So there is no difference between current focus and highest visible priority...

[pchote]

The idea of having a focus priority assigned for each widget, and using this to decide what widget should be focused when you don't have an explicit assignment is good (and this was what I asked for originally). Removing the focus tracking, and instead manipulating the widget focus priorities at runtime (and then walking the widget tree for every keyboard event) is not good.

Please reintroduce TakeKeyboardFocus, make it so that the focus priorities remain constant with their default or yaml-assigned values, and then use the focus priorities to assign the default focus when opening a new window or yielding an existing focus.

[deniz1a]

OK, I'll change this like that.

[deniz1a]

When should TakeKeyboardFocus() be called?

• Mouse click on TextField and HotkeyEntry widget (Currently these are the only places where it is called)
• When OpenWindow() in Widget.cs is called
• At focus yield

What about tabs? It would also need to be called when the active tab is changed. And ingame chat widget would also need it I think...

So this would make it more complicated than it currently is. And the benefit would be caching the current focus widget and not checking widget tree at every key press. But is this worth it?

Currently, this code checks the entire Ui.Root at every key press. But if a widget is not visible, it is skipped and its children are not checked. And Ui.Root only holds one window at a time. When a new window is opened, the current one is removed from Ui.Root although it is still in WindowList. So only one window is checked at every key press.

Which one is better, checking highest priority at each key press and eliminating the use of TakeKeyboardFocus() entirely (except for mouse clicks), or adding TakeKeyboardFocus() to specific places and checking priority only when needed? The problem with the second option is determining where to add TakeKeyboardFocus().

[deniz1a]

And I just realized, in the current version, it is not necessary to call ForceYieldKeyboardFocus() at widget removal. Because it will already be removed and not appear in widget tree...

[deniz1a]

Actually, focus caching would be implemented with a separate Ui.UpdateFocus() method and not TakeKeyboardFocus(), because that is an instance method and gives focus to a specific widget. So the problem would be determining when to call Ui.UpdateFocus().

[deniz1a]

OK, current focus widget must be cached because HasKeyboardFocus property, which calls Ui.GetKeyboardFocus(), is evaluated every tick by TextFieldWidget here:
https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.Mods.Common/Widgets/TextFieldWidget.cs#L251

So, for TextField widgets, this patch in fact walks widget tree in every tick, not just at every key press!

So... how should I do this for tabs? Manually adding Ui.UpdateFocus() at every tab.OnClick() seems counter productive.

[penev92]

As I understand, this is potentially quite dangerous. Also nobody seems to be willing to review it.
At this point I don't even know what it's meant to do and what problem it solves. I also can't read all the spam above.

Can you please explain in short what this PR does, why, and how, so we can decide if it's something we actually want?

[deniz1a]

I decided to scrap the focus management idea and opened a different PR with just the text field improvements: #8680.

[deniz1a]

A priority based automatic focus management would be better but I couldn't do it in a simple way...