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.15k stars 1.17k forks source link

Opening TextContextMenu causes containing Column to change height #2729

Closed ialokim closed 1 year ago

ialokim commented 1 year ago

Describe the bug Opening the TextContextMenu on a Text contained in a Column with verticalArrangement = Arrangement.spacedBy(8.dp) adds a space to the Column, increasing its height.

Affected platforms Select one of the platforms below:

If the bug is Android-only, report it in the Jetpack Compose tracker

Versions

To Reproduce

import androidx.compose.foundation.border
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.text.selection.SelectionContainer
import androidx.compose.material.Text
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.unit.dp
import androidx.compose.ui.window.singleWindowApplication

fun main() = singleWindowApplication {
    Column(
        modifier = Modifier.border(2.dp, Color.Black),
        verticalArrangement = Arrangement.spacedBy(8.dp)
    ) {
        SelectionContainer {
            Text("Test")
        }
    }
}

Expected behavior The height of the column should not change when the context menu is shown.

ialokim commented 1 year ago

Just confirmed the same bug exists with Compose Multiplatform version 1.3.0.

m-sasha commented 1 year ago

The problem here is that the context menu uses a Popup, which is added as a child to the Column. The popup is a 0x0 element, so the assumption is that it won't affect the parent layout. But if you use a Column with verticalArrangement=spacedBy, then a 0x0 child causes the extra spacing to be added. The same code, without a verticalArrangement doesn't have this issue.

Here's a simpler reproducer:

@OptIn(ExperimentalFoundationApi::class)
fun main() = singleWindowApplication {
    var showPopup by remember { mutableStateOf(false) }
    Column(
        modifier = Modifier.border(1.dp, Color.Black).onClick { showPopup = !showPopup },
        verticalArrangement = Arrangement.spacedBy(8.dp)
    ) {
        Text("Test", Modifier.border(1.dp, Color.Red))
        if (showPopup){
            Popup(offset = IntOffset(0, 100)){
                Text("Popup content")
            }
        }
    }
}
m-sasha commented 1 year ago

To fix this, we should get rid of the empty Layout in PopupLayout.

m-sasha commented 1 year ago

By the way, the same problem (with Popup) exists on Android, and for the same reason.

m-sasha commented 1 year ago

In the case where a Popup is used directly, there is no easy solution on the side of the library, because there's currently no way to obtain the parent's coordinates without emitting a Layout. But this case has an easy workaround on the user side: just wrap the content and Popup into a Box.

As for SelectionContainer where the user has no control over the placement of the Popup in the hierarchy, I submitted a PR that wraps the content and the context menu Representation (which ends up being a Popup) into a Box.

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.