Skip to content

RUM-9030: Fix artifacts in Jetpack Compose scrolling#2610

Merged
jonathanmos merged 2 commits into
developfrom
jmoskovich/rum-9030/duplicate-artifacts
Apr 22, 2025
Merged

RUM-9030: Fix artifacts in Jetpack Compose scrolling#2610
jonathanmos merged 2 commits into
developfrom
jmoskovich/rum-9030/duplicate-artifacts

Conversation

@jonathanmos

@jonathanmos jonathanmos commented Apr 21, 2025

Copy link
Copy Markdown
Member

What does this PR do?

When scrolling a list in Compose occasionally the correct position of the element in the root coordinate space cannot immediately be retrieved and Compose will default to returning 0,0. In the replays this leads to a brief duplication of the translated element on the screen, thereby creating a visual artifact that harms the experience. In this PR we attempt to identify these artifacts by skipping all leaf nodes positioned at 0,0. While in some cases this may conceivably cause legitimate elements to be skipped, this is preferable to having the visual artifacts.

Motivation

Visual artifacts seen in replays.

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@jonathanmos
jonathanmos force-pushed the jmoskovich/rum-9030/duplicate-artifacts branch from f877308 to 57a6934 Compare April 21, 2025 12:16
@codecov-commenter

codecov-commenter commented Apr 21, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.10%. Comparing base (72376a1) to head (1928707).
Report is 135 commits behind head on develop.

Files with missing lines Patch % Lines
...sessionreplay/compose/internal/SemanticsNodeExt.kt 0.00% 2 Missing ⚠️
...ionreplay/compose/internal/utils/SemanticsUtils.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2610      +/-   ##
===========================================
+ Coverage    69.98%   70.10%   +0.12%     
===========================================
  Files          819      820       +1     
  Lines        30579    30583       +4     
  Branches      5121     5123       +2     
===========================================
+ Hits         21399    21439      +40     
+ Misses        7740     7729      -11     
+ Partials      1440     1415      -25     
Files with missing lines Coverage Δ
...ernal/mappers/semantics/RootSemanticsNodeMapper.kt 79.31% <100.00%> (+0.24%) ⬆️
...ionreplay/compose/internal/utils/SemanticsUtils.kt 70.93% <0.00%> (-0.41%) ⬇️
...sessionreplay/compose/internal/SemanticsNodeExt.kt 0.00% <0.00%> (ø)

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jonathanmos
jonathanmos marked this pull request as ready for review April 21, 2025 13:10
@jonathanmos
jonathanmos requested review from a team as code owners April 21, 2025 13:10
@jonathanmos jonathanmos changed the title RUM-9030: Fix artifacts in compose scrolling RUM-9030: Fix artifacts in Jetpack Compose scrolling Apr 21, 2025
0xnm
0xnm previously approved these changes Apr 22, 2025
mariusc83
mariusc83 previously approved these changes Apr 22, 2025
@jonathanmos
jonathanmos dismissed stale reviews from mariusc83 and 0xnm via 1928707 April 22, 2025 08:02
@ambushwork

Copy link
Copy Markdown
Member

very smart workaround,I think it will also improve the performance overhead a little bit since we are skipping some nodes, let's keep an eye on the benchmarking. Good job!

@jonathanmos
jonathanmos merged commit 6f1b7e7 into develop Apr 22, 2025
@jonathanmos
jonathanmos deleted the jmoskovich/rum-9030/duplicate-artifacts branch April 22, 2025 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants