Skip to content

Commit

Permalink
Fix shaky scrolling of LazyColumn when the items are of varying size …
Browse files Browse the repository at this point in the history 
…(issue compose-jb/2338). (#362)
  • Loading branch information
@m-sasha
m-sasha authored Jan 4, 2023
1 parent 5e4a90f commit de3c710
Show file tree
Hide file tree
Showing 2 changed files with 195 additions and 38 deletions.
69 changes: 34 additions & 35 deletions ...dation/foundation/src/desktopMain/kotlin/androidx/compose/foundation/Scrollbar.desktop.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,8 @@ private fun Scrollbar(
}

val minimalHeight = style.minimalHeight.toPx()
val sliderAdapter = remember(adapter, containerSize, minimalHeight, reverseLayout) {
SliderAdapter(adapter, containerSize, minimalHeight, reverseLayout)
val sliderAdapter = remember(adapter, containerSize, minimalHeight, reverseLayout, isVertical) {
SliderAdapter(adapter, containerSize, minimalHeight, reverseLayout, isVertical)
}

val scrollThickness = style.thickness.roundToPx()
Expand Down Expand Up @@ -250,11 +250,7 @@ private fun Scrollbar(
.scrollbarDrag(
interactionSource = interactionSource,
draggedInteraction = dragInteraction,
onStarted = { sliderAdapter.rawPosition = sliderAdapter.position },
onDelta = { offset ->
sliderAdapter.rawPosition += if (isVertical) offset.y else offset.x
},
onFinished = { sliderAdapter.rawPosition = sliderAdapter.position }
sliderAdapter = sliderAdapter,
)
)
},
Expand All @@ -265,28 +261,26 @@ private fun Scrollbar(
)
}


