JetBrains / compose-multiplatform

Compose Multiplatform, a modern UI framework for Kotlin that makes building performant and beautiful user interfaces easy and enjoyable.
https://jetbrains.com/lp/compose-multiplatform
Apache License 2.0
16.19k stars 1.18k forks source link

First focusable Compose component doesn't get focus in IDE toolwindow #3888

Closed rock3r closed 5 months ago

rock3r commented 1 year ago

Describe the bug When working in the IDE, if I have a ToolWindow with a ComposePanel as its tab content, when the tab is shown, the focus is not passed to the first focusable Composable, even though the ComposePanel does get focused.

Affected platforms Select one of the platforms below:

Versions

To Reproduce Create an IJ plugin using Jewel. Add a toolwindow and add Compose content to it.

val composePanel = ComposePanel()

composePanel.setContent {
    MyTheme {
        var text by remember { mutableStateOf("") }
        TextField(text, { text = it })
    }
}
val wrapper = Wrapper(composePanel) // com.intellij.ui.components.panels.Wrapper
val tabContent = contentManager.factory.createContent(wrapper, "My tab", false)
tabContent.isCloseable = isCloseable
contentManager.addContent(tabContent)

Expected behavior The first composable component should get focused when the composition starts.

Screenshots image

Additional context @Walingar can provide it

m-sasha commented 5 months ago

Does it work if you request focus on the TextField?

val focusRequester = remember { FocusRequester() }
TextField(
    ...
    modifier = Modifier.focusRequester(focusRequester)
)
LaunchedEffect(Unit) {
    focusRequester.requestFocus()
}
rock3r commented 5 months ago

Yep, that's the current workaround in the Jewel IDE sample. It isn't good enough tho; you can either only ever do it the first time a tab is shown (with a Launched effect(Unit)), or every time (with a SideEffect). But in the former case, this means the next time the tab is shown it won't get focus in the composition, and in the latter it will reset focus to a given component, which isn't necessarily the one that was last focused.

m-sasha commented 5 months ago

I don't think this is an issue specific to ComposePanel - a tabbed pane in pure Compose would have the same problem. So the question is in general about Compose - how to save/restore focus when switching between tabs/screens. I would encourage you to ask for assistance in the #compose channel in the kotlinlang Slack.

rock3r commented 5 months ago

The problem I reported here is that when the ComposePanel gets focused (it is never destroyed when not visible, to be clear), it doesn't pass the focus inwards to its content.

This is the issue, and not how to save/restore focus, which is not necessary in this case. It is not a "holding it wrong" problem, as far as I can tell. Nikolay was aware of this issue before moving out of the team and may have further information about the internals, but he did validate this as a ComposePanel issue.

m-sasha commented 5 months ago

The problem I reported here is that when the ComposePanel gets focused (it is never destroyed when not visible, to be clear), it doesn't pass the focus inwards to its content.

But it does (see code below); it's just that Compose does not automatically focus anything. If you open a regular Compose application (without ComposePanel) with a TextField, and don't manually request focus for it, it will not be focused.

Here is a ComposePanel with a textfield that requests focus for itself when shown:

fun main() = SwingUtilities.invokeLater {
    val frame = JFrame()
    val button = JButton("Show Compose Panel")
    button.addActionListener {
        val composePanel = ComposePanel().apply {
            setContent {
                val focusRequester = remember { FocusRequester() }
                var text by remember { mutableStateOf("") }
                TextField(
                    value = text,
                    onValueChange = { text = it },
                    modifier = Modifier.focusRequester(focusRequester)
                )
                LaunchedEffect(Unit) {
                    focusRequester.requestFocus()
                }
            }
        }
        composePanel.size = Dimension(400, 200)
        composePanel.location = Point(0, 100)
        frame.contentPane.add(composePanel, BorderLayout.CENTER)
    }
    frame.contentPane.add(button, BorderLayout.NORTH)
    frame.size = Dimension(400, 300)
    frame.isVisible = true
}
rock3r commented 5 months ago

Maybe I am not explaining myself clearly enough, so let me try again.

