-
Notifications
You must be signed in to change notification settings - Fork 142
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
♻️ [RUMF-1368] use the PointerDown event target for click actions #1731
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* dispatch `pointerdown` and `pointerup` events * add a duration between those events
BenoitZugmeyer
force-pushed
the
benoit/heatmap--get-pointerdown-target
branch
from
September 13, 2022 10:26
f9048b9
to
5d3342e
Compare
bcaudan
reviewed
Sep 14, 2022
packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #1731 +/- ##
=======================================
Coverage 90.97% 90.98%
=======================================
Files 128 128
Lines 4944 4958 +14
Branches 1101 1103 +2
=======================================
+ Hits 4498 4511 +13
- Misses 446 447 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
liywjl
approved these changes
Sep 15, 2022
bcaudan
approved these changes
Sep 15, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
packages/rum-core/src/domain/rumEventsCollection/action/listenActionEvents.spec.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/rumEventsCollection/action/trackClickActions.ts
Outdated
Show resolved
Hide resolved
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Click target inaccuracy
When the click starts (mouse/pointer down) on an element and ends (mouse/pointer up) on another element, the
click
event target is set to theBODY
element by the browser. This is problematic, because we report clicks occuring on theBODY
even if the user uses a whole other element. This impacts the RUM click action target name (@action.target.name
), as well as the experimental heatmaps CSS selector generation.Using the
pointerdown
event target instead improves the situation: the element used to compute the action target name and CSS selector is the element the user intended to click.Heatmap CSS selector improvement
Some application add a temporary class name when an element is being clicked (similarly to the
:active
CSS pseudo-class) or focused. Those temporary class names blocks us from finding again the element when rendering the Heatmap again.Generating the CSS selector before the application UI reacts to it should improve the situation a bit (
pointerdown
is the first event triggered when a click occurs).Frustration signal follow-up
This change is also a first step in fixing one of the most common dead click false positive cases: currently, we are only watching for page activity during or following a
click
event. But in some cases the application updates the UI earlier (ex: onmousedown
,focus
...). Those kind of clicks we are incorrectly considered a click as "dead".In a future follow-up, we'll fix this by watching page activity during or following the
pointerdown
event as well, to avoid marking those clicks as "dead".Changes
Beside a bit of refactoring, the main change was to update the
listenActionEvents
to call a callback onpointerdown
. This callback can return aonClick
closure that will be called during the nextclick
event. This allows to build the target-related properties earlier and still build theClick
during theclick
event.Testing
Notes
This PR does not fix the issue in the recorder part: clicks event will still be reported on the
body
element when the element target changes between mousedown and mouseup. This should not matter too much, as the click position is also collected.To keep backward compatibility, the
event
send as context in thebeforeSend
callback is still theclick
event.I have gone over the contributing documentation.