fix: SplitButton.Click is not fired when SplitButton is not in focused
#16940
Conversation
|
You can test this PR using the following package version. |
| base.OnKeyDown(e); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| protected override void OnKeyUp(KeyEventArgs e) | ||
| { | ||
| var key = e.Key; |
There was a problem hiding this comment.
For both of these changes it's a lot easier to read the changes if you just check and exit on entry:
if (!IsFocused)
{
base.OnKeyUp(e);
return;
}- That would remove a lot of noise in this PR
- It helps with maintainability and readability to reduce nesting in the logic
There was a problem hiding this comment.
I preferred this solution because IL is more compact, see example.
There was a problem hiding this comment.
I don't think IL really matters here, as JIT might produce very similarly efficient code in the end.
But I don't like doubling base.OnKeyUp(e); call. Early returns are great, but duplicating base calls is not so much.
| var key = e.Key; | ||
|
|
||
| if (key == Key.Space || key == Key.Enter) | ||
| if (IsFocused) |
There was a problem hiding this comment.
What about IsEffectivelyEnabled? We should probably check that too, right?
|
@workgroupengineering Whatever is done here MUST also be done to button. SplitButton copied the logic from Button originally so it would have the same issue. Edit: Looking back at the Button PR for similar issue's you are not implementing the changes the same. I don't think that's a good idea. Button itself probably shouldn't even process Code should not diverge like this (admittedly this is my fault I'm going to have to fix in Avalonia 12. I shouldn't have followed WinUI and should just have inherited Button in SplitButton). |
|
You can test this PR using the following package version. |
There was a problem hiding this comment.
I agree with @robloo here: other keys should still be processed even if the SplitButton doesn't have the focus directly, as the focus is probably on a child.
We should special case Space only, matching Button.
(As I mentioned in the PR for Button, ideally we shouldn't have anything to do at all. These PR are workarounds for a very specific issue, as redesigning the whole input system isn't on the table right now.)
|
I had another thought here. This situation isn't even handled well in WinUI. Microsoft probably made a mistake adding Space key support for controls like Button. ENTER alone is just fine for usability. We might want to completely remove space handling from Button and SplitButton honestly. |
|
Space handling is a bit wider question, and also touches how we map controller keys (which is supported in 11.1 on backend-basis): Avalonia/src/iOS/Avalonia.iOS/InputHandler.cs Line 149 in 3a9a316 Avalonia/src/iOS/Avalonia.iOS/InputHandler.cs Line 374 in 3a9a316 (and the same is done in Tizen, but I think on Tizen SDK side automatically) |
I disagree, Space has been used to press buttons since at least Windows 3.1. Personally, I'm still using this very often, in a variety of programs. This shouldn't be removed from Avalonia because of this issue. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
MrJul
added
the
backport-candidate-11.2.x
…cused (#16940) * test: Check that `SplitButton.Click` is not fired when `SplitButton` is not in focused * fix: `SplitButton.Click` is not fired when `SplitButton` is not in focused * fix: Address review * fix: Address review --------- Co-authored-by: Julien Lebosquain <julien@lebosquain.net>
maxkatz6
added
backported-11.2.x
and removed
backport-candidate-11.2.x
What does the pull request do?
What is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
Continue of #16184