First things first: I think it is reasonable to expect that when a ComposePanel gets focus for the first time, and it contains focusable composables, it will pass the focus inwards to the first focusable composable. This is because:

  1. It makes no sense for the ComposePanel itself to hold the focus (just like a JPanel wouldn't, it would pass it down to its first focusable JComponent)
  2. This is how containers that have focusable children generally work in UI frameworks, including Swing and Compose

With that out of way, let's look at what's happening here. I have created the most minimal reproducer I can think of: a JTabbedPane with a ComposePanel (no external dependencies), and a JPanel, each having three buttons. You can find the reproducer code here: https://github.com/rock3r/cfd-repros/blob/main/src/main/kotlin/TabbedUI.kt

Screen recording

  1. The ComposePanel is the initially visible tab content. The first focusable/clickable composable does not get the focus.
  2. The JPanel is the second tab's content. When I switch to it, its first component is focused.
  3. Switching back to the Compose tab, the composition has lost the focus again. I would expect it to get the focus back when switching to it, just like the Swing version does.
rock3r commented 5 months ago

Again, to be extra clear: I am ok with whatever is the first focusable component in the ComposePanel getting the focus. I am not asking to save which composable specifically has the focus, and restoring it (although there is no sane way to do that with the current focus APIs, that is a different discussion and not one that is important here)

m-sasha commented 5 months ago

First things first: I think it is reasonable to expect that when a ComposePanel gets focus for the first time, and it contains focusable composables, it will pass the focus inwards to the first focusable composable. This is because:

  1. It makes no sense for the ComposePanel itself to hold the focus (just like a JPanel wouldn't, it would pass it down to its first focusable JComponent)
  2. This is how containers that have focusable children generally work in UI frameworks, including Swing and Compose

This is true in Swing, but isn't true in Compose. I agree it's reasonable to expect, but it's just not true.

However, it appears that ComposePanel is making some effort to behave like a Swing component would. It sometimes succeeds, but not always.

When I run the reproducer you linked to (in jb-main), the Compose "First button" does initially get focus. What makes it happen is a focus listener in ComposePanel, which calls focusManager.moveFocus(FocusDirection.Next) when it receives a FocusEvent.Cause.TRAVERSAL_FORWARD event.

When I switch to the Swing tab and back, however, the "First button" is no longer focused. This is because the cause of the focus event in this case is, for some reason, FocusEvent.Cause.UNKNOWN, and the focus listener doesn't do anything on that. If I add

    when (e.cause) {
        ...
        FocusEvent.Cause.UNKNOWN -> {
            focusManager.moveFocus(FocusDirection.Enter)
        }
    }

the "First button" receives focus correctly again.

It appears that FocusEvent.Cause.UNKNOWN is also what is sent when we just do ComposePanel.requestFocus(), which also fixes, for example, this use case:

fun main() = SwingUtilities.invokeLater {
    val frame = JFrame()
    val button = JButton("Show Compose Panel")
    var composePanel: ComposePanel? = null

    fun addComposePanel() {
        val panel = ComposePanel()
        panel.setContent {
            var text by remember { mutableStateOf("") }
            TextField(
                value = text,
                onValueChange = { text = it },
            )
        }
        panel.size = Dimension(400, 200)
        panel.location = Point(0, 100)
        frame.contentPane.add(panel, BorderLayout.CENTER)
        composePanel = panel
    }

    button.addActionListener {
        if (composePanel == null)
            addComposePanel()
        else
            composePanel!!.isVisible = !composePanel!!.isVisible
        button.setText(if (composePanel!!.isVisible) "Hide ComposePanel" else "Show ComposePanel")
        if (composePanel!!.isVisible)
            composePanel!!.requestFocus()
    }
    frame.contentPane.add(button, BorderLayout.NORTH)
    frame.size = Dimension(400, 300)
    frame.isVisible = true
}

I don't exactly know why UNKNOWN is sent, or what other causes may be sent, but it appears that this particular change fixes this particular case, so we can try it.

rock3r commented 5 months ago

Thanks for the in-depth analysis, Sasha. I am not sure why the cause is set to UNKNOWN, but that's probably because it's a programmatic change (the JTabbedPane does it on tab change, I reckon).

It'd be great to at least sort out that case.

m-sasha commented 5 months ago

By the way, in your reproducer, onFocusChanged comes after clickable (focusable, really). If you want it to actually be called, it needs to come before.

rock3r commented 5 months ago

Ah, yep — just a leftover from a previous iteration that also had focusable. Thanks for pointing it out tho — it's an easy mistake to make.

okushnikov commented 2 months ago

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.