Skip to content

Commit

Permalink
chore: refactor actionability check to go through node-side retry (#2…
Browse files Browse the repository at this point in the history 
…8982)

This allows to inject a checkpoint in between the actionability checks.

Drive-by: cleanup `InjectedScriptPoll`-related code.
  • Loading branch information
@dgozman
dgozman authored Jan 17, 2024
1 parent a3c38bc commit e6d51cf
Show file tree
Hide file tree
Showing 10 changed files with 202 additions and 368 deletions.
294 changes: 119 additions & 175 deletions packages/playwright-core/src/server/dom.ts
View file

Large diffs are not rendered by default.

4 changes: 1 addition & 3 deletions packages/playwright-core/src/server/frames.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { ManualPromise } from '../utils/manualPromise';
import { debugLogger } from '../common/debugLogger';
import type { CallMetadata } from './instrumentation';
import { serverSideCallMetadata, SdkObject } from './instrumentation';
import type { InjectedScript, ElementStateWithoutStable, FrameExpectParams, InjectedScriptPoll, InjectedScriptProgress } from './injected/injectedScript';
import type { InjectedScript, ElementStateWithoutStable, FrameExpectParams } from './injected/injectedScript';
import { isSessionClosedError } from './protocolError';
import { type ParsedSelector, isInvalidSelectorError } from '../utils/isomorphic/selectorParser';
import type { ScreenshotOptions } from './screenshotter';
Expand Down Expand Up @@ -80,8 +80,6 @@ export type NavigationEvent = {
isPublic?: boolean;
};

export type SchedulableTask<T> = (injectedScript: js.JSHandle<InjectedScript>) => Promise<js.JSHandle<InjectedScriptPoll<T>>>;
export type DomTaskBody<T, R, E> = (progress: InjectedScriptProgress, element: E, data: T, elements: Element[]) => R | symbol;
type ElementCallback<T, R> = (injected: InjectedScript, element: Element, data: T) => R;

export class NavigationAbortedError extends Error {
Expand Down
234 changes: 63 additions & 171 deletions packages/playwright-core/src/server/injected/injectedScript.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,31 +35,8 @@ import { asLocator } from '../../utils/isomorphic/locatorGenerators';
import type { Language } from '../../utils/isomorphic/locatorGenerators';
import { normalizeWhiteSpace, trimStringWithEllipsis } from '../../utils/isomorphic/stringUtils';

type Predicate<T> = (progress: InjectedScriptProgress) => T | symbol;

export type InjectedScriptProgress = {
injectedScript: InjectedScript;
continuePolling: symbol;
aborted: boolean;
log: (message: string) => void;
logRepeating: (message: string) => void;
};

export type LogEntry = {
message?: string;
};

export type FrameExpectParams = Omit<channels.FrameExpectParams, 'expectedValue'> & { expectedValue?: any };

export type InjectedScriptPoll<T> = {
run: () => Promise<T>,
// Takes more logs, waiting until at least one message is available.
takeNextLogs: () => Promise<LogEntry[]>,
// Takes all current logs without waiting.
takeLastLogs: () => LogEntry[],
cancel: () => void,
};

export type ElementStateWithoutStable = 'visible' | 'hidden' | 'enabled' | 'disabled' | 'editable' | 'checked' | 'unchecked';
export type ElementState = ElementStateWithoutStable | 'stable';

Expand Down Expand Up @@ -449,92 +426,6 @@ export class InjectedScript {
});
}

pollRaf<T>(predicate: Predicate<T>): InjectedScriptPoll<T> {
return this.poll(predicate, next => requestAnimationFrame(next));
}

private poll<T>(predicate: Predicate<T>, scheduleNext: (next: () => void) => void): InjectedScriptPoll<T> {
return this._runAbortableTask(progress => {
let fulfill: (result: T) => void;
let reject: (error: Error) => void;
const result = new Promise<T>((f, r) => { fulfill = f; reject = r; });

const next = () => {
if (progress.aborted)
return;
try {
const success = predicate(progress);
if (success !== progress.continuePolling)
fulfill(success as T);
else
scheduleNext(next);
} catch (e) {
progress.log(' ' + e.message);
reject(e);
}
};

next();
return result;
});
}

private _runAbortableTask<T>(task: (progess: InjectedScriptProgress) => Promise<T>): InjectedScriptPoll<T> {
let unsentLog: LogEntry[] = [];
let takeNextLogsCallback: ((logs: LogEntry[]) => void) | undefined;
let taskFinished = false;
const logReady = () => {
if (!takeNextLogsCallback)
return;
takeNextLogsCallback(unsentLog);
unsentLog = [];
takeNextLogsCallback = undefined;
};

const takeNextLogs = () => new Promise<LogEntry[]>(fulfill => {
takeNextLogsCallback = fulfill;
if (unsentLog.length || taskFinished)
logReady();
});

let lastMessage = '';
const progress: InjectedScriptProgress = {
injectedScript: this,
aborted: false,
continuePolling: Symbol('continuePolling'),
log: (message: string) => {
lastMessage = message;
unsentLog.push({ message });
logReady();
},
logRepeating: (message: string) => {
if (message !== lastMessage)
progress.log(message);
},
};

const run = () => {
const result = task(progress);

// After the task has finished, there should be no more logs.
// Release any pending `takeNextLogs` call, and do not block any future ones.
// This prevents non-finished protocol evaluation calls and memory leaks.
result.finally(() => {
taskFinished = true;
logReady();
});

return result;
};

return {
takeNextLogs,
run,
cancel: () => { progress.aborted = true; },
takeLastLogs: () => unsentLog,
};
}