private fun Modifier.scrollbarDrag(
interactionSource: MutableInteractionSource,
draggedInteraction: MutableState<DragInteraction.Start?>,
onStarted: () -> Unit,
onDelta: (Offset) -> Unit,
onFinished: () -> Unit
sliderAdapter: SliderAdapter,
): Modifier = composed {
val currentInteractionSource by rememberUpdatedState(interactionSource)
val currentDraggedInteraction by rememberUpdatedState(draggedInteraction)
val currentOnStarted by rememberUpdatedState(onStarted)
val currentOnDelta by rememberUpdatedState(onDelta)
val currentOnFinished by rememberUpdatedState(onFinished)
val currentSliderAdapter by rememberUpdatedState(sliderAdapter)

pointerInput(Unit) {
forEachGesture {
awaitPointerEventScope {
val down = awaitFirstDown(requireUnconsumed = false)
val interaction = DragInteraction.Start()
currentInteractionSource.tryEmit(interaction)
currentDraggedInteraction.value = interaction
currentOnStarted.invoke()
currentSliderAdapter.onDragStarted()
val isSuccess = drag(down.id) { change ->
currentOnDelta.invoke(change.positionChange())
currentSliderAdapter.onDragDelta(change.positionChange())
change.consume()
}
val finishInteraction = if (isSuccess) {
Expand All @@ -296,7 +290,6 @@ private fun Modifier.scrollbarDrag(
}
currentInteractionSource.tryEmit(finishInteraction)
currentDraggedInteraction.value = null
currentOnFinished.invoke()
}
}
}
Expand Down Expand Up @@ -527,7 +520,8 @@ private class SliderAdapter(
val adapter: ScrollbarAdapter,
val containerSize: Int,
val minHeight: Float,
val reverseLayout: Boolean
val reverseLayout: Boolean,
val isVertical: Boolean,
) {
private val contentSize get() = adapter.maxScrollOffset(containerSize) + containerSize
private val visiblePart get() = containerSize.toFloat() / contentSize
Expand All @@ -544,40 +538,45 @@ private class SliderAdapter(
return if (extraContentSpace == 0f) 1f else extraScrollbarSpace / extraContentSpace
}

/**
* A position with cumulative offset, may be out of the container when dragging
*/
var rawPosition: Float = position
set(value) {
field = value
position = value
}

/**
* Actual scroll of content regarding slider layout
*/
private var scrollPosition: Float
private var rawPosition: Float
get() = scrollScale * adapter.scrollOffset
set(value) {
runBlocking {
adapter.scrollTo(containerSize, value / scrollScale)
}
}

/**
* Actual position of a thumb within slider container
*/
var position: Float
get() = if (reverseLayout) containerSize - size - scrollPosition else scrollPosition
get() = if (reverseLayout) containerSize - size - rawPosition else rawPosition
set(value) {
scrollPosition = if (reverseLayout) {
rawPosition = if (reverseLayout) {
containerSize - size - value
} else {
value
}
}

val bounds get() = position..position + size

// Stores the unrestricted position during a dragging gesture
private var positionDuringDrag = 0f

/** Called when the thumb dragging starts */
fun onDragStarted() {
positionDuringDrag = position
}

/** Called on every movement while dragging the thumb */
fun onDragDelta(offset: Offset) {
val dragDelta = if (isVertical) offset.y else offset.x
val maxScrollPosition = adapter.maxScrollOffset(containerSize) * scrollScale
val sliderDelta =
(positionDuringDrag + dragDelta).coerceIn(0f, maxScrollPosition) -
positionDuringDrag.coerceIn(0f, maxScrollPosition)
position += sliderDelta // Have to add to position for smooth content scroll if the items are of different size
positionDuringDrag += dragDelta
}

}

private fun verticalMeasurePolicy(
Expand Down
164 changes: 161 additions & 3 deletions ...foundation/foundation/src/desktopTest/kotlin/androidx/compose/foundation/ScrollbarTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.width
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.LazyListScope
import androidx.compose.foundation.lazy.LazyListState
import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.lazy.rememberLazyListState
Expand Down Expand Up @@ -139,20 +140,149 @@ class ScrollbarTest {
@Test
fun `drag outside slider and back`() {
runBlocking(Dispatchers.Main) {
val scale = 2f // Content distance to corresponding scrollbar distance
rule.setContent {
TestBox(size = 100.dp, childSize = 20.dp, childCount = 10, scrollbarWidth = 10.dp)
TestBox(size = 100.dp, childSize = 10.dp * scale, childCount = 10, scrollbarWidth = 10.dp)
}
rule.awaitIdle()

// While thumb is at the top, drag it up and then down the same distance. Content should not move.
rule.onNodeWithTag("scrollbar").performMouseInput {
moveTo(Offset(10f, 25f))
moveTo(Offset(0f, 25f))
press()
moveBy(Offset(0f, 50f))
moveBy(Offset(0f, -50f))
moveBy(Offset(0f, 50f))
release()
}
rule.awaitIdle()
rule.onNodeWithTag("box0").assertTopPositionInRootIsEqualTo(0.dp)

// While thumb is at the top, drag it up and then down a bit more. Content should move by the diff.
rule.onNodeWithTag("scrollbar").performMouseInput {
moveTo(Offset(0f, 25f))
press()
moveBy(Offset(0f, -50f))
moveBy(Offset(0f, 51f))
release()
}
rule.awaitIdle()
rule.onNodeWithTag("box0").assertTopPositionInRootIsEqualTo(-1.dp * scale)

// Drag thumb exactly to the end. Content should be at the bottom.
rule.onNodeWithTag("scrollbar").performMouseInput {
moveTo(Offset(0f, 25f))
press()
moveBy(Offset(0f, 50f))
release()
}
rule.onNodeWithTag("box0").assertTopPositionInRootIsEqualTo(-50.dp * scale)

// While thumb is at the bottom, drag it down and then up the same distance. Content should not move
rule.onNodeWithTag("scrollbar").performMouseInput {
moveTo(Offset(0f, 75f))
press()
moveBy(Offset(0f, 50f))
moveBy(Offset(0f, -50f))
release()
}
rule.onNodeWithTag("box0").assertTopPositionInRootIsEqualTo(-50.dp * scale)

// While thumb is at the bottom, drag it down and then up a bit more. Content should move by the diff
rule.onNodeWithTag("scrollbar").performMouseInput {
moveTo(Offset(0f, 75f))
press()
moveBy(Offset(0f, 50f))
moveBy(Offset(0f, -51f))
release()
}
rule.onNodeWithTag("box0").assertTopPositionInRootIsEqualTo(-49.dp * scale)

}
}

@Test
fun `drag slider with varying size items`() {
runBlocking(Dispatchers.Main) {
val listState = LazyListState()
rule.setContent {
LazyTestBox(state = listState, size = 100.dp, scrollbarWidth = 10.dp){
item {
Box(Modifier.size(20.dp))
}
item {
Box(Modifier.size(180.dp))
}
item {
Box(Modifier.size(20.dp))
}
item {
Box(Modifier.size(180.dp))
}
}
}
rule.awaitIdle()


rule.onNodeWithTag("scrollbar").performMouseInput {
moveTo(Offset(0f, 5f))
press()
}

// Scroll all the way down, one pixel at a time. Make sure the content moves "up" every time.
for (i in 1..100){
val firstVisibleItemIndexBefore = listState.firstVisibleItemIndex
val firstVisibleItemScrollOffsetBefore = listState.firstVisibleItemScrollOffset
rule.onNodeWithTag("scrollbar").performMouseInput {
moveBy(Offset(0f, 1f))
}
rule.awaitIdle()
val firstVisibleItemIndexAfter = listState.firstVisibleItemIndex
val firstVisibleItemScrollOffsetAfter = listState.firstVisibleItemScrollOffset

if (firstVisibleItemIndexAfter < firstVisibleItemIndexBefore)
throw AssertionError(
"First visible item index decreased on iteration $i while dragging down; " +
"before=$firstVisibleItemIndexBefore, after=$firstVisibleItemIndexAfter"
)
else if ((firstVisibleItemIndexAfter == firstVisibleItemIndexBefore) &&
(firstVisibleItemScrollOffsetAfter < firstVisibleItemScrollOffsetBefore))
throw AssertionError(
"First visible item offset decreased on iteration $i while dragging down; " +
"item index=$firstVisibleItemIndexAfter, " +
"offset before=$firstVisibleItemScrollOffsetBefore, " +
"offset after=$firstVisibleItemScrollOffsetAfter"
)
}

// Scroll back all the way up, one pixel at a time. Make sure the content moves "down" every time
for (i in 1..100){
val firstVisibleItemIndexBefore = listState.firstVisibleItemIndex
val firstVisibleItemScrollOffsetBefore = listState.firstVisibleItemScrollOffset
rule.onNodeWithTag("scrollbar").performMouseInput {
moveBy(Offset(0f, -1f))
}
rule.awaitIdle()
val firstVisibleItemIndexAfter = listState.firstVisibleItemIndex
val firstVisibleItemScrollOffsetAfter = listState.firstVisibleItemScrollOffset

if (firstVisibleItemIndexAfter > firstVisibleItemIndexBefore)
throw AssertionError(
"First visible item index increased on iteration $i while dragging up; " +
"before=$firstVisibleItemIndexBefore, after=$firstVisibleItemIndexAfter"
)
else if ((firstVisibleItemIndexAfter == firstVisibleItemIndexBefore) &&
(firstVisibleItemScrollOffsetAfter > firstVisibleItemScrollOffsetBefore))
throw AssertionError(
"First visible item offset increased on iteration $i while dragging up; " +
"item index=$firstVisibleItemIndexAfter, " +
"offset before=$firstVisibleItemScrollOffsetBefore, " +
"offset after=$firstVisibleItemScrollOffsetAfter"
)
}

rule.onNodeWithTag("scrollbar").performMouseInput {
release()
}
}
}

Expand Down Expand Up @@ -556,6 +686,34 @@ class ScrollbarTest {
}
}

@Suppress("SameParameterValue")
@Composable
private fun LazyTestBox(
state: LazyListState,
size: Dp,
scrollbarWidth: Dp,
reverseLayout: Boolean = false,
content: LazyListScope.() -> Unit,
) = withTestEnvironment {
Box(Modifier.size(size)) {
LazyColumn(
Modifier.fillMaxSize().testTag("column"),
state,
reverseLayout = reverseLayout,
content = content
)

VerticalScrollbar(
adapter = rememberScrollbarAdapter(state),
reverseLayout = reverseLayout,
modifier = Modifier
.width(scrollbarWidth)
.fillMaxHeight()
.testTag("scrollbar")
)
}
}

private fun MouseInjectionScope.instantDrag(start: Offset, end: Offset) {
moveTo(start)
press()
Expand Down

0 comments on commit de3c710

Please sign in to comment.