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

fix(rrweb): clean up pointer tap circles when seeking by breadcrumb #209

Merged
merged 6 commits into from
Jun 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/rrweb/src/replay/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,7 @@
sn?.type === NodeType.Element &&
sn?.tagName.toUpperCase() === 'HTML'
) {
const { documentElement, head } = iframeEl.contentDocument!;

Check warning on line 939 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L939

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
this.insertStyleRules(
documentElement as HTMLElement | RRElement,
head as HTMLElement | RRElement,
Expand All @@ -955,14 +955,14 @@
};

buildNodeWithSN(mutation.node, {
doc: iframeEl.contentDocument! as Document,

Check warning on line 958 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L958

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
mirror: mirror as Mirror,
hackCss: true,
skipChild: false,
afterAppend,
cache: this.cache,
});
afterAppend(iframeEl.contentDocument! as Document, mutation.node.id);

Check warning on line 965 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L965

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.

for (const { mutationInQueue, builtNode } of collected) {
this.attachDocumentToIframe(mutationInQueue, builtNode);
Expand Down Expand Up @@ -1049,7 +1049,7 @@
* pause when there are some canvas drawImage args need to be loaded
*/
private async preloadAllImages(): Promise<void[]> {
let beforeLoadState = this.service.state;

Check warning on line 1052 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L1052

[@typescript-eslint/no-unused-vars] 'beforeLoadState' is assigned a value but never used.
const stateHandler = () => {
beforeLoadState = this.service.state;
};
Expand Down Expand Up @@ -1084,8 +1084,8 @@
const ctx = canvas.getContext('2d');
const imgd = ctx?.createImageData(canvas.width, canvas.height);
let d = imgd?.data;
d = JSON.parse(data.args[0]) as Uint8ClampedArray;

Check warning on line 1087 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L1087

[@typescript-eslint/no-unused-vars] 'd' is assigned a value but never used.
ctx?.putImageData(imgd!, 0, 0);

Check warning on line 1088 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L1088

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
}
}
private async deserializeAndPreloadCanvasEvents(
Expand Down Expand Up @@ -1219,6 +1219,20 @@
if (isSync) {
if (d.type === MouseInteractions.TouchStart) {
pointer.touchActive = true;

// prevents multiple touch circles from staying on the screen
// when the user seeks by breadcrumbs
Object.values(this.pointers).forEach((p) => {
// don't set p.touchActive to false if p.touchActive is already true
// so that multitouch still works.
// p.touchActive can be null (in which case
// we still want to set it as false) - it's set as null
// in the ReplayerEvents.Flush handler after
// the 'touch-active' class is added or removed.
if (p !== pointer && !p.touchActive) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the !p.touchActive ensures that this works for multitouch when seeking - all taps should show up

p.touchActive = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm since the condition is !p.touchActive, wouldn't this mean touchActive is already false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if p.touchActive === true (this can be true for various pointers at the same time in the case of multitouch) then we don't want to set touchActive = false. otherwise we'd remove the pointer erroneously for multitouch. but touchActive can also be null for the pointers that aren't active, so those are the cases we want to set touchAcitve = false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we clarify this in a code comment? (and also what it means for it to be null vs false)

}
});
} else if (d.type === MouseInteractions.TouchEnd) {
pointer.touchActive = false;
pointer.pointerEl.remove();
Expand Down Expand Up @@ -1446,7 +1460,7 @@
// Only apply virtual dom optimization if the fast-forward process has node mutation. Because the cost of creating a virtual dom tree and executing the diff algorithm is usually higher than directly applying other kind of events.
if (this.config.useVirtualDom && !this.usingVirtualDom && isSync) {
this.usingVirtualDom = true;
buildFromDom(this.iframe.contentDocument!, this.mirror, this.virtualDom);

Check warning on line 1463 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L1463

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
// If these legacy missing nodes haven't been resolved, they should be converted to virtual nodes.
if (Object.keys(this.legacy_missingNodeRetryMap).length) {
for (const key in this.legacy_missingNodeRetryMap) {
Expand Down Expand Up @@ -1563,7 +1577,7 @@
// If the parent is attached a shadow dom after it's created, it won't have a shadow root.
if (!hasShadowRoot(parent)) {
(parent as Element | RRElement).attachShadow({ mode: 'open' });
parent = (parent as Element | RRElement).shadowRoot! as Node | RRNode;

Check warning on line 1580 in packages/rrweb/src/replay/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/replay/index.ts#L1580

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
} else parent = parent.shadowRoot as Node | RRNode;
}

Expand Down
181 changes: 181 additions & 0 deletions packages/rrweb/test/events/is-sync-tap.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
import {
EventType,
IncrementalSource,
MouseInteractions,
} from '@sentry-internal/rrweb-types';
import type { eventWithTime } from '../../../types/src';

const events: eventWithTime[] = [
{
type: 4,
data: {
href: '',
width: 1600,
height: 900,
},
timestamp: 0,
},
{
type: 2,
data: {
node: {
type: 0,
childNodes: [
{ type: 1, name: 'html', publicId: '', systemId: '', id: 2 },
{
type: 2,
tagName: 'html',
attributes: { lang: 'en' },
childNodes: [
{
type: 2,
tagName: 'head',
attributes: {},
childNodes: [
{
id: 101,
type: 2,
tagName: 'style',
attributes: {},
childNodes: [
{
id: 102,
type: 3,
isStyle: true,
textContent: 'div:hover { background-color: gold; }',
},
],
},
],
id: 4,
},
{ type: 3, textContent: '\n ', id: 13 },
{
type: 2,
tagName: 'body',
attributes: {},
childNodes: [
{ type: 3, textContent: '\n ', id: 15 },
{
type: 2,
tagName: 'div',
attributes: {
style:
'border: 1px solid #000000; width: 100px; height: 100px;',
},
childNodes: [{ type: 3, textContent: '\n ', id: 17 }],
id: 16,
},
],
id: 14,
},
],
id: 3,
},
],
id: 1,
},
initialOffset: { left: 0, top: 0 },
},
timestamp: 10,
},
{
type: 3,
data: {
source: IncrementalSource.MouseInteraction,
type: MouseInteractions.TouchStart,
id: 16,
x: 30,
y: 30,
pointerId: 0,
},
timestamp: 100,
},
{
type: 3,
data: {
source: IncrementalSource.TouchMove,
positions: [
{
id: 0,
x: 149.436,
y: 433.929,
timeOffset: 30,
},
{
id: 1,
x: 243.436,
y: 155.929,
timeOffset: 0,
},
],
pointerId: 0,
},
timestamp: 150,
},
{
type: 3,
data: {
source: IncrementalSource.MouseInteraction,
type: MouseInteractions.TouchEnd,
id: 16,
x: 30,
y: 30,
pointerId: 0,
},
timestamp: 155,
},
{
type: 3,
data: {
source: IncrementalSource.MouseInteraction,
type: MouseInteractions.TouchStart,
id: 16,
x: 30,
y: 30,
pointerId: 1,
},
timestamp: 160,
},
{
type: 3,
data: {
source: IncrementalSource.TouchMove,
positions: [
{
id: 0,
x: 149.436,
y: 433.929,
timeOffset: 30,
},
{
id: 1,
x: 243.436,
y: 155.929,
timeOffset: 0,
},
],
pointerId: 1,
},
timestamp: 170,
},
{
type: 3,
data: {
source: IncrementalSource.MouseInteraction,
type: MouseInteractions.TouchEnd,
id: 16,
x: 30,
y: 30,
pointerId: 1,
},
timestamp: 180,
},
{
type: EventType.IncrementalSnapshot,
data: { source: IncrementalSource.Scroll, id: 1, x: 0, y: 250 },
timestamp: 220,
},
];

export default events;
39 changes: 36 additions & 3 deletions packages/rrweb/test/replayer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import touchAllPointerEvents from './events/touch-all-pointer-id';
import touchSomePointerEvents from './events/touch-some-pointer-id';
import mouseEvents from './events/mouse';
import scrollEvents from './events/scroll';
import isSyncTapEvents from './events/is-sync-tap';
import scrollWithParentStylesEvents from './events/scroll-with-parent-styles';
import inputEvents from './events/input';
import iframeEvents from './events/iframe';
Expand Down Expand Up @@ -881,7 +882,7 @@ describe('replayer', function () {
`);

// No pointer should exist yet
await expect(
expect(
await page.evaluate(
() => document.querySelectorAll('.replayer-mouse')!.length,
),
Expand All @@ -892,7 +893,7 @@ describe('replayer', function () {
`);

// One mouse pointer should exist
await expect(
expect(
await page.evaluate(
() => document.querySelectorAll('.replayer-mouse')!.length,
),
Expand All @@ -903,13 +904,45 @@ describe('replayer', function () {
`);

// Pointer should still exist after all events execute
await expect(
expect(
await page.evaluate(
() => document.querySelectorAll('.replayer-mouse')!.length,
),
).toEqual(1);
});

it('removes tap circles from the screen when isSync = true', async () => {
await page.evaluate(`events = ${JSON.stringify(isSyncTapEvents)}`);
await page.evaluate(`
const { Replayer } = rrweb;
const replayer = new Replayer(events);
`);

// No pointer should exist yet
expect(
await page.evaluate(
() => document.querySelectorAll('.replayer-mouse')!.length,
),
).toEqual(0);

// Seek to second tap
await page.evaluate(`
replayer.pause(161);
`);

// Seek to first tap
await page.evaluate(`
replayer.pause(101);
`);

// Only one tap should exist, not both
expect(
await page.evaluate(
() => document.querySelectorAll('.touch-active')!.length,
),
).toEqual(1);
});

it('should destroy the replayer after calling destroy()', async () => {
await page.evaluate(`events = ${JSON.stringify(events)}`);
await page.evaluate(`
Expand Down
Loading