Skip to content

[JEWEL-921] Migrate .composed Calls to Modifier.Node API#3423

Open
DanielSouzaBertoldi wants to merge 1 commit intoJetBrains:masterfrom
DanielSouzaBertoldi:dsb/JEWEL-921
Open

[JEWEL-921] Migrate .composed Calls to Modifier.Node API#3423
DanielSouzaBertoldi wants to merge 1 commit intoJetBrains:masterfrom
DanielSouzaBertoldi:dsb/JEWEL-921

Conversation

@DanielSouzaBertoldi
Copy link
Collaborator

Context

About three years ago the Compose team created the Modifier.Node API to circumvent the performance issues that composed {} had. This PR migrates (or better yet, upgrades) our custom modifiers to use this new API

Changes

  • Updated the modifiers in Activation, Border, Scrollbar and Slider.
  • Added missing KDoc to public modifiers

Evidences

Activation

Scenario Screen Recording
Bridge
yeah.mp4
Standalone
yeah2.mp4

Border

yeah3.mp4

Scrollbar

Screen.Recording.2026-02-13.at.10.04.43.mov

Slider

Screen.Recording.2026-02-13.at.10.06.19.mov


Modifier.modifierLocalProvider(ModifierLocalActivated) { parentActivated }
override fun update(node: TrackActivationNode) {
// no-op
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detekt do be detekting

private class ActivatedModifierLocal : ModifierLocalProvider<Boolean>, ModifierLocalConsumer {
private var parentActivated: Boolean by mutableStateOf(false)
private class TrackActivationNode :
Modifier.Node(), FocusEventModifierNode, ModifierLocalModifierNode, ObserverModifierNode {
Copy link
Collaborator Author

@DanielSouzaBertoldi DanielSouzaBertoldi Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to use both ModifierLocalModifierNode and ObserverModifierNode to provide and consume local modifiers + get notified of changes in said modifiers.

It's basically the equivalent of onModifierLocalsUpdated previously used.

You can find a sample of ObserverModifier Node here

Comment on lines +171 to +172
override val shouldAutoInvalidate: Boolean = false
override val isImportantForBounds = false
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stole this from Compose Foundation's Border impl 😛 since we are invalidating our border modifier manually, we can safely avoid unnecessary invalidations

The importantForBounds is basically if talkback should highlight the component taking into account the set border (AFAIK). It's a useless info semantics-wise in the case of borders

You can check it here

Comment on lines +731 to +738
private val pointerInputNode =
delegate(
SuspendingPointerInputModifierNode {
val scroller = TrackPressScroller(coroutineScope, sliderAdapter, reverseLayout, clickBehavior)

detectScrollViaTrackGestures(isVertical = isVertical, scroller = scroller)
}
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We store the delegate in a property (instead of using an init block) because we need to reference it to call resetPointerInputHandler()

The TrackPressScroller is instantiated only once inside the suspending block, capturing the initial state of the parameters. If those parameters change, the scroller becomes stale, so we must manually reset the handler to restart the block and recreate the scroller

}

override fun onFocusEvent(focusState: FocusState) {
if (isFocused != focusState.isFocused) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify - are focusState.isFocused and focusState.hasFocus equivalent in this context?

My initial assumption was that a focusGroup node can never be focused itself, only its children can. This would mean focusState.isFocused is always false on TrackActivationNode, making updateProvidedValue() always compute parentActivated && false = false and silently breaking the activation chain.

To verify, I put together a minimal reproducer simulating trackWindowActivation providing true from above, with onActivated on a child layout node:

private data object SimulatedWindowActivationElement : ModifierNodeElement<SimulatedWindowActivationNode>() {
    override fun create() = SimulatedWindowActivationNode()
    override fun update(node: SimulatedWindowActivationNode) = Unit
    override fun InspectorInfo.inspectableProperties() { name = "simulatedWindowActivation" }
}

private class SimulatedWindowActivationNode : Modifier.Node(), ModifierLocalModifierNode {
    override val providedValues = modifierLocalMapOf(ModifierLocalActivated to true)
}

fun main(): Unit = application {
    Window(onCloseRequest = ::exitApplication) {
        IntUiTheme(theme = JewelTheme.lightThemeDefinition(), styling = ComponentStyling.default()) {
            var activationReceived by remember { mutableStateOf(false) }

            Column(Modifier.then(SimulatedWindowActivationElement).trackActivation()) {
                Text(
                    "Activation received: $activationReceived",
                    modifier = Modifier.onActivated { activationReceived = it },
                )
                OutlinedButton(onClick = {}) { Text("Click to focus") }
            }
        }
    }
}

I also added logging inside TrackActivationNode.onFocusEvent and updateProvidedValue:

override fun onFocusEvent(focusState: FocusState) {
    println("onFocusEvent: isFocused=${focusState.isFocused} hasFocus=${focusState.hasFocus}")
    println("node.isAttached=$isAttached")
    println("focusState=$focusState")
    println("focusState::class=${focusState::class}")
    if (isFocused != focusState.isFocused) {
        isFocused = focusState.isFocused
        updateProvidedValue()
    }
}

private fun updateProvidedValue() {
    println("updateProvidedValue: parentActivated=$parentActivated isFocused=$isFocused result=${parentActivated && isFocused}")
    provide(ModifierLocalActivated, parentActivated && isFocused)
}

Which produced the following output when clicking the button:

updateProvidedValue: parentActivated=true isFocused=false result=false
onFocusEvent: isFocused=false hasFocus=false
node.isAttached=true
focusState=Inactive
focusState::class=class androidx.compose.ui.focus.FocusStateImpl
onFocusEvent: isFocused=true hasFocus=true
node.isAttached=true
focusState=Active
focusState::class=class androidx.compose.ui.focus.FocusStateImpl
updateProvidedValue: parentActivated=true isFocused=true result=true

activationReceived correctly transitions to true, and isFocused and hasFocus move together. The TrackActivationNode receives FocusStateImpl.Active - not ActiveParent - when a child gains focus, which suggests onFocusEvent delivers the bubbled-up state from the event source rather than reflecting the layout node's own focus state in the tree hierarchy.

Is that the right understanding? If so, focusState.isFocused is correct here and the two properties are equivalent in this context.

Copy link
Collaborator

@rock3r rock3r Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr but isFocused == this node is the one owning the focus, hasFocus == this node or any of its children have the focus

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we could use focusState.isFocused or focusState.hasFocus interchangeably 😄

Given the nature of the code (active if the parent or children have focus), hasFocus is more semantically aligned with the intent, though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mine understanding was that the node focusGroup is attached to can't itself be focused but only it's children. That why I though focusState.isFocus would not be true, and focusState.hasFocus is the API we had to use here :)
Thanks for explaining this!
@DanielSouzaBertoldi I agree with you that usage of hasFocus is more semantically aligned and it's helps reader understand the code easier. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants