Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opening TextContextMenu causes containing Column to change height #2729

Closed
1 of 6 tasks
ialokim opened this issue Feb 11, 2023 · 6 comments · Fixed by JetBrains/compose-multiplatform-core#436
Closed
1 of 6 tasks
Assignees
Labels
bug Something isn't working reproduced

Comments

@ialokim
Copy link

ialokim commented Feb 11, 2023

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:

  • All
  • Desktop
  • Web (K/JS) - HTML based API
  • Web (K/JS) - Canvas based API
  • iOS
  • Other

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

Versions

  • Kotlin version*: 1.7.20
  • Compose Multiplatform version*: 1.2.1
  • OS version(s)* (required for Desktop and iOS issues): Linux
  • OS architecture (x86 or arm64): x86

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 ialokim changed the title Opening TextContextMenu causes containing Row to change height Opening TextContextMenu causes containing Column to change height Feb 11, 2023
@ialokim
Copy link
Author

ialokim commented Feb 11, 2023

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

@pjBooms pjBooms added reproduced bug Something isn't working labels Feb 13, 2023
@m-sasha
Copy link
Member

m-sasha commented Mar 11, 2023

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
Copy link
Member

m-sasha commented Mar 11, 2023

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

@m-sasha m-sasha self-assigned this Mar 11, 2023
@m-sasha
Copy link
Member

m-sasha commented Mar 12, 2023

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

@m-sasha
Copy link
Member

m-sasha commented Mar 13, 2023

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working reproduced
Projects
None yet
5 participants