Skip to content

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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+89 −59
Open

Make Radio Button behaviour modular and consistent with other widgets #21294

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49e7053
Make Radio Button behaviour modular and consistent with other widgets
PPakalns Sep 30, 2025
9806468
Fix bug with event propagation cancellation
PPakalns Oct 2, 2025
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
154 changes: 92 additions & 62 deletions crates/bevy_ui_widgets/src/radio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use bevy_ecs::{
entity::Entity,
hierarchy::{ChildOf, Children},
observer::On,
query::{Has, With},
query::{Has, With, Without},
reflect::ReflectComponent,
system::{Commands, Query},
};
Expand Down Expand Up @@ -39,12 +39,18 @@ use crate::ValueChange;
#[require(AccessibilityNode(accesskit::Node::new(Role::RadioGroup)))]
pub struct RadioGroup;

/// Headless widget implementation for radio buttons. These should be enclosed within a
/// [`RadioGroup`] widget, which is responsible for the mutual exclusion logic.
/// Headless widget implementation for radio buttons. They can be used independently,
/// but enclosing them in a [`RadioGroup`] widget allows them to behave as a single,
/// mutually exclusive unit.
///
/// According to the WAI-ARIA best practices document, radio buttons should not be focusable,
/// but rather the enclosing group should be focusable.
/// See <https://www.w3.org/WAI/ARIA/apg/patterns/radio>/
///
/// The widget emits a [`ValueChange<bool>`] event with the value `true` whenever it becomes checked,
/// either through a mouse click or when a [`RadioGroup`] checks the widget.
/// If the [`RadioButton`] is focusable, it can also be checked using the `Enter` or `Space` keys,
/// in which case the event will likewise be emitted.
#[derive(Component, Debug)]
#[require(AccessibilityNode(accesskit::Node::new(Role::RadioButton)), Checkable)]
#[derive(Reflect)]
Expand Down Expand Up @@ -132,7 +138,12 @@ fn radio_group_on_key_input(

let (next_id, _) = radio_buttons[next_index];

// Trigger the on_change event for the newly checked radio button
// Trigger the value change event on the radio button
commands.trigger(ValueChange::<bool> {
Copy link
Contributor

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 trigger Activate 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 use ValueChange, it should always reflect the current state of the widget triggering it: a listener that only listens to ValueChange should always know what state the source is in. We can't do this if it only sends ValueChange for some state transitions and not others.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@PPakalns PPakalns Oct 1, 2025 •
edited
Loading

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.

source: next_id,
value: true,
});
// Trigger the on_change event for the newly checked radio button on radio group
commands.trigger(ValueChange::<Entity> {
source: ev.focused_entity,
value: next_id,
Expand All @@ -141,82 +152,101 @@ fn radio_group_on_key_input(
}
}

fn radio_group_on_button_click(
mut ev: On<Pointer<Click>>,
// Provides functionality for standalone focusable [`RadioButton`] to react
// on `Space` or `Enter` key press.
fn radio_button_on_key_input(
mut ev: On<FocusedInput<KeyboardInput>>,
q_radio_button: Query<Has<Checked>, (With<RadioButton>, Without<InteractionDisabled>)>,
q_group: Query<(), With<RadioGroup>>,
q_radio: Query<(Has<Checked>, Has<InteractionDisabled>), With<RadioButton>>,
q_parents: Query<&ChildOf>,
q_children: Query<&Children>,
mut commands: Commands,
) {
if q_group.contains(ev.entity) {
// Starting with the original target, search upward for a radio button.
let radio_id = if q_radio.contains(ev.original_event_target()) {
ev.original_event_target()
} else {
// Search ancestors for the first radio button
let mut found_radio = None;
for ancestor in q_parents.iter_ancestors(ev.original_event_target()) {
if q_group.contains(ancestor) {
// We reached a radio group before finding a radio button, bail out
return;
}
if q_radio.contains(ancestor) {
found_radio = Some(ancestor);
break;
}
}
let Ok(checked) = q_radio_button.get(ev.focused_entity) else {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@PPakalns PPakalns Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed in 9806468

// Not a radio button
return;
};

// Radio button already checked
if checked {
return;
}

match found_radio {
Some(radio) => radio,
None => return, // No radio button found in the ancestor chain
}
};
let event = &ev.event().input;
if event.state == ButtonState::Pressed
&& !event.repeat
&& (event.key_code == KeyCode::Enter || event.key_code == KeyCode::Space)
{
ev.propagate(false);

// Radio button is disabled.
if q_radio.get(radio_id).unwrap().1 {
return;
}
trigger_radio_button_and_radio_group_value_change(
ev.focused_entity,
&q_group,
&q_parents,
&mut commands,
);
}
}

// 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
}
fn radio_button_on_click(
mut ev: On<Pointer<Click>>,
q_group: Query<(), With<RadioGroup>>,
q_radio: Query<Has<Checked>, (With<RadioButton>, Without<InteractionDisabled>)>,
q_parents: Query<&ChildOf>,
mut commands: Commands,
) {
let Ok(checked) = q_radio.get(ev.entity) else {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9806468

// Not a radio button
return;
};

// Pick out the radio button that is currently checked.
ev.propagate(false);
let current_radio = radio_buttons
.iter()
.find(|(_, checked)| *checked)
.map(|(id, _)| *id);

if current_radio == Some(radio_id) {
// If they clicked the currently checked radio button, do nothing
return;
}
ev.propagate(false);

// Radio button is already checked
if checked {
return;
}

// Trigger the on_change event for the newly checked radio button
trigger_radio_button_and_radio_group_value_change(
ev.entity,
&q_group,
&q_parents,
&mut commands,
);
}

fn trigger_radio_button_and_radio_group_value_change(
radio_button: Entity,
q_group: &Query<(), With<RadioGroup>>,
q_parents: &Query<&ChildOf>,
commands: &mut Commands,
) {
commands.trigger(ValueChange::<bool> {
source: radio_button,
value: true,
});

// Find if radio button is inside radio group
let radio_group = q_parents
.iter_ancestors(radio_button)
.find(|ancestor| q_group.contains(*ancestor));

// If is inside radio group
if let Some(radio_group) = radio_group {
// Trigger event for radio group
commands.trigger(ValueChange::<Entity> {
source: ev.entity,
value: radio_id,
source: radio_group,
value: radio_button,
});
}
}

/// Plugin that adds the observers for the [`RadioGroup`] widget.
/// Plugin that adds the observers for [`RadioButton`] and [`RadioGroup`] widget.
pub struct RadioGroupPlugin;

impl Plugin for RadioGroupPlugin {
fn build(&self, app: &mut App) {
app.add_observer(radio_group_on_key_input)
.add_observer(radio_group_on_button_click);
.add_observer(radio_button_on_click)
.add_observer(radio_button_on_key_input);
}
}