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.12k stars 1.18k forks source link

In Compose version 1.6.1, the optional text component causes an exception when clicking on no text area. #4555

Closed ParrotCoder closed 6 months ago

ParrotCoder commented 6 months ago

Describe the bug In Compose 1.6.1 version, declare the Text component in SelectionContainer and set the size of the Text component. When clicking an area without text, an error occurs: "java.lang.IllegalStateException: SelectionLayout must not be empty." The same code is in Compose 1.5.10 Version will not cause errors

Affected platforms

Versions

To Reproduce

  1. Run the following code
  2. Click on the area where the text component has no text
  3. See error "SelectionLayout must not be empty."
    
    import androidx.compose.foundation.background
    import androidx.compose.foundation.layout.Box
    import androidx.compose.foundation.layout.fillMaxSize
    import androidx.compose.foundation.layout.padding
    import androidx.compose.foundation.text.selection.SelectionContainer
    import androidx.compose.material.Text
    import androidx.compose.runtime.Composable
    import androidx.compose.runtime.getValue
    import androidx.compose.runtime.mutableStateOf
    import androidx.compose.runtime.remember
    import androidx.compose.ui.Alignment
    import androidx.compose.ui.Modifier
    import androidx.compose.ui.graphics.Color
    import androidx.compose.ui.unit.dp
    import androidx.compose.ui.window.Window
    import androidx.compose.ui.window.WindowPosition
    import androidx.compose.ui.window.application
    import androidx.compose.ui.window.rememberWindowState

fun main() = application { Window( onCloseRequest = ::exitApplication, state = rememberWindowState( position = WindowPosition(Alignment.Center) ) ) { App() } }

@Composable private fun App() { Box( modifier = Modifier.padding(10.dp) ) { SelectionContainer { val text by remember { mutableStateOf("Text Content\r\nText Content2") } Text( text = text, modifier = Modifier .fillMaxSize() .background(Color.LightGray) ) } } }



**Expected behavior**
No errors should occur

**Screenshots**
![image](https://github.com/JetBrains/compose-multiplatform/assets/136803253/bf7a6964-0d4a-4667-96d3-4046137cca71)
m-sasha commented 6 months ago

The difference in our version that causes this is in MultiWidgetSelectionDelegate.kt:

internal fun SelectionLayoutBuilder.appendSelectableInfo(
    textLayoutResult: TextLayoutResult,
    localPosition: Offset,
    previousHandlePosition: Offset,
    selectableId: Long,
) {
    val bounds = Rect(
        0.0f,
        0.0f,
        textLayoutResult.multiParagraph.width.toFloat(),
        textLayoutResult.multiParagraph.height.toFloat()
    )
    ...
}

We use textLayoutResult.multiParagraph for the bounds size, while Android uses textLayoutResult.size. In the reproducer, textLayoutResult.size includes the entire Text (height is large), while textLayoutResult.multiParagraph only includes (vertically, at least) the area with the visual text itself (height is small; although width is large, not sure why).

Due to this, the code that follows it immediately behaves differently. currentYDirection is AFTER instead of ON, and then isSelected() a few lines below returns false, which causes the function to return without calling appendInfo. Then SelectionManager.getSelectionLayout calls SelectionLayoutBuilder.build, which throws an exception since infoList is empty.

I can't say whether the bug is the use of textLayoutResult.multiParagraph itself, or perhaps SelectionManager.getSelectionLayout should not assume that appendSelectableInfoToBuilder succeeded, or maybe appendSelectableInfo should behave differently when currentYDirection is AFTER.

It makes sense that even if dragging begins outside of the bounds of the visible text, it should still start selecting, so that when the cursor is dragged inside the visible text, the selection works. So I think appendSelectableInfo should call appendInfo in this case.

The change was introduced in 2021 in commit 37564c1fd132a637afd8d31466e52c290c1bfb17 and was meant to fix several bugs. I've changed the code to use, as in Android, textLayoutResult.size, and checked all the cases the commit refers to. All of them work correctly.

Because of this, and because it's much easier to revert to the Android version than to understand and agree on how the surrounding code should behave, then change and upstream it, I'd recommend reverting to the Android version.

@igordmn ?

okushnikov commented 3 months ago

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