TextInput#24929
Conversation
* FeathersLazyMenu * FeathersMenuToolButton
| pub enum TextInput { | ||
| /// Text input functions normally | ||
| #[default] | ||
| Editable, | ||
| /// Cursor movement, selection, and copy to clipboard is still enabled, but no mutations are allowed | ||
| ReadOnly, | ||
| /// Display only, all interactions disabled - this is used by number input widget when dragging. | ||
| DisplayOnly, | ||
| } |
There was a problem hiding this comment.
I don't like the variant names either. Not sure I have any better suggestions though, probably not worth fussing about.
| #[require(EditableText)] | ||
| #[require(AccessibilityNode(accesskit::Node::new(Role::TextInput)))] | ||
| #[reflect(Component)] | ||
| pub enum TextInput { |
There was a problem hiding this comment.
I think this should be called something like TextInputMode or TextInputInteractionState. The name TextInput suggests that this is the primary text input component. But it's not part of the user-facing API, it's used for tracking the input's interaction state internally.
There was a problem hiding this comment.
OK so here's the problem I have: this component does more than just control the readonly mode - it also pulls in the AccessibilityNode required component. If I were to rename this to ReadWriteMode as we discussed, then this required component doesn't make sense, and I'd have to move it to EditableText. But I'm not sure if bevy_text should be pulling in accessibility - it depends on whether you consider EditableText to be a widget, or just a buffer.
Worse, if this truly is just a mode switch and not a "widget" then it probably should be optional, which means I definitely can't put the accessibility node required component here.
|
|
||
| let mut queue_edit = |edit| { | ||
| if keyboard_input.input.state.is_pressed() { | ||
| let mut queue_edit = |edit, readonly| { |
There was a problem hiding this comment.
The readonly param feels a bit error prone. It'd be clearer with two functions queue_edit and queue_read_only or something but it'd need to use a RefCell or macro.
There was a problem hiding this comment.
Maybe we could have a separate function that returns true if a TextEdit is destructive that we can filter by in queue_edit?
There was a problem hiding this comment.
Seems fine, agree with the overall direction. I think maybe instead of the bool param on queue_edit, we could implement an is_destructive method for TextEdit that returns true for destructive TextEdit s.
Also I haven't thought it through properly but possibly there could be problems with IME if the widget is changed to readonly during composition? I'm not sure we should worry too much though.
| if *text_input == TextInput::Editable | ||
| || (readonly && *text_input == TextInput::ReadOnly) | ||
| && keyboard_input.input.state.is_pressed() | ||
| { |
There was a problem hiding this comment.
It doesn't check is_pressed in TextInput::Editable mode. It doesn't double enter characters because the
if let Some(text) = &keyboard_input.input.text
guard fails, but everything else like cursor and delete inputs are applied on release as well as press now.
| if *text_input == TextInput::Editable | |
| || (readonly && *text_input == TextInput::ReadOnly) | |
| && keyboard_input.input.state.is_pressed() | |
| { | |
| if keyboard_input.input.state.is_pressed() | |
| && (*text_input == TextInput::Editable | |
| || (readonly && *text_input == TextInput::ReadOnly)) | |
| { |
| (COMMAND, Key::Character(c)) if c.eq_ignore_ascii_case("x") => queue_edit(TextEdit::Cut), | ||
| (COMMAND, Key::Character(c)) if c.eq_ignore_ascii_case("v") => { | ||
| queue_edit(TextEdit::Paste); | ||
| queue_edit(TextEdit::Paste, true); |
There was a problem hiding this comment.
| queue_edit(TextEdit::Paste, true); | |
| queue_edit(TextEdit::Paste, false); |
|
|
||
| if *text_input != TextInput::Editable { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Need to the drain IME reader the same as with the other guards.
| } | |
| if *text_input != TextInput::Editable { | |
| // Still need to drain the reader to prevent stale events on next focus. | |
| ime_reader.read().for_each(drop); | |
| return; | |
| } |
Maybe it would be more robust if we consolidated the three guards and just drain one time.
There was a problem hiding this comment.
I thought about this, but even though they have the same effect, the comments in the blocks are different.
| return; | ||
| }; | ||
|
|
||
| if *text_input == TextInput::DisplayOnly { |
There was a problem hiding this comment.
| if *text_input == TextInput::DisplayOnly { | |
| if *text_input != TextInput::Editable { |
| /// System that enables or disables IME on the primary window based on whether the focused entity | ||
| /// is an [`EditableText`]. | ||
| /// | ||
| /// IME is enabled when an `EditableText` gains focus and disabled when focus moves elsewhere. |
There was a problem hiding this comment.
Needs to be updated to add that IME state also depends on TextInput now.
| mut editable_text_query: Query<&mut EditableText, With<TextInput>>, | ||
| ) { | ||
| if let Ok(mut editable_text) = editable_text_query.get_mut(trigger.entity) { | ||
| editable_text.queue_edit(TextEdit::clear_ime_compose()); |
There was a problem hiding this comment.
We also need to queue TextEdit::clear_ime_compose() somewhere when TextInput changes away from Editable.
Edit: Added it to the suggested changes for listen_for_ime_input_when_text_input_focused.
| #[default] | ||
| Editable, | ||
| /// Cursor movement, selection, and copy to clipboard is still enabled, but no mutations are allowed | ||
| ReadOnly, |
There was a problem hiding this comment.
Maybe:
| ReadOnly, | |
| NavigableOnly, |
There was a problem hiding this comment.
One reason for preferring ReadOnly is that it's consistent with the behavior of the HTML readonly attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Attributes/readonly
| /// Cursor movement, selection, and copy to clipboard is still enabled, but no mutations are allowed | ||
| ReadOnly, | ||
| /// Display only, all interactions disabled - this is used by number input widget when dragging. | ||
| DisplayOnly, |
There was a problem hiding this comment.
Then this could be NonInteractable or something.
There was a problem hiding this comment.
I also thought about Static
Co-authored-by: ickshonpe <david.curthoys@googlemail.com>
This PR splits the functionality of the
EditableTextcomponent into two separate components:EditableText(which lives in bevy_text) andTextInput(which lives in bevy_ui_widgets).See release notes for more details.