Adapt focus interop code to Compose 1.7 changes #1398

igordmn · 2024-06-10T03:38:34Z

After https://android-review.googlesource.com/c/platform/frameworks/support/+/2813125, there are some changes that affected our code for focus interop with ComposePanel/SwingPanel:

the root node no longer focusable
FocusOwnerImpl now has iterop callbacks and our own modifications of this class (in jb-main, integration was reset) doesn't work anymore

Testing

added new tests
ComposeFocusTest now pass

Release notes

Fixes - Multiple platforms

Fix "ComposePanel. Focus moves to child after focusing/unfocusing the main window"

igordmn · 2024-06-10T03:40:24Z

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposePanel.desktop.kt

                            FocusEvent.Cause.UNKNOWN, FocusEvent.Cause.ACTIVATION -> {
-                                focusManager.moveFocus(FocusDirection.Enter)
+                                if (!focusManager.hasFocus) {


This particular check fixes JetBrains/compose-multiplatform#4919

igordmn · 2024-06-10T03:44:30Z

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/SwingPanel.desktop.kt

-    private val backwardRequester = FocusRequester()
-    private val forwardRequester = FocusRequester()
-    private var isRequesting = false
+    private val backwardTracker = FocusTracker {


New features of FocusSwitcher:

simple refactoring

avoiding recursive calls (can happen with empty panels with the new code from the upstream)

igordmn · 2024-06-10T03:47:28Z

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposePanel.desktop.kt

                        when (e.cause) {
-                            FocusEvent.Cause.TRAVERSAL_FORWARD -> {


This is moved to ComposeSceneMediator, as it is valid for all containers using it

igordmn · 2024-06-10T03:50:20Z

compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/awt/ComposePanelTest.kt

@@ -462,59 +467,4 @@ class ComposePanelTest {
            window.dispose()
        }
    }
-
-    @Test
-    fun `requestFocus assigns focus to first focusable element`() = runApplicationTest {


I moved this test to another test, and adapted to common logic there

igordmn · 2024-06-10T04:17:43Z

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposePanel.desktop.kt

                            FocusEvent.Cause.UNKNOWN, FocusEvent.Cause.ACTIVATION -> {
-                                focusManager.moveFocus(FocusDirection.Enter)
+                                if (!focusManager.hasFocus) {
+                                    focusManager.takeFocus(FocusDirection.Next)


FocusDirection.Enter doesn't work anymore (probably a commonMain bug), but there is no semantic difference with Next here

Fixes JetBrains/compose-multiplatform#2944 Fixes JetBrains/compose-multiplatform#4919 Fixes https://youtrack.jetbrains.com/issue/COMPOSE-1212/Integration.-Check-changes-in-FocusOwnerImpl After https://android-review.googlesource.com/c/platform/frameworks/support/+/2813125, there are some changes that affected our code for focus interop with ComposePanel/SwingPanel: - the root node no longer focusable - FocusOwnerImpl now has iterop callbacks and our own modification of this file (in jb-main, integration was reset) doesn't work anymore ## Testing - added new tests - ComposeFocusTest now pass ## Release notes ### Fixes - Multiple platforms - Fix "ComposePanel. Focus moves to child after focusing/unfocusing the main window"

MatkovIvan · 2024-06-10T07:16:46Z

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/SwingPanel.desktop.kt

@@ -194,67 +207,77 @@ private class FocusSwitcher<T : Component>(
    private val interopComponent: SwingInteropComponent<T>,
    private val focusManager: FocusManager,
 ) {


I'd like to extract this class into separate file (with name something like SwingInteropFocusSwitcher) at some point. Not a blocker

I wouldn't move it out of this file, until the file size is manageable (<1000 lines). Because this logic is strictly bound to SwingPanel.

A thing I would extract is FocusTracker - it might be useful for other platforms. But only when it is needed - there is a chance that other implementations should use the Android way.

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt

compose/ui/ui/api/desktop/ui.api

…gor.demin/integration-fix-focus

m-sasha · 2024-06-10T12:51:10Z

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/SwingPanel.desktop.kt

@@ -156,6 +156,19 @@ public fun <T : Component> SwingPanel(
    }
 }

+/**
+ * true, if the event is handled by SwingPanel.


"Returns whether the event is handled by SwingPanel"

m-sasha · 2024-06-10T12:52:52Z

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposePanel.desktop.kt

-                    // In that case, SwingPanel will take care of it.
-                    if (!isParentOf(e.oppositeComponent)) {
-                        focusManager.requestFocus()
+                    if (!e.isTemporary && !e.isFocusGainedHandledBySwingPanel(this@ComposePanel)) {


A bit confusing that this is called from ComposePanel (the function is about "SwingPanel"). Is this correct?

Yes, this is ugly, but this was the fastest way to implement SwingPanel back in the days. I didn't change the logic, just added a name. There is no other purpose besides using for SwingPanel.

A good alternative exists, but it needs investigation. I described it in the KDoc of this function.

m-sasha · 2024-06-10T13:00:23Z

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/node/RootNodeOwner.skiko.kt

    private val rootModifier = EmptySemanticsElement(rootSemanticsNode)
+        .then(Modifier.focusProperties {


The then here could be omitted.

igordmn commented Jun 10, 2024

View reviewed changes

igordmn force-pushed the igor.demin/integration-fix-focus branch from 6c6f035 to f003a4a Compare June 10, 2024 03:46

igordmn commented Jun 10, 2024

View reviewed changes

igordmn force-pushed the igor.demin/integration-fix-focus branch from f003a4a to bd67fde Compare June 10, 2024 04:17

igordmn commented Jun 10, 2024

View reviewed changes

igordmn force-pushed the igor.demin/integration-fix-focus branch from bd67fde to 942bbfc Compare June 10, 2024 04:21

igordmn requested review from MatkovIvan and m-sasha June 10, 2024 04:22

MatkovIvan reviewed Jun 10, 2024

View reviewed changes

compose/ui/ui/api/desktop/ui.api Outdated Show resolved Hide resolved

igordmn added 3 commits June 10, 2024 13:17

Dump API

f2225ff

Merge remote-tracking branch 'refs/remotes/origin/integration' into i…

7c02d1b

…gor.demin/integration-fix-focus

Discussions

1e5a9e0

MatkovIvan approved these changes Jun 10, 2024

View reviewed changes

m-sasha approved these changes Jun 10, 2024

View reviewed changes

Discussions

bbf01e8

igordmn merged commit a696ac0 into integration Jun 10, 2024
3 of 8 checks passed

igordmn deleted the igor.demin/integration-fix-focus branch June 10, 2024 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt focus interop code to Compose 1.7 changes #1398

Adapt focus interop code to Compose 1.7 changes #1398

igordmn commented Jun 10, 2024 •

edited

Loading

igordmn Jun 10, 2024

igordmn Jun 10, 2024 •

edited

Loading

igordmn Jun 10, 2024

igordmn Jun 10, 2024

igordmn Jun 10, 2024 •

edited

Loading

MatkovIvan Jun 10, 2024

igordmn Jun 10, 2024 •

edited

Loading

m-sasha Jun 10, 2024

igordmn Jun 10, 2024

m-sasha Jun 10, 2024

igordmn Jun 10, 2024

m-sasha Jun 10, 2024

igordmn Jun 10, 2024

		private val rootModifier = EmptySemanticsElement(rootSemanticsNode)
		.then(Modifier.focusProperties {

Adapt focus interop code to Compose 1.7 changes #1398

Adapt focus interop code to Compose 1.7 changes #1398

Conversation

igordmn commented Jun 10, 2024 • edited Loading

Testing

Release notes

Fixes - Multiple platforms

Choose a reason for hiding this comment

igordmn Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igordmn Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igordmn Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igordmn commented Jun 10, 2024 •

edited

Loading

igordmn Jun 10, 2024 •

edited

Loading

igordmn Jun 10, 2024 •

edited

Loading

igordmn Jun 10, 2024 •

edited

Loading