Listen to browser clipboard events and bind them with Compose TextFieldSelectionManager and SelectionManager #1206
Conversation
…ldSelectionManager
| addTypedEvent<KeyboardEvent>("keydown") { event -> | ||
| val skikoEvent: SkikoKeyboardEvent = event.toSkikoEvent(SkikoKeyboardEventKind.DOWN) | ||
| if (skikoEvent.key in setOfClipboardKeys && skikoEvent.modifiers != SkikoInputModifiers.EMPTY) { |
There was a problem hiding this comment.
Two things I wanted to discuss regarding this fragment. First, we already have a mechanism for passing events to the browser - that's onKeyboardEventWithResult what if we'll move this logic of whether we are dispatching clipboard event over there?
Second, it seems to be the right way to filter out some specific keyboard event is to have a helper function that have actual access to the keymappings . Check out, for instance, this code
internal actual fun isCopyKeyEvent(keyEvent: KeyEvent) =
platformDefaultKeyMapping.map(keyEvent) == KeyCommand.COPY
There was a problem hiding this comment.
Fixed this one by updating KeyMapping for web. Please have a look at the latest commit.
| @@ -1191,3 +1193,9 @@ private fun notifyFocusedRect( | |||
| internal const val USE_WINDOW_FOCUS_ENABLED = false | |||
There was a problem hiding this comment.
is this a flag of a kind? Is this supposed to be set externally somehow?
There was a problem hiding this comment.
To be honest, I don't know. The logic I added is not related (at least directly) by this flag. It was there before.
| @@ -304,6 +304,8 @@ internal fun CoreTextField( | |||
| val coroutineScope = rememberCoroutineScope() | |||
| val bringIntoViewRequester = remember { BringIntoViewRequester() } | |||
|
|
|||
| rememberClipboardEventsHandler(manager, state.hasFocus) | |||
There was a problem hiding this comment.
what worries me that we are introducing a platform-specific hook to the common so that we do something on one platform. I do realise that this is sometimes unavoidable, all I'm trying to say that before it's unavoidable it worth to invest into investigating any non-invasiva alternative/
There was a problem hiding this comment.
I looked once again and I reach a conclusion that to implement this feature, we can't avoid changing common. Here is what I base my conclusion on:
- the events handler needs to "know" some textfield state (what text field is active, what text is selected, etc.). In this case the necesary state is provided by TextFieldSelectionManager (it's internal in foundation module)
- there is no global state of text fields. TextFieldState and TextFieldSelectionManager are created (and remembered) for every CoreTextField during its initialization. It makse CoreTextField a convenient place to initialize our event handler too.
- An alternative approach to find a focused node and to dispatch an event to it would require changes in common too, because currently the nodes can handle just key events, but not higher-level clipboard events.
There're similar cases when one platform has a feature/function implemented, while all other platfroms have just empty function body.
The fact that we change the common code here, doesn't require us to urgently upstream this change, because it's an internal implementation detail which doesn't affect android behaviour.
| OS.MacOS -> createMacosDefaultKeyMapping() | ||
| else -> defaultKeyMapping | ||
| } | ||
| @Composable |
There was a problem hiding this comment.
Note: github is displaying this incorrectly. File KeyMapping.jsMain.kt was renamed into KeyMapping.web.kt.
But file Actuals.jvm.kt is actually a new file.
| @@ -0,0 +1,46 @@ | |||
| /* | |||
There was a problem hiding this comment.
This is not a new file. It was renamed from KeyMapping.js.kt + updated for clipboard related key events
eymar
changed the title
Notes:
I had to almost copy/paste the code for k/js and k/wasm because of k/wasm-js and k/js-js interop differences. The function is not too big, so I consider it as a compromise.
Test in mpp:demo: