Skip to content

Make non-primary click events focus buttons and checkboxes. #1329

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

Draft
PoignardAzur wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Make non-primary click events focus buttons and checkboxes. #1329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f1653c
DRAFT: Make non-primary click events focus buttons and checkboxes.
PoignardAzur Aug 13, 2025
0abce18
Change logic
PoignardAzur Sep 19, 2025
7af1490
Fix unit test
PoignardAzur Sep 19, 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
7 changes: 5 additions & 2 deletions masonry/src/tests/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,10 @@ fn click_anchors_focus() {
))
.with_child(NewWidget::new(Button::with_text("")))
.with_child(NewWidget::new(Button::with_text("")))
.with_child(NewWidget::new_with_tag(Button::with_text(""), child_3))
.with_child(NewWidget::new_with_tag(
SizedBox::empty().size(10.px(), 10.px()),
child_3,
))
.with_child(NewWidget::new_with_tag(Button::with_text(""), child_4))
.with_child(NewWidget::new(Button::with_text("")))
.with_auto_id();
Expand All @@ -244,7 +247,7 @@ fn click_anchors_focus() {
let child_4_id = harness.get_widget(child_4).id();
let other_id = harness.get_widget(other).id();

// Clicking a button doesn't focus it.
// Clicking a sized box doesn't focus it.
harness.mouse_click_on(child_3_id);
assert_eq!(harness.focused_widget_id(), None);

Expand Down
37 changes: 30 additions & 7 deletions masonry/src/widgets/button.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,24 @@ impl Widget for Button {
event: &PointerEvent,
) {
match event {
PointerEvent::Down { .. } => {
ctx.capture_pointer();
// Changes in pointer capture impact appearance, but not accessibility node
ctx.request_paint_only();
trace!("Button {:?} pressed", ctx.widget_id());
PointerEvent::Down { button, .. } => {
if *button == Some(PointerButton::Primary) {
ctx.capture_pointer();
// Changes in pointer capture impact appearance, but not accessibility node
ctx.request_paint_only();
trace!("Button {:?} pressed", ctx.widget_id());
}
Comment on lines +116 to +121
Copy link
Member

@DJMcNab DJMcNab Oct 27, 2025

Choose a reason for hiding this comment

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

This change means that buttons are no longer usable on touch screens (e.g. on Android).
We cannot land this change with that regression in place.

It also breaks button_any_pointer on the Xilem side, which is fine, but we should just remove it if we're doing that. But no longer supporting touch screens is not a good plan. button_any_pointer was added as a quick hack to unblock a user, but I'm not going to dig into who that was at the moment.

Copy link
Contributor Author

@PoignardAzur PoignardAzur Oct 27, 2025

Choose a reason for hiding this comment

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

So what kind of filter could we use that accepts touchscreen presses but rejects right-clicks?

Is it Some(PointerButton::Primary) | None ?

Copy link
Member

@DJMcNab DJMcNab Oct 28, 2025

Choose a reason for hiding this comment

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

Yes. I should have said that explicitly

// Any click event should lead to this widget getting focused.
ctx.request_focus();
Copy link
Member

@DJMcNab DJMcNab Oct 27, 2025

Choose a reason for hiding this comment

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

Should this set the focus anchor, rather than actually setting the focus directly? I think either way would be viable. On the web, it seems to do the former, from some brief testing.

This doesn't block this PR.

}
PointerEvent::Up { button, .. } => {
PointerEvent::Up {
button: Some(PointerButton::Primary),
..
} => {
if ctx.is_active() && ctx.is_hovered() {
ctx.submit_action::<Self::Action>(ButtonPress { button: *button });
ctx.submit_action::<Self::Action>(ButtonPress {
button: Some(PointerButton::Primary),
});
trace!("Button {:?} released", ctx.widget_id());
}
// Changes in pointer capture impact appearance, but not accessibility node
Expand Down Expand Up @@ -316,6 +325,7 @@ impl Widget for Button {
// --- MARK: TESTS
#[cfg(test)]
mod tests {
use assert_matches::assert_matches;
use masonry_testing::{TestHarnessParams, assert_failing_render_snapshot};

use super::*;
Expand Down Expand Up @@ -363,6 +373,19 @@ mod tests {
);
}

#[test]
fn button_right_click() {
let button = NewWidget::new(Button::with_text("Hello"));

let mut harness = TestHarness::create(default_property_set(), button);
let button_id = harness.root_id();

harness.mouse_move_to(button_id);
harness.mouse_button_press(PointerButton::Secondary);
assert_eq!(harness.focused_widget_id(), Some(button_id));
assert_matches!(harness.pop_action_erased(), None);
}
Comment on lines +376 to +387
Copy link
Member

@DJMcNab DJMcNab Oct 27, 2025

Choose a reason for hiding this comment

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

We need a test for an emulated touch event here. That's a Down event with a button of None.


#[test]
fn edit_button() {
let image_1 = {
Expand Down
34 changes: 30 additions & 4 deletions masonry/src/widgets/checkbox.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use accesskit::{Node, Role, Toggled};
use masonry_core::core::HasProperty;
use tracing::{Span, trace, trace_span};
use ui_events::keyboard::Key;
use ui_events::pointer::PointerButton;
use vello::Scene;
use vello::kurbo::Rect;
use vello::kurbo::{Affine, BezPath, Cap, Dashes, Join, Size, Stroke};
Expand Down Expand Up @@ -112,11 +113,20 @@ impl Widget for Checkbox {
event: &PointerEvent,
) {
match event {
PointerEvent::Down { .. } => {
ctx.capture_pointer();
trace!("Checkbox {:?} pressed", ctx.widget_id());
PointerEvent::Down { button, .. } => {
if *button == Some(PointerButton::Primary) {
ctx.capture_pointer();
// Checked state impacts appearance and accessibility node
ctx.request_render();
trace!("Checkbox {:?} pressed", ctx.widget_id());
}
// Any click event should lead to this widget getting focused.
ctx.request_focus();
}
PointerEvent::Up { .. } => {
PointerEvent::Up {
button: Some(PointerButton::Primary),
..
} => {
if ctx.is_active() && ctx.is_hovered() {
ctx.submit_action::<Self::Action>(CheckboxToggled(!self.checked));
trace!("Checkbox {:?} released", ctx.widget_id());
Expand Down Expand Up @@ -349,6 +359,8 @@ impl Widget for Checkbox {
// --- MARK: TESTS
#[cfg(test)]
mod tests {
use assert_matches::assert_matches;

use super::*;
use crate::core::{Properties, StyleProperty};
use crate::properties::ContentColor;
Expand Down Expand Up @@ -411,6 +423,20 @@ mod tests {
harness.focus_on(Some(checkbox_id));
assert_render_snapshot!(harness, "checkbox_focus_focused");
}

#[test]
fn checkbox_right_click() {
let checkbox = NewWidget::new(Checkbox::new(true, "Hello"));

let mut harness = TestHarness::create(default_property_set(), checkbox);
let checkbox_id = harness.root_id();

harness.mouse_move_to(checkbox_id);
harness.mouse_button_press(PointerButton::Secondary);
assert_eq!(harness.focused_widget_id(), Some(checkbox_id));
assert_matches!(harness.pop_action_erased(), None);
}

#[test]
fn edit_checkbox() {
let image_1 = {
Expand Down
Loading