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.

Sign up for GitHub

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

Use only DOMHighResTimeStamp to calculate duration #143

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Use only DOMHighResTimeStamp to calculate duration #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"scripts": {
"test": "tsc && yarn run build && yarn test:playwright && testem ci && node test/headless/run",
"serve": "yarn run build && node test/headless/server/app",
"test:headless": "mocha --require @babel/register test/headless/specs/**/*.js --exit",
"test:headless": "mocha --require @babel/register test/headless/specs/**/*.js test/headless/specs/*.js --exit --timeout 5000",
"test:playwright": "playwright test --config=test/playwright/playwright.config.ts",
"watch": "broccoli-timepiece exports",
"build": "./scripts/build.sh",
Expand Down
43 changes: 38 additions & 5 deletions src/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,36 +28,69 @@ export interface SpanielObserverInit {
USE_NATIVE_IO?: boolean;
}

export interface TimeCompat {
highResTime: number;
unixTime: number;
}

export interface SpanielRecord {
target: SpanielTrackedElement;
payload: any;
thresholdStates: SpanielThresholdState[];
lastSeenEntry: IntersectionObserverEntry | null;
lastSeenEntry: InternalIntersectionObserverEntry | null;
}

export interface SpanielThresholdState {
lastSatisfied: Boolean;
lastEntry: IntersectionObserverEntry | null;
lastEntry: InternalIntersectionObserverEntry | null;
threshold: SpanielThreshold;
lastVisible: number;
lastVisible: TimeCompat;
visible: boolean;
timeoutId?: number;
}

export interface SpanielIntersectionObserverEntryInit {
time: DOMHighResTimeStamp;
highResTime: DOMHighResTimeStamp;
unixTime: number;
rootBounds: DOMRectPojo;
boundingClientRect: DOMRectPojo;
intersectionRect: DOMRectPojo;
target: SpanielTrackedElement;
}

export interface SpanielObserverEntry extends IntersectionObserverEntryInit {
export interface SpanielRect extends DOMRectPojo {
readonly height: number;
readonly width: number;
readonly x: number;
readonly y: number;
}

export interface SpanielObserverEntry {
isIntersecting: boolean;
duration: number;
visibleTime: number;
intersectionRatio: number;
entering: boolean;
label?: string;
payload?: any;
unixTime: number;
highResTime: number;
time: number;
target: Element;
boundingClientRect: SpanielRect;
intersectionRect: SpanielRect;
rootBounds: SpanielRect | null;
}

export interface InternalIntersectionObserverEntry {
time: number;
highResTime: DOMHighResTimeStamp;
target: Element;
boundingClientRect: SpanielRect;
intersectionRect: SpanielRect;
rootBounds: SpanielRect | null;
intersectionRatio: number;
isIntersecting: boolean;
}

export interface IntersectionObserverClass {
Expand Down
56 changes: 32 additions & 24 deletions src/intersection-observer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@ import { Frame, ElementScheduler, generateToken } from './metal/index';
import {
SpanielTrackedElement,
DOMString,
DOMRectPojo,
IntersectionObserverInit,
DOMMargin,
SpanielIntersectionObserverEntryInit
SpanielIntersectionObserverEntryInit,
InternalIntersectionObserverEntry,
DOMRectPojo
} from './interfaces';

interface EntryEvent {
entry: IntersectionObserverEntry;
entry: InternalIntersectionObserverEntry;
numSatisfiedThresholds: number;
}

Expand All @@ -37,7 +38,7 @@ function marginToRect(margin: DOMMargin): DOMRectPojo {
bottom,
right,
width: right - left,
height: bottom - top
height: bottom - top,
};
}

Expand Down Expand Up @@ -142,16 +143,17 @@ export class SpanielIntersectionObserver implements IntersectionObserver {
}
}

function addRatio(entryInit: SpanielIntersectionObserverEntryInit): IntersectionObserverEntry {
const { time, rootBounds, boundingClientRect, intersectionRect, target } = entryInit;
function addRatio(entryInit: SpanielIntersectionObserverEntryInit): InternalIntersectionObserverEntry {
const { unixTime, highResTime, rootBounds, boundingClientRect, intersectionRect, target } = entryInit;
const boundingArea = boundingClientRect.height * boundingClientRect.width;
const intersectionRatio = boundingArea > 0 ? (intersectionRect.width * intersectionRect.height) / boundingArea : 0;

return {
time,
rootBounds: pojoToToDOMRectReadOnly(rootBounds),
boundingClientRect: pojoToToDOMRectReadOnly(boundingClientRect),
intersectionRect: pojoToToDOMRectReadOnly(intersectionRect),
time: unixTime,
highResTime,
rootBounds,
boundingClientRect,
intersectionRect,
target,
intersectionRatio,
isIntersecting: calculateIsIntersecting({ intersectionRect })
Expand Down Expand Up @@ -183,28 +185,31 @@ export function generateEntry(
clientRect: ClientRect,
el: HTMLElement,
rootMargin: DOMMargin
): IntersectionObserverEntry {
): InternalIntersectionObserverEntry {
if (el.style.display === 'none') {
return {
time: frame.dateNow,
highResTime: frame.highResTime,
boundingClientRect: emptyRect(),
intersectionRatio: 0,
intersectionRect: emptyRect(),
isIntersecting: false,
rootBounds: emptyRect(),
target: el,
time: frame.timestamp
target: el
};
}
let { bottom, right } = clientRect;
const left = frame.left + rootMargin.left;
const top = frame.top + rootMargin.top;
let rootBounds: DOMRectPojo = {
left: frame.left + rootMargin.left,
top: frame.top + rootMargin.top,
x: frame.left + rootMargin.left,
y: frame.top + rootMargin.top,
left,
top,
bottom: rootMargin.bottom,
right: rootMargin.right,
width: frame.width - (rootMargin.right + rootMargin.left),
height: frame.height - (rootMargin.bottom + rootMargin.top)
height: frame.height - (rootMargin.bottom + rootMargin.top),
y: top,
x: left
};

let intersectX = Math.max(rootBounds.left, clientRect.left);
Expand All @@ -213,20 +218,23 @@ export function generateEntry(
let width = Math.min(rootBounds.left + rootBounds.width, clientRect.right) - intersectX;
let height = Math.min(rootBounds.top + rootBounds.height, clientRect.bottom) - intersectY;

const interLeft = width >= 0 ? intersectX : 0;
const interTop = intersectY >= 0 ? intersectY : 0;
let intersectionRect: DOMRectPojo = {
left: width >= 0 ? intersectX : 0,
top: intersectY >= 0 ? intersectY : 0,
x: width >= 0 ? intersectX : 0,
y: intersectY >= 0 ? intersectY : 0,
left: interLeft,
top: interTop,
x: interLeft,
y: interTop,
Comment on lines +226 to +227
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these new? I might have missed where are x and y read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it's just that we previously were not actually returning a https://developer.mozilla.org/en-US/docs/Web/API/DOMRect
so adding it to be closer to the native behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like ClientRect is a subset of SpanielRect so there's no need to do ClientRect & SpanielRect. I also noticed there's built-in DOMRectReadOnly from https://github.com/microsoft/TypeScript/blob/main/lib/lib.dom.d.ts. So

spaniel/src/interfaces.ts

Line 83 in 6b74e4d

export interface DOMRectReadOnly extends DOMRectInit, DOMMargin {}
and #143 (comment) might not be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I created export type DOMRectPojo = Omit<DOMRectReadOnly, 'toJSON'>;
Instead of ClientRect & SpanielRect

width,
height,
right,
bottom
};

return addRatio({
time: frame.timestamp,
rootBounds: pojoToToDOMRectReadOnly(rootBounds),
unixTime: frame.dateNow,
highResTime: frame.highResTime,
rootBounds,
target: <SpanielTrackedElement>el,
boundingClientRect: pojoToToDOMRectReadOnly(marginToRect(clientRect)),
intersectionRect: pojoToToDOMRectReadOnly(intersectionRect)
Expand Down
3 changes: 2 additions & 1 deletion src/metal/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ export interface ElementSchedulerInterface extends BaseSchedulerInterface {
}

export interface FrameInterface {
timestamp: number;
dateNow: number;
highResTime: DOMHighResTimeStamp;
scrollTop: number;
scrollLeft: number;
width: number;
Expand Down
4 changes: 3 additions & 1 deletion src/metal/scheduler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ let tokenCounter = 0;

export class Frame implements FrameInterface {
constructor(
public timestamp: number,
public dateNow: number,
public highResTime: number,
public scrollTop: number,
public scrollLeft: number,
public width: number,
Expand All @@ -47,6 +48,7 @@ export class Frame implements FrameInterface {
const rootMeta = this.revalidateRootMeta(root);
return new Frame(
Date.now(),
performance.now(),
rootMeta.scrollTop,
rootMeta.scrollLeft,
rootMeta.width,
Expand Down
57 changes: 44 additions & 13 deletions src/spaniel-observer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

import {
DOMMargin,
DOMRectPojo,
DOMString,
InternalIntersectionObserverEntry,
IntersectionObserverInit,
SpanielObserverEntry,
SpanielObserverInit,
Expand All @@ -26,7 +28,7 @@ import { generateToken, off, on, scheduleWork } from './metal/index';
import w from './metal/window-proxy';
import { calculateIsIntersecting } from './utils';

let emptyRect = { x: 0, y: 0, width: 0, height: 0 };
let emptyRect: DOMRectPojo = { x: 0, y: 0, width: 0, height: 0, bottom: 0, left: 0, top: 0, right: 0 };

export function DOMMarginToRootMargin(d: DOMMargin): DOMString {
return `${d.top}px ${d.right}px ${d.bottom}px ${d.left}px`;
Expand Down Expand Up @@ -108,7 +110,8 @@ export class SpanielObserver implements SpanielObserverInterface {
this.paused = false;

let ids = Object.keys(this.recordStore);
let time = this.generateObserverTimestamp();
const highResTime = performance.now();
const time = this.generateObserverTimestamp();
for (let i = 0; i < ids.length; i++) {
let entry = this.recordStore[ids[i]].lastSeenEntry;
if (entry) {
Expand All @@ -117,6 +120,7 @@ export class SpanielObserver implements SpanielObserverInterface {
intersectionRatio,
boundingClientRect,
time,
highResTime,
isIntersecting,
rootBounds,
intersectionRect,
Expand All @@ -134,41 +138,57 @@ export class SpanielObserver implements SpanielObserverInterface {
this.queuedEntries = [];
}
}
private generateSpanielEntry(entry: IntersectionObserverEntry, state: SpanielThresholdState): SpanielObserverEntry {
private generateSpanielEntry(
entry: InternalIntersectionObserverEntry,
state: SpanielThresholdState
): SpanielObserverEntry {
let { intersectionRatio, rootBounds, boundingClientRect, intersectionRect, isIntersecting, time, target } = entry;
let record = this.recordStore[(<SpanielTrackedElement>target).__spanielId];
const timeOrigin = w.performance.timeOrigin || w.performance.timing.navigationStart;
const unixTime = this.usingNativeIo ? Math.floor(timeOrigin + time) : time;
const unixTime = this.usingNativeIo
Copy link
Contributor

Choose a reason for hiding this comment

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

This ultimately becomes visibleTime in the watcher callback right?

We should stick to Date.now for start time (visibleTime), and use hr time for calculating the duration. Mixing timeOrigin / timing.navigationStart and Date.now is not reliable.

Pausing the hr time clock during sleep is okay for us to calculate impression duration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we use Date.now when using native intersection observer, that's also lossy because the callback is async. So capturing a Date.now() in the callback is not necessarily when the event happened. do you have an opinion on which approach is less bad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I don't think we are ever mixing timeOrigin / timing.navigationStart and Date.now for any individual entry (as in, https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserverEntry). I would rather use the HR time all throughout the entire entry or Date throughout the entire entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

The line below, Math.floor(w.performance.timeOrigin || w.performance.timing.navigationStart + time) is time unix time? The correct should be hrTime which is relative to timeOrigin. Logic looks incorrect to me.

Left another related comment at line 209.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the logic is indeed incorrect, do we have a test to cover?

Copy link
Contributor Author

@asakusuma asakusuma Aug 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The correct should be hrTime which is relative to timeOrigin

Sort of, it's correct if we want to mirror native IO, but that is a breaking change.

This change just modifies how duration is calculated.

Added tests for visibleTime earlier today: https://github.com/linkedin/spaniel/blob/v2-release/test/playwright/modules/visible-time.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah wait I see what you mean, how using native IO previously meant using hr time. I will add another test for this behavior first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait no we always are using unix timestamp time for the time field, even for native IO: https://github.com/linkedin/spaniel/blob/v2-release/src/spaniel-observer.ts#L141
So we should keep that behavior for now until a breaking change. I'll add a test regardless

? Math.floor((w.performance.timeOrigin || w.performance.timing.navigationStart) + time)
: time;
const highResTime = this.usingNativeIo ? time : entry.highResTime;
if (!highResTime) {
throw new Error('Missing intersection entry timestamp');
}
return {
intersectionRatio,
isIntersecting,
unixTime,
time: unixTime,
highResTime,
rootBounds,
boundingClientRect,
intersectionRect,
target: <SpanielTrackedElement>target,
duration: 0,
visibleTime: isIntersecting ? unixTime : -1,
entering: false,
payload: record.payload,
label: state.threshold.label
};
}
private handleRecordExiting(record: SpanielRecord) {
const time = Date.now();
const perfTime = performance.now();
record.thresholdStates.forEach((state: SpanielThresholdState) => {
const boundingClientRect = record.lastSeenEntry && record.lastSeenEntry.boundingClientRect;
this.handleThresholdExiting(
{
intersectionRatio: -1,
isIntersecting: false,
unixTime: time,
time,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is not used anywhere anymore, can we remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While it's true that it's technically not needed anymore (because time is a unix timestamp), in next major version, I want to make time the high res timestamp to mirror the native observer API as closely as possible, get rid of the highRes property, and keep the unixTime property if you need unix time (which we do need a unix timestamp for visibleTime.

Copy link
Contributor Author

@asakusuma asakusuma Aug 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Added tests for visibleTime earlier today: https://github.com/linkedin/spaniel/blob/v2-release/test/playwright/modules/visible-time-tests.ts

highResTime: perfTime,
payload: record.payload,
label: state.threshold.label,
entering: false,
rootBounds: emptyRect,
boundingClientRect: boundingClientRect || emptyRect,
intersectionRect: emptyRect,
duration: time - state.lastVisible,
visibleTime: state.lastVisible.unixTime,
// Next line (duration) is always overwritten if the record becomes a callback event
duration: perfTime - state.lastVisible.highResTime,
target: record.target
},
state
Expand All @@ -179,19 +199,20 @@ export class SpanielObserver implements SpanielObserverInterface {
});
}
private handleThresholdExiting(spanielEntry: SpanielObserverEntry, state: SpanielThresholdState) {
let { time } = spanielEntry;
let { highResTime } = spanielEntry;
let hasTimeThreshold = !!state.threshold.time;
if (state.lastSatisfied && (!hasTimeThreshold || (hasTimeThreshold && state.visible))) {
// Make into function
spanielEntry.duration = time - state.lastVisible;
spanielEntry.duration = highResTime - state.lastVisible.highResTime;
spanielEntry.visibleTime = state.lastVisible.unixTime;
spanielEntry.entering = false;
state.visible = false;
this.queuedEntries.push(spanielEntry);
}

clearTimeout(state.timeoutId);
}
private handleObserverEntry(entry: IntersectionObserverEntry) {
private handleObserverEntry(entry: InternalIntersectionObserverEntry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this.
handleObserverEntry is bound as IO callback, and receives hrTime, why switch to use InternalIntersectionObserverEntry which has extra fields? And it seems we generates unix time inside spaniel, but internalCallback will pass in hrTime.

let target = <SpanielTrackedElement>entry.target;
let record = this.recordStore[target.__spanielId];

Expand All @@ -217,19 +238,26 @@ export class SpanielObserver implements SpanielObserverInterface {
if (isSatisfied != state.lastSatisfied) {
if (isSatisfied) {
spanielEntry.entering = true;
state.lastVisible = spanielEntry.time;
state.lastVisible = {
highResTime: spanielEntry.highResTime,
unixTime: spanielEntry.unixTime
};
if (hasTimeThreshold) {
const timerId: number = Number(
setTimeout(() => {
state.visible = true;
spanielEntry.duration = Date.now() - state.lastVisible;
spanielEntry.duration = performance.now() - state.lastVisible.highResTime;
spanielEntry.visibleTime = state.lastVisible.unixTime;
this.callback([spanielEntry]);
}, state.threshold.time)
);
state.timeoutId = timerId;
} else {
state.visible = true;
spanielEntry.duration = Date.now() - state.lastVisible;
// TODO: Remove setting duration here, as it's irrelevant and should be very close to 0.
// It doesn't make sense to calculate duration when the entry represents entering, not
// exiting the viewport.
spanielEntry.duration = Date.now() - state.lastVisible.unixTime;
this.queuedEntries.push(spanielEntry);
}
} else {
Expand Down Expand Up @@ -286,7 +314,10 @@ export class SpanielObserver implements SpanielObserverInterface {
lastEntry: null,
threshold,
visible: false,
lastVisible: 0
lastVisible: {
unixTime: 0,
highResTime: -1
}
}))
};
this.observer.observe(trackedTarget);
Expand Down
Loading