getElementBorderWidth(node: Node): { left: number; top: number; } {
if (node.nodeType !== Node.ELEMENT_NODE || !node.ownerDocument || !node.ownerDocument.defaultView)
return { left: 0, top: 0 };
Expand Down Expand Up @@ -581,65 +472,73 @@ export class InjectedScript {
return element;
}

waitForElementStatesAndPerformAction<T>(node: Node, states: ElementState[], force: boolean | undefined,
callback: (node: Node, progress: InjectedScriptProgress) => T | symbol): InjectedScriptPoll<T | 'error:notconnected'> {
async checkElementStates(node: Node, states: ElementState[]): Promise<'error:notconnected' | { missingState: ElementState } | undefined> {
if (states.includes('stable')) {
const stableResult = await this._checkElementIsStable(node);
if (stableResult === false)
return { missingState: 'stable' };
if (stableResult === 'error:notconnected')
return stableResult;
}
for (const state of states) {
if (state !== 'stable') {
const result = this.elementState(node, state);
if (result === false)
return { missingState: state };
if (result === 'error:notconnected')
return result;
}
}
}

private async _checkElementIsStable(node: Node): Promise<'error:notconnected' | boolean> {
const continuePolling = Symbol('continuePolling');
let lastRect: { x: number, y: number, width: number, height: number } | undefined;
let counter = 0;
let samePositionCounter = 0;
let stableRafCounter = 0;
let lastTime = 0;

return this.pollRaf(progress => {
if (force) {
progress.log(` forcing action`);
return callback(node, progress);
const check = () => {
const element = this.retarget(node, 'no-follow-label');
if (!element)
return 'error:notconnected';

// Drop frames that are shorter than 16ms - WebKit Win bug.
const time = performance.now();
if (this._stableRafCount > 1 && time - lastTime < 15)
return continuePolling;
lastTime = time;

const clientRect = element.getBoundingClientRect();
const rect = { x: clientRect.top, y: clientRect.left, width: clientRect.width, height: clientRect.height };
if (lastRect) {
const samePosition = rect.x === lastRect.x && rect.y === lastRect.y && rect.width === lastRect.width && rect.height === lastRect.height;
if (!samePosition)
return false;
if (++stableRafCounter >= this._stableRafCount)
return true;
}
lastRect = rect;
return continuePolling;
};

for (const state of states) {
if (state !== 'stable') {
const result = this.elementState(node, state);
if (typeof result !== 'boolean')
return result;
if (!result) {
progress.logRepeating(` element is not ${state} - waiting...`);
return progress.continuePolling;
}
continue;
}
let fulfill: (result: 'error:notconnected' | boolean) => void;
let reject: (error: Error) => void;
const result = new Promise<'error:notconnected' | boolean>((f, r) => { fulfill = f; reject = r; });

const element = this.retarget(node, 'no-follow-label');
if (!element)
return 'error:notconnected';

// First raf happens in the same animation frame as evaluation, so it does not produce
// any client rect difference compared to synchronous call. We skip the synchronous call
// and only force layout during actual rafs as a small optimisation.
if (++counter === 1)
return progress.continuePolling;

// Drop frames that are shorter than 16ms - WebKit Win bug.
const time = performance.now();
if (this._stableRafCount > 1 && time - lastTime < 15)
return progress.continuePolling;
lastTime = time;

const clientRect = element.getBoundingClientRect();
const rect = { x: clientRect.top, y: clientRect.left, width: clientRect.width, height: clientRect.height };
const samePosition = lastRect && rect.x === lastRect.x && rect.y === lastRect.y && rect.width === lastRect.width && rect.height === lastRect.height;
if (samePosition)
++samePositionCounter;
const raf = () => {
try {
const success = check();
if (success !== continuePolling)
fulfill(success);
else
samePositionCounter = 0;
const isStable = samePositionCounter >= this._stableRafCount;
const isStableForLogs = isStable || !lastRect;
lastRect = rect;
if (!isStableForLogs)
progress.logRepeating(` element is not stable - waiting...`);
if (!isStable)
return progress.continuePolling;
requestAnimationFrame(raf);
} catch (e) {
reject(e);
}
};
requestAnimationFrame(raf);

return callback(node, progress);
});
return result;
}

elementState(node: Node, state: ElementStateWithoutStable): boolean | 'error:notconnected' {
Expand Down Expand Up @@ -675,9 +574,7 @@ export class InjectedScript {
throw this.createStacklessError(`Unexpected element state "${state}"`);
}

selectOptions(optionsToSelect: (Node | { valueOrLabel?: string, value?: string, label?: string, index?: number })[],
node: Node, progress: InjectedScriptProgress): string[] | 'error:notconnected' | symbol {

selectOptions(node: Node, optionsToSelect: (Node | { valueOrLabel?: string, value?: string, label?: string, index?: number })[]): string[] | 'error:notconnected' | 'error:optionsnotfound' {
const element = this.retarget(node, 'follow-label');
if (!element)
return 'error:notconnected';
Expand Down Expand Up @@ -713,19 +610,16 @@ export class InjectedScript {
break;
}
}
if (remainingOptionsToSelect.length) {
progress.logRepeating(' did not find some options - waiting... ');
return progress.continuePolling;
}
if (remainingOptionsToSelect.length)
return 'error:optionsnotfound';
select.value = undefined as any;
selectedOptions.forEach(option => option.selected = true);
progress.log(' selected specified option(s)');
select.dispatchEvent(new Event('input', { bubbles: true, composed: true }));
select.dispatchEvent(new Event('change', { bubbles: true }));
return selectedOptions.map(option => option.value);
}

fill(value: string, node: Node, progress: InjectedScriptProgress): 'error:notconnected' | 'needsinput' | 'done' {
fill(node: Node, value: string): 'error:notconnected' | 'needsinput' | 'done' {
const element = this.retarget(node, 'follow-label');
if (!element)
return 'error:notconnected';
Expand All @@ -734,10 +628,8 @@ export class InjectedScript {
const type = input.type.toLowerCase();
const kInputTypesToSetValue = new Set(['color', 'date', 'time', 'datetime-local', 'month', 'range', 'week']);
const kInputTypesToTypeInto = new Set(['', 'email', 'number', 'password', 'search', 'tel', 'text', 'url']);
if (!kInputTypesToTypeInto.has(type) && !kInputTypesToSetValue.has(type)) {
progress.log(` input of type "${type}" cannot be filled`);
if (!kInputTypesToTypeInto.has(type) && !kInputTypesToSetValue.has(type))
throw this.createStacklessError(`Input of type "${type}" cannot be filled`);
}
if (type === 'number') {
value = value.trim();
if (isNaN(Number(value)))
Expand Down
16 changes: 4 additions & 12 deletions packages/playwright-core/src/server/progress.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,9 @@ import type { LogName } from '../common/debugLogger';
import type { CallMetadata, Instrumentation, SdkObject } from './instrumentation';
import type { ElementHandle } from './dom';
import { ManualPromise } from '../utils/manualPromise';
import type { LogEntry } from './injected/injectedScript';

export interface Progress {
log(message: string): void;
logEntry(entry: LogEntry): void;
timeUntilDeadline(): number;
isRunning(): boolean;
cleanupWhenAborted(cleanup: () => any): void;
Expand Down Expand Up @@ -74,16 +72,10 @@ export class ProgressController {

const progress: Progress = {
log: message => {
progress.logEntry({ message });
},
logEntry: entry => {
if ('message' in entry) {
const message = entry.message!;
if (this._state === 'running')
this.metadata.log.push(message);
// Note: we might be sending logs after progress has finished, for example browser logs.
this.instrumentation.onCallLog(this.sdkObject, this.metadata, this._logName, message);
}
if (this._state === 'running')
this.metadata.log.push(message);
// Note: we might be sending logs after progress has finished, for example browser logs.
this.instrumentation.onCallLog(this.sdkObject, this.metadata, this._logName, message);
},
timeUntilDeadline: () => this._deadline ? this._deadline - monotonicTime() : 2147483647, // 2^31-1 safe setTimeout in Node.
isRunning: () => this._state === 'running',
Expand Down
3 changes: 2 additions & 1 deletion tests/page/elementhandle-scroll-into-view.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,5 +120,6 @@ it('should timeout waiting for visible', async ({ page, server }) => {
await page.setContent('<div style="display:none">Hello</div>');
const div = await page.$('div');
const error = await div.scrollIntoViewIfNeeded({ timeout: 3000 }).catch(e => e);
expect(error.message).toContain('element is not displayed, retrying in 100ms');
expect(error.message).toContain('element is not visible');
expect(error.message).toContain('retrying scroll into view action');
});
3 changes: 2 additions & 1 deletion tests/page/page-click-timeout-1.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,6 @@ it('should timeout waiting for button to be enabled', async ({ page }) => {
const error = await page.click('text=Click target', { timeout: 3000 }).catch(e => e);
expect(await page.evaluate('window.__CLICKED')).toBe(undefined);
expect(error.message).toContain('page.click: Timeout 3000ms exceeded.');
expect(error.message).toContain('element is not enabled - waiting');
expect(error.message).toContain('element is not enabled');
expect(error.message).toContain('retrying click action');
});
6 changes: 4 additions & 2 deletions tests/page/page-click-timeout-2.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ it('should timeout waiting for display:none to be gone', async ({ page, server }
const error = await page.click('button', { timeout: 5000 }).catch(e => e);
expect(error.message).toContain('page.click: Timeout 5000ms exceeded.');
expect(error.message).toContain('waiting for element to be visible, enabled and stable');
expect(error.message).toContain('element is not visible - waiting');
expect(error.message).toContain('element is not visible');
expect(error.message).toContain('retrying click action');
});

it('should timeout waiting for visibility:hidden to be gone', async ({ page, server }) => {
Expand All @@ -32,5 +33,6 @@ it('should timeout waiting for visibility:hidden to be gone', async ({ page, ser
const error = await page.click('button', { timeout: 5000 }).catch(e => e);
expect(error.message).toContain('page.click: Timeout 5000ms exceeded.');
expect(error.message).toContain('waiting for element to be visible, enabled and stable');
expect(error.message).toContain('element is not visible - waiting');
expect(error.message).toContain('element is not visible');
expect(error.message).toContain('retrying click action');
});
3 changes: 2 additions & 1 deletion tests/page/page-click-timeout-4.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ it('should timeout waiting for stable position', async ({ page, server }) => {
const error = await button.click({ timeout: 3000 }).catch(e => e);
expect(error.message).toContain('elementHandle.click: Timeout 3000ms exceeded.');
expect(error.message).toContain('waiting for element to be visible, enabled and stable');
expect(error.message).toContain('element is not stable - waiting');
expect(error.message).toContain('element is not stable');
expect(error.message).toContain('retrying click action');
});

it('should click for the second time after first timeout', async ({ page, server, mode }) => {
Expand Down
Loading

0 comments on commit e6d51cf

Please sign in to comment.