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.

Sign up for GitHub

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

Fix shaky scrolling of LazyColumn when the items are of varying size #362

Merged
merged 1 commit into from
Jan 4, 2023
Merged

Fix shaky scrolling of LazyColumn when the items are of varying size #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 34 additions & 35 deletions ...dation/foundation/src/desktopMain/kotlin/androidx/compose/foundation/Scrollbar.desktop.kt
View file
Open in desktop
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
*/
dima-avdeev-jb marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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