Make non-primary click events focus buttons and checkboxes. #1329
Conversation
|
Curious what the reasoning is behind this and whether anyone else does this? |
|
The right click behaviour matches e.g. Firefox (but obviously, that also open a context menu, which we don't have). As accidentally demonstrated when I did right-click, escape, enter on the "ready for review" button! |
|
It's also the behavior for middle-click, and I assume other non-primary clicks? I haven't tested it in that much detail. |
This matches the web's behavior.
PoignardAzur
force-pushed
the
focus_on_press
branch
from
d510166 to
0abce18
Compare
|
On closer inspection, I realized that the web behavior is "Any click on an focusable widget focuses it". It's just that primary clicks on buttons and links will usually navigate away, so we don't notice them. |
PoignardAzur
changed the title
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
This change currently has a fatal flaw for touch screens, and
The small hack of supporting | None is something which I'd be willing for us to land. But I would like to double check who needed this support originally first.
| 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()); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So what kind of filter could we use that accepts touchscreen presses but rejects right-clicks?
Is it Some(PointerButton::Primary) | None ?
There was a problem hiding this comment.
Yes. I should have said that explicitly
| #[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); | ||
| } |
There was a problem hiding this comment.
We need a test for an emulated touch event here. That's a Down event with a button of None.
| trace!("Button {:?} pressed", ctx.widget_id()); | ||
| } | ||
| // Any click event should lead to this widget getting focused. | ||
| ctx.request_focus(); |
There was a problem hiding this comment.
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.
Keeping this as a draft, because I think we already want non-primary clicks on buttons to send actions. This very low-priority for now.