The desktop pointer cursor is not representative of mobile#662
Merged
Yuyz0112 merged 8 commits intorrweb-io:masterfrom Aug 17, 2021
Merged
The desktop pointer cursor is not representative of mobile#662Yuyz0112 merged 8 commits intorrweb-io:masterfrom
Yuyz0112 merged 8 commits intorrweb-io:masterfrom
Conversation
eoghanmurray
referenced
this pull request
Aug 11, 2021
4dd1bbd to
97d42a0
Compare
…to index.ts from machine.ts (so they can be dealt with in bulk)
…o the last position so that it's in the correct place when the timer starts - previous `needCastInSyncMode` added in 4bf533a meant that the isSync versions of MouseMove/TouchMove were being accidentally ignored - each synchronous MouseMove would have resulted in a separate mouse position update - the Click/TouchStart/TouchEnd events didn't have an async version
97d42a0 to
44de72b
Compare
… on a mobile device. Instead, check a recording for any presence of a Touch event, and switch to a touch visualisation mode for the entire recording. (for now, we use this mode even for mixed touch/mouse devices - this could be improved upon in future) Show a round circle representing the users' finger which is visible only between TouchStart and TouchEnd events Again this can be evolved upon, but this change should be a good start in the right direction.
… as user can lift finger off screen and place elsewhere; however we can now have much smoother touch movement during the .touch-active phase as we know the finger is on the screen. This has a .25s delaying effect on the touch position which IMO is acceptable; e.g. scroll position can lag behind a touch movement and this seems to bring them more in sync
…f synchronous events
44de72b to
72bf5e1
Compare
…n user has lifted their finger and placed it into a new position. This is apparent in a replay session where the user is scrolling the page using repeated TouchMove bottom-to-top movements
Juice10
approved these changes
Aug 14, 2021
Yuyz0112
reviewed
Aug 14, 2021
packages/rrweb/src/replay/index.ts
Outdated
|
|
||
| private newDocumentQueue: addedNodeMutation[] = []; | ||
|
|
||
| private mouseState: mouseState | null = null; |
Member
There was a problem hiding this comment.
The code to maintain this state looks a bit complex. Is it possible to use a getter to compute whenever need?
Contributor
Author
There was a problem hiding this comment.
It's supposed to be a lightweight object to hold mouse x/y and whether touch is active or not. The extra id (and debugData) is for moveAndHover.
It's only relevant during application of 'sync' events, to maintain a record of the last change to mouse state, and is cleared after each batch.
I'm not sure what a getter would achieve?
Contributor
Author
There was a problem hiding this comment.
Ok actually some of the complexity comes from me trying to wrap two separate things into the same state object. I've separated them out in 5606987 and this should make things clearer.
…into their own global vars
Member
|
Good Job! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The desktop pointer cursor is not representative of what is happening on a mobile device.
Instead, check a recording for any presence of a Touch event, and switch to a touch visualisation mode for the entire recording.
(for now, we use this mode even for mixed touch/mouse devices - this could be improved upon in future)
Show a round circle representing the users' finger which is visible only between TouchStart and TouchEnd events
Again this can be evolved upon, but this change should be a good start in the right direction.