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

BUG: waitForStylesheetLoad resumes playing of video when paused #268

Closed
Juice10 opened this issue Jul 27, 2020 · 4 comments · Fixed by #326
Closed

BUG: waitForStylesheetLoad resumes playing of video when paused #268

Juice10 opened this issue Jul 27, 2020 · 4 comments · Fixed by #326

Comments

@Juice10
Copy link
Contributor

Juice10 commented Jul 27, 2020

In some cases I've experienced that waitForStylesheetLoad will trigger a resume when the player should be paused.

      const { Replayer } = rrweb;
      const replayer = new Replayer(events);
      replayer.play(15000); // <-- setTimeout in waitForStylesheetLoad called
      replayer.pause(); // <-- should pause
// <-- setTimeout ends and triggers resume

Current contents of waitForStylesheetLoad:

rrweb/src/replay/index.ts

Lines 394 to 434 in 6cf3253

private waitForStylesheetLoad() {
const head = this.iframe.contentDocument?.head;
if (head) {
const unloadSheets: Set<HTMLLinkElement> = new Set();
let timer: number;
let beforeLoadState = this.service.state;
head
.querySelectorAll('link[rel="stylesheet"]')
.forEach((css: HTMLLinkElement) => {
if (!css.sheet) {
unloadSheets.add(css);
css.addEventListener('load', () => {
unloadSheets.delete(css);
// all loaded and timer not released yet
if (unloadSheets.size === 0 && timer !== -1) {
if (beforeLoadState.matches('playing')) {
this.resume(this.getCurrentTime());
}
this.emitter.emit(ReplayerEvents.LoadStylesheetEnd);
if (timer) {
window.clearTimeout(timer);
}
}
});
}
});
if (unloadSheets.size > 0) {
// find some unload sheets after iterate
this.service.send({ type: 'PAUSE' });
this.emitter.emit(ReplayerEvents.LoadStylesheetStart);
timer = window.setTimeout(() => {
if (beforeLoadState.matches('playing')) {
this.resume(this.getCurrentTime());
}
// mark timer was called
timer = -1;
}, this.config.loadTimeout);
}
}
}

@Juice10
Copy link
Contributor Author

Juice10 commented Jul 27, 2020

What might fix this is a different strategy that does not depend on replayer.play(number); replayer.pause() to seek and pause. Maybe we could allow people to use replayer.pause(number) to move the player to the point in time they need to pause at.

Alternatively we could also implement replayer.seek(number) if we are afraid changing pause is might break things.

@Yuyz0112
Copy link
Member

I believe the root cause is it only checks the beforeLoadState after the load.

So if we have these action sequence, it fails:

play
wait style load <- beforeLoadState is 'playing'
pause
loaded <- check beforeLoadState, do resume

pause at time offset only fix some use case, but users can still trigger this action sequence with the API.

@Yuyz0112
Copy link
Member

Yuyz0112 commented Aug 9, 2020

Please let me know if the fix does not work.

@Juice10
Copy link
Contributor Author

Juice10 commented Sep 4, 2020

Please let me know if the fix does not work.

@Yuyz0112 Apologies that it took me a while to verify this.
Unfortunately the fix doesn't work. Here is an events.json file that can be used to verify this issue.

Code to trigger the issue:

replayer.pause(2829);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants