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

Jump to bottom

fix(screenshot): do not stall on hideHiglight attempt 2 #13222

Merged
merged 1 commit into from
Mar 31, 2022
Merged

fix(screenshot): do not stall on hideHiglight attempt 2 #13222

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 14 additions & 8 deletions packages/playwright-core/src/server/screenshotter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,11 +238,18 @@ export class Screenshotter {
}));
}

async _maskElements(progress: Progress, options: ScreenshotOptions) {
async _maskElements(progress: Progress, options: ScreenshotOptions): Promise<() => Promise<void>> {
const framesToParsedSelectors: MultiMap<Frame, ParsedSelector> = new MultiMap();

const cleanup = async () => {
await Promise.all([...framesToParsedSelectors.keys()].map(async frame => {
await frame.hideHighlight();
}));
};

if (!options.mask || !options.mask.length)
return false;
return cleanup;

const framesToParsedSelectors: MultiMap<Frame, ParsedSelector> = new MultiMap();
await Promise.all((options.mask || []).map(async ({ frame, selector }) => {
const pair = await frame.resolveFrameForSelectorNoWait(selector);
if (pair)
Expand All @@ -253,8 +260,8 @@ export class Screenshotter {
await Promise.all([...framesToParsedSelectors.keys()].map(async frame => {
await frame.maskSelectors(framesToParsedSelectors.get(frame));
}));
progress.cleanupWhenAborted(() => this._page.hideHighlight());
return true;
progress.cleanupWhenAborted(cleanup);
return cleanup;
}

private async _screenshot(progress: Progress, format: 'png' | 'jpeg', documentRect: types.Rect | undefined, viewportRect: types.Rect | undefined, fitsViewport: boolean, options: ScreenshotOptions): Promise<Buffer> {
Expand All @@ -268,14 +275,13 @@ export class Screenshotter {
}
progress.throwIfAborted(); // Avoid extra work.

const hasHighlight = await this._maskElements(progress, options);
const cleanupHighlight = await this._maskElements(progress, options);
progress.throwIfAborted(); // Avoid extra work.

const buffer = await this._page._delegate.takeScreenshot(progress, format, documentRect, viewportRect, options.quality, fitsViewport, options.size || 'device');
progress.throwIfAborted(); // Avoid restoring after failure - should be done by cleanup.

if (hasHighlight)
await this._page.hideHighlight();
await cleanupHighlight();
progress.throwIfAborted(); // Avoid restoring after failure - should be done by cleanup.

if (shouldSetDefaultBackground)
Expand Down
13 changes: 13 additions & 0 deletions tests/page/page-screenshot.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,19 @@ it.describe('page screenshot', () => {
await route.fulfill({ body: '' });
await done;
});

it('should work when subframe used document.open after a weird url', async ({ page, server }) => {
await page.goto(server.EMPTY_PAGE);
await page.evaluate(() => {
const iframe = document.createElement('iframe');
iframe.src = 'javascript:hi';
document.body.appendChild(iframe);
iframe.contentDocument.open();
iframe.contentDocument.write('Hello');
iframe.contentDocument.close();
});
await page.screenshot({ mask: [ page.locator('non-existent') ] });
});
});
});

Expand Down