Use onPlaced instead of onGloballyPositioned in PopupLayout

[m-sasha]

Use Modifier.onPlaced instead of Modifier.onGloballyPositioned to avoid calls when the window is moved.
Also use a MutableState instead of a delegate to it, in order to avoid an extra recomposition.

This also allows showing the popup without an extra recomposition because onPlaced is called in the layout phase following the initial composition when the popup is shown. It's also called before the popup measure policy is called, so there is not even a need to re-layout.

Testing

Tested manually and added a unit test.

import androidx.compose.foundation.background
import androidx.compose.foundation.border
import androidx.compose.foundation.layout.*
import androidx.compose.material.*
import androidx.compose.runtime.*
import androidx.compose.ui.*
import androidx.compose.ui.Modifier
import androidx.compose.ui.geometry.Offset
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.unit.*
import androidx.compose.ui.window.*

@OptIn(ExperimentalMaterialApi::class)
fun main() {
    singleWindowApplication {
        Box(Modifier.fillMaxSize()) {
            var offsetX by remember { mutableStateOf(0) }
            LaunchedEffect(Unit) {
                var direction = 1
                while (true) {
                    withFrameNanos {
                        offsetX += direction
                        if (offsetX > 100) {
                            direction = -1
                        } else if (offsetX <= 0) {
                            direction = 1
                        }
                    }
                }
            }
            val dropdownState = remember { DropdownMenuState() }
            Box(
                modifier = Modifier
                    .offset(x = offsetX.dp, y = 0.dp)
                    .size(400.dp)
                    .border(1.dp, Color.Black)
                    .contextMenuOpenDetector(dropdownState),
                contentAlignment = Alignment.Center
            ) {
                Text("Right click to open dropdown")

                val status = dropdownState.status
                if (status is DropdownMenuState.Status.Open) {
                    Popup(
                        onDismissRequest = { dropdownState.status = DropdownMenuState.Status.Closed },
                        popupPositionProvider = popupPositionProvider(status.position),
                        properties = PopupProperties(
                            clippingEnabled = false
                        )
                    ) {
                        Box(
                            modifier = Modifier
                                .size(300.dp, 400.dp)
                                .background(Color.Yellow)
                                .border(10.dp, Color.Black),
                            contentAlignment = Alignment.Center
                        ) {
                            Text("I'm a dropdown")
                        }
                    }
                }
            }
        }
    }
}

@Composable
private fun popupPositionProvider(offset: Offset): PopupPositionProvider {
    return remember(offset) {
        object : PopupPositionProvider {
            override fun calculatePosition(
                anchorBounds: IntRect,
                windowSize: IntSize,
                layoutDirection: LayoutDirection,
                popupContentSize: IntSize
            ): IntOffset {
                return anchorBounds.topLeft
            }
        }
    }
}

Release Notes

N/A

[m-sasha]

Added two reviewers, because you both have context, but only one is needed.

[MatkovIvan]

Why it's required to unroll this function? can we just replace the implementation inside?

[m-sasha]

I inlined it because

1. It's important to use onPlaced specifically, and a separate function hides that
2. I wanted to add a comment explaining how it works without a recomposition (or even a relayout) and such a comment only makes sense when all the context is present in one place.

[MatkovIvan]

I'm not sure I follow. How exactly wrapping modifier creation in function causes extra compositions?

[m-sasha]

It doesn't. Not using onPlaced does, and I wanted to highlight that.

[MatkovIvan]

I'd prefer keeping meaningfuly named functions over comments even if it's used once.
Probably not a blocker here

[MatkovIvan]

Can we keep this logic? Or add require/assert to make sure that it's initialized? I mean for possible future changes

[m-sasha]

I'd need to make the type nullable (IntRect?) again, which I prefer not to do.

But the unit test I added checks exactly this...

[MatkovIvan]

I'd prefer keeping meaningfuly named functions over comments even if it's used once.
Probably not a blocker here

[MatkovIvan]

Wait. It HAS TO be onGloballyPositioned.

Case: not full screen compose view opens Popup. This view is moved by platform. Popup must update the position.

[m-sasha]

After checking, there's a mechanism in Compose that makes sure to re-layout nodes that use LayoutCoordinates in their measuring/layout when the coordinates relative to window/screen change, so this is not a problem.

[MatkovIvan]

It's quite unexpected behavior that onPlaced is called in the same cases as onGloballyPositioned. It seems dangerous to rely on this.
Let's make sure that

• such cases are covered by tests
• it's confirmed that it's expected by Googlers (or file a bug)

[m-sasha]

It's not that onPlaced is called when onGloballyPositioned is.
It's that nodes that read LayoutCoordinates.windowToLocal get re-laid out when the window coordinates change.