Skip to content

RUM-3868: Don't traverse non-visible ViewGroups for searching user interaction targets#1969

Merged
0xnm merged 1 commit into
developfrom
nogorodnikov/rum-3868/dont-traverse-non-visible-viewgroups-for-searching-user-interaction-targets
Apr 8, 2024
Merged

RUM-3868: Don't traverse non-visible ViewGroups for searching user interaction targets#1969
0xnm merged 1 commit into
developfrom
nogorodnikov/rum-3868/dont-traverse-non-visible-viewgroups-for-searching-user-interaction-targets

Conversation

@0xnm

@0xnm 0xnm commented Apr 5, 2024

Copy link
Copy Markdown
Member

What does this PR do?

This a fix for the following bug: let's imagine we have a container view and a child view in this container, both are visible. If we set View.GONE or View.INVISIBLE for the container view, both views won't be visible, but if we click on the place where child view would be rendered, the tap will be reported.

This happens because when we hide container view, child view still have a visibility View.VISIBLE despite not participating in rendering (if parent group has View.INVISIBLE) or even in the layout process (if parent group has View.GONE).

This means we need to take into account the visibility of the view groups as well when we are searching for the user interaction target.

Note: child cannot be shown without parent shown, meaning even if we have View.INVISIBLE for parent container switching visibility for child view has no effect (at least for built-in view groups as I tested).

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)

@0xnm
0xnm requested review from a team as code owners April 5, 2024 12:29

@xgouchet xgouchet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice catch !
And I think you won the longest branch name contest too :D

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #1969 (ff9bc4f) into develop (337e299) will decrease coverage by 0.03%.
Report is 2 commits behind head on develop.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1969      +/-   ##
===========================================
- Coverage    83.35%   83.33%   -0.03%     
===========================================
  Files          489      489              
  Lines        17900    17897       -3     
  Branches      2669     2669              
===========================================
- Hits         14920    14913       -7     
- Misses        2243     2245       +2     
- Partials       737      739       +2     
Files Coverage Δ
...ernal/instrumentation/gestures/GesturesListener.kt 93.15% <100.00%> (+0.79%) ⬆️

... and 30 files with indirect coverage changes

@0xnm
0xnm merged commit 0706c32 into develop Apr 8, 2024
@0xnm
0xnm deleted the nogorodnikov/rum-3868/dont-traverse-non-visible-viewgroups-for-searching-user-interaction-targets branch April 8, 2024 09:40
@xgouchet xgouchet modified the milestones: 2.11.x, 2.7.x Jul 31, 2024
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.

4 participants