Fix SearchBar key event condition #811
Conversation
| @@ -457,7 +459,7 @@ private fun SearchBarInputField( | |||
| .focusRequester(focusRequester) | |||
| .onFocusChanged { if (it.isFocused) onActiveChange(true) } | |||
| .onKeyEvent { | |||
| if (it.key == Key.Escape) { | |||
| if (it.key == Key.Escape && it.type == KeyEventType.KeyDown) { | |||
There was a problem hiding this comment.
Not sure it's really needed, but just in case I've been doing it like this:
if (it.key == Key.Escape) {
if (it.type == KeyEventType.KeyDown) {
action()
}
true
}
else {
false
}
So that they key-up is also consumed.
@igordmn What do you recommend?
There was a problem hiding this comment.
We do not consume up event, so we shouldn't fake-consume it
There was a problem hiding this comment.
I don't have a strong opinion here, but as I can't imagine cases where we need to handle KeyUp, even if KeyDown is consumed, I lean toward consuming everything.
There was a problem hiding this comment.
We do not consume up event, so we shouldn't fake-consume it
This was my first response as well, but I didn't send it :). Let's Ivan decide.
There was a problem hiding this comment.
An argument for not consuming:
- we should consume only what we handle
An argument for consuming:
- someone else can handle KeyUp, not knowing that another element consumes KeyDown
If we choose to consume, we can say that we consume "KeyDown gesture": KeyDown -> perform action -> KeyUp
There was a problem hiding this comment.
There are a list of places where we follow
we should consume only what we handle
So, let's be consistent
There was a problem hiding this comment.
Yes, I agree, that we should do the same in this PR. the question - should we change the pattern everywhere.
There was a problem hiding this comment.
I think it's a bit safer to consume either both or neither. But since nobody has complained so far, maybe it doesn't affect (m)any people.
There was a problem hiding this comment.
Let's keep it as-as for now, and reconsider it in case of issues
## Proposed Changes Addition to #801 ## Testing Test: Don't change the state in `onActiveChange`, press `Esc` - it was called twice
## Proposed Changes Addition to #801 ## Testing Test: Don't change the state in `onActiveChange`, press `Esc` - it was called twice
Proposed Changes
Addition to #801
Testing
Test: Don't change the state in
onActiveChange, pressEsc- it was called twice