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.

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

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

Conversation

0xnm
Copy link
Member

@0xnm 0xnm commented Apr 5, 2024

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
Copy link
Member

@xgouchet xgouchet left a comment

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

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
23 checks passed
@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