-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Make Radio Button behaviour modular and consistent with other widgets #21294
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
base: main
Are you sure you want to change the base?
Conversation
|
||
// Trigger the on_change event for the newly checked radio button | ||
// Trigger the value change event on the radio button | ||
commands.trigger(ValueChange::<bool> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an odd way to use ValueChange
:
- If the value is always the same (
true
), then we should triggerActivate
instead.Activate
is meant for widgets whose state never changes. - Alternatively, we could trigger
ValueChange(false)
when the button gets deselected because of mutual exclusion. However, that's more complicated because now you would have to pass around the entity id of the previously checked button (which is not a problem here, but might be further down).
More generally, if we're going to useValueChange
, it should always reflect the current state of the widget triggering it: a listener that only listens toValueChange
should always know what state the source is in. We can't do this if it only sendsValueChange
for some state transitions and not others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use Activate
. But i used ValueChange
instead because all other Checkable widgets raise ValueChange event.
And for me it seemed logical to think that RadioButton can only be checked by interacting with it. To uncheck it users need to add other widgets / radio group / logic that can uncheck it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I would prefer ValueChange<bool>
because all checkable widgets use that event.
EDIT: P.S. But activate is fine with me too.
crates/bevy_ui_widgets/src/radio.rs
Outdated
break; | ||
} | ||
} | ||
let Ok(checked) = q_radio_button.get(ev.focused_entity) else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One consequence of bailing out here is that disabled radio buttons no longer consume the SPACE/ENTER keyboard event, instead the event will propagate to the ancestors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See answer to
#21294 (comment)
Currently we have different behaviour for keyboard and clicks which is probably a bug in current implementation.
I used clicks as a reference implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9806468
crates/bevy_ui_widgets/src/radio.rs
Outdated
q_parents: Query<&ChildOf>, | ||
mut commands: Commands, | ||
) { | ||
let Ok(checked) = q_radio.get(ev.entity) else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bailing out here means that a disabled radio button doesn't consume the click event, which will propagate to the ancestors. In other words, it will cause the click to trigger on whatever container entity holds the radio button, which I think is probably incorrect.
I think that disabled widgets shouldn't behave as if the widget was absent; rather, they should behave as if the widget was read-only. This means that they still receive click events, they just don't respond to them.
This is why I used Has<InteractionDisabled>
rather than Without<InteractionDisabled>
- so that I could detect clicks on disabled buttons and stop propagation on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to implement behaviour based on the current click implementation for radio group / button.
Current implementation doesn't cancel click event propagation on a disabled widget.
bevy/crates/bevy_ui_widgets/src/radio.rs
Lines 176 to 196 in eac8bc7
// Radio button is disabled. | |
if q_radio.get(radio_id).unwrap().1 { | |
return; | |
} | |
// Gather all the enabled radio group descendants for exclusion. | |
let radio_buttons = q_children | |
.iter_descendants(ev.entity) | |
.filter_map(|child_id| match q_radio.get(child_id) { | |
Ok((checked, false)) => Some((child_id, checked)), | |
Ok((_, true)) | Err(_) => None, | |
}) | |
.collect::<Vec<_>>(); | |
if radio_buttons.is_empty() { | |
return; // No enabled radio buttons in the group | |
} | |
// Pick out the radio button that is currently checked. | |
ev.propagate(false); | |
let current_radio = radio_buttons |
I can fix that too in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9806468
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
Apologies for not responding on this, I haven't forgotten about it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay. I finally had a chance to check this out locally and verify that the keyboard navigation behavior still worked as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this now, but it needs a migration guide :) The changes here are quite subtle and won't result in compiler errors.
Objective
Fixes #21261
Solution
Changes:
This makes radio button behavior:
Current behavior in examples doesn't change with this PR.
Testing
Tested using existing examples. See
feathers
example, behavior doesn't change.Additionally, tested in bevy_immediate where widget consistency is useful.