Fix SwingPanel focus interop #1821
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes https://youtrack.jetbrains.com/issue/CMP-7472/Fix-Swing-interop-focus-after-1.8.0-alpha08-merge The failing tests were a regression after https://android-review.googlesource.com/c/platform/frameworks/support/+/3417182 that added requesting focus on the owner at the end of a focus transaction: ``` if (ComposeUiFlags.isViewFocusFixEnabled && requireLayoutNode().getInteropView() == null) { // This isn't an AndroidView, so we should be focused on this ComposeView requireOwner().focusOwner.requestFocusForOwner(FocusDirection.Next, null) } ``` As we can see here, there is an exception if the node has `SwingPanel` (interop view), because when we focus on this node, it will redirect the focus to the panel itself. But this exception didn't work on desktop because we had 2 invisible nodes around `SwingPanel`: ``` backwardTrackerModifier // redirects focus to the component, but then it will be overridden by `requestFocusForOwner` interopViewModifier // getInteropView() != null Swing component forwardTrackerModifier ``` Now the logic is rewritten similar to Android inside [Modifier.focusInteropModifier]: ``` parentModifier // fake node to intercept `onEnter` to know the focus direction childModifier // redirects focus to the component, skip `requestFocusForOwner` interopViewModifier // getInteropView() != null Swing component ``` The recursive focus check inside `Tracker` is removed, because it is not possible to have recursive calls if we don't request focus inside `onFocusEvent` # Testing `ComposeFocusTest` passes, it covers all possible edge cases of focus interop # Release Notes N/A
MatkovIvan
reviewed
Feb 4, 2025
| update = { | ||
| it.background = background.toAwtColor() | ||
| update(it) | ||
| } | ||
| ) | ||
|
|
||
| EmptyLayout(focusSwitcher.forwardTrackerModifier) |
There was a problem hiding this comment.
Glad to see that it's finally gone. As far as I remember it blocked some unification between platforms before. Please add some TODO for me - I'll check that refactor later
MatkovIvan
reviewed
Feb 4, 2025
Just checked, when we disable it - the tests are passed with this PR (and without the PR) |
m-sasha
approved these changes
Feb 5, 2025
| FocusDirection.Previous -> { | ||
| focusPolicy.getLastComponent(group)?.requestFocus(TRAVERSAL_BACKWARD) | ||
| } | ||
| else -> null |
| import androidx.compose.ui.focus.focusRequester | ||
| import androidx.compose.ui.focus.focusTarget | ||
| import androidx.compose.ui.focus.onFocusEvent | ||
| import androidx.compose.ui.viewinterop.InteropViewGroup | ||
| import java.awt.event.FocusEvent | ||
| import java.awt.event.FocusEvent.Cause.TRAVERSAL_BACKWARD | ||
| import java.awt.event.FocusEvent.Cause.TRAVERSAL_FORWARD | ||
|
|
||
| internal class InteropFocusSwitcher( |
There was a problem hiding this comment.
Maybe add a few lines about what this does and how it does it?
Comment on lines
+125
to
+127
| val modifier = Modifier | ||
| .then(parentModifier) | ||
| .then(childModifier) |
There was a problem hiding this comment.
Is the split to two modifiers just for ease-of-reading/cleanliness? It seems the only place the two are used is right here, so there's no technical need to have two.
There was a problem hiding this comment.
Yes, only for ease-of-reading. If we don't split, it is not obvious why we used focusTarget 2 times.
igordmn
added a commit
to JetBrains/compose-multiplatform
that referenced
this pull request
Feb 6, 2025
The current script contains JetBrains/compose-multiplatform-core#1821 because it started with #, not with ## # Testing ``` kotlin changelog.main.kts v1.8.0-alpha02..v1.8.0+dev2047 ``` Doesn't contain JetBrains/compose-multiplatform-core#1821 # Release Notes N/A
igordmn
added a commit
to JetBrains/compose-multiplatform
that referenced
this pull request
Feb 6, 2025
The current script contains JetBrains/compose-multiplatform-core#1821 because it started with #, not with ## # Testing ``` kotlin changelog.main.kts v1.8.0-alpha02..v1.8.0+dev2047 ``` Doesn't contain JetBrains/compose-multiplatform-core#1821 # Release Notes N/A
igordmn
added a commit
to JetBrains/compose-multiplatform
that referenced
this pull request
Feb 7, 2025
The current script contains JetBrains/compose-multiplatform-core#1821 because it started with #, not with ## # Testing ``` kotlin changelog.main.kts v1.8.0-alpha02..v1.8.0+dev2047 ``` Doesn't contain JetBrains/compose-multiplatform-core#1821 # Release Notes N/A
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes https://youtrack.jetbrains.com/issue/CMP-7472/Fix-Swing-interop-focus-after-1.8.0-alpha08-merge
The failing tests were a regression after https://android-review.googlesource.com/c/platform/frameworks/support/+/3417182 that added requesting focus on the owner at the end of a focus transaction:
As we can see here, there is an exception case if the node has
SwingPanel(interop view), because when we focus on this node, it will redirect the focus to the panel itself.But this exception case didn't work on desktop because we had 2 invisible nodes around
SwingPanel:Now the logic is rewritten similar to Android inside Modifier.focusInteropModifier:
Also, the recursive focus check inside
Trackeris removed, because it is not possible to have recursive calls if we don't request focus insideonFocusEventTesting
ComposeFocusTestpasses, it covers all possible edge cases of focus interopRelease Notes
N/A