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

Render full snapshot on scrub to zero #1140

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

eoghanmurray
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Feb 16, 2023

🦋 Changeset detected

Latest commit: ea0be35

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb Minor
@rrweb/types Minor
rrweb-snapshot Minor
rrdom Minor
rrdom-nodejs Minor
rrweb-player Minor
@rrweb/web-extension Minor
rrvideo Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@eoghanmurray eoghanmurray force-pushed the renderFullSnapshotOnScrubToZero branch from bddfd78 to 3064119 Compare February 22, 2023 13:46
@Juice10
Copy link
Contributor

Juice10 commented Mar 3, 2023

Tests are failing, maybe you could add a test for this use case if the current failing tests don't cover it. Apart from that this looks good and can be approved/merged

eoghanmurray and others added 16 commits April 13, 2023 12:29
…circle rather than a point during replay

 - We have to switch to 'onpointerdown' & 'onpointerup' in order to actually capture `e.pointerType`
 - this replaces 4 event listeners (MouseDown/MouseUp/TouchStart/TouchEnd) with 2 pointer ones which should fire in all 4 scenarios. We still output the old types according to the MouseInteractions enum
 - there is no Pointer equivalent of Click, so we leave that is, but use the last Pointer event to attach a pointerType to (only) the click event, where it is most useful
 - we can fallback to the old method for any browsers not supporting `window.PointerEvent`, in which case \`pointerType\` will be absent from all events
… ended up as a TouchEvent instead of an individual Touch object I think
… where a subsequent click would be picked up as a touch (see `thisEventKey` change)
… should be applied synchronously

 - where this came up:
     replayer.play(timestamp_of_a_snapshot)
     replayer.pause()

Because of the immediate pause, the timer requestAnimationFrame never got to execute, and the snapshot wasn't rendered.

Of course this could also have been achieved with:

     replayer.pause(timestamp_of_a_snapshot)

But I didn't know that
…der a FullSnapshot, similar to how the firstFullsnapshot logic works upon initilization of the player
@eoghanmurray
Copy link
Contributor Author

eoghanmurray commented Apr 13, 2023

Tests are failing as this PR makes things more complicated!
e.g. 'will start actions when play' now has 2 less actions queued up as it has already 'sync executed' the first action which is a DomContentLoaded as well as a subsequent FullSnapshot.
I've just subtract 2 from the number of events expected, but not sure if this is a positive change to the tests?

@eoghanmurray eoghanmurray force-pushed the renderFullSnapshotOnScrubToZero branch from 67ddf68 to 3cebc69 Compare April 13, 2023 11:48
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.

2 participants