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: skip unloaded iframes for all apis #752

Merged
merged 6 commits into from
Jun 15, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion packages/playwright/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions packages/playwright/test/axe-playwright.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,24 @@ describe('@axe-core/playwright', () => {
assert.equal(res?.status(), 200);
assert.strictEqual(count, 9);
});

it('handles unloaded iframes (e.g. loading=lazy)', async () => {
const res = await page.goto(`${addr}/external/lazy-loaded-iframe.html`);

const results = await new AxeBuilder({ page })
.options({ runOnly: ['label', 'frame-tested'] })
.analyze();

assert.equal(res?.status(), 200);
assert.lengthOf(results.incomplete, 0);
assert.equal(results.violations[0].id, 'label');
assert.lengthOf(results.violations[0].nodes, 1);
assert.deepEqual(results.violations[0].nodes[0].target, [
'#ifr-lazy',
'#lazy-baz',
'input'
]);
})
});

describe('include/exclude', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/puppeteer/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion packages/puppeteer/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,16 @@ export async function assertFrameReady(frame: Frame): Promise<void> {
// Check if the page is loaded.
let pageReady = false;
try {
pageReady = await frame.evaluate(pageIsLoaded);
// Puppeteer freezes on unloaded iframes. Set a race timeout in order to handle that.
// @see https://github.com/dequelabs/axe-core-npm/issues/727
const timeoutPromise = new Promise(resolve => {
setTimeout(() => {
resolve('timeout');
straker marked this conversation as resolved.
Show resolved Hide resolved
}, 1000)
});
const evaluatePromise = frame.evaluate(pageIsLoaded);
const raceResults = await Promise.race([timeoutPromise, evaluatePromise]);
pageReady = raceResults !== 'timeout';
} catch {
/* ignore */
}
Expand Down
19 changes: 19 additions & 0 deletions packages/puppeteer/test/axe-puppeteer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -872,6 +872,25 @@ describe('AxePuppeteer', function () {
assert.equal(res?.status(), 200);
assert.deepEqual(pageResults, frameResults);
});

it('skips unloaded iframes (e.g. loading=lazy)', async () => {
const res = await page.goto(`${addr}/external/lazy-loaded-iframe.html`);
const results = await new AxePuppeteer(page)
.options({ runOnly: ['label', 'frame-tested'] })
.analyze();

assert.equal(res?.status(), 200);
assert.equal(results.incomplete[0].id, 'frame-tested');
assert.lengthOf(results.incomplete[0].nodes, 1);
assert.deepEqual(results.incomplete[0].nodes[0].target, ['#ifr-lazy', '#lazy-iframe']);
assert.equal(results.violations[0].id, 'label');
assert.lengthOf(results.violations[0].nodes, 1);
assert.deepEqual(results.violations[0].nodes[0].target, [
'#ifr-lazy',
'#lazy-baz',
'input'
]);
})
});

describe('axe.finishRun errors', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/webdriverio/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 28 additions & 5 deletions packages/webdriverio/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import {
axeRunPartial,
axeFinishRun,
axeRunLegacy,
configureAllowedOrigins
configureAllowedOrigins,
FRAME_LOAD_TIMEOUT
} from './utils';
import { getFilename } from 'cross-dirname';
import { pathToFileURL } from 'url';
Expand Down Expand Up @@ -241,7 +242,21 @@ export default class AxeBuilder {
return await this.runLegacy(context);
}

const partials = await this.runPartialRecursive(context);
// ensure we fail quickly if an iframe cannot be loaded (instead of waiting
// the default length of 30 seconds)
const { pageLoad } = await this.client.getTimeouts();
this.client.setTimeout({
pageLoad: FRAME_LOAD_TIMEOUT,
});

let partials: PartialResults | null
try {
partials = await this.runPartialRecursive(context);
} finally {
this.client.setTimeout({
pageLoad,
});
}

try {
return await this.finishRun(partials);
Expand Down Expand Up @@ -300,8 +315,10 @@ export default class AxeBuilder {
*/

private async runPartialRecursive(
context: SerialContextObject
context: SerialContextObject,
frameStack: Element[] = []
): Promise<PartialResults> {
const title = await this.client.getTitle();
straker marked this conversation as resolved.
Show resolved Hide resolved
const frameContexts = await axeGetFrameContext(this.client, context);
const partials: PartialResults = [
await axeRunPartial(this.client, context, this.option)
Expand All @@ -313,10 +330,16 @@ export default class AxeBuilder {
assert(frame, `Expect frame of "${frameSelector}" to be defined`);
await this.client.switchToFrame(frame);
await axeSourceInject(this.client, this.script);
partials.push(...(await this.runPartialRecursive(frameContext)));
partials.push(...(await this.runPartialRecursive(frameContext, [...frameStack, frame])));
} catch (error) {
const windows = await this.client.getWindowHandles()
await this.client.switchToWindow(windows[0])
straker marked this conversation as resolved.
Show resolved Hide resolved

for (const frameElm of frameStack) {
await this.client.switchToFrame(frameElm);
}

partials.push(null);
await this.client.switchToParentFrame();
}
}
await this.client.switchToParentFrame();
Expand Down
36 changes: 36 additions & 0 deletions packages/webdriverio/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import type {
SerialContextObject
} from 'axe-core';

export const FRAME_LOAD_TIMEOUT = 1000

/**
* Validates that the client provided is WebdriverIO v5 or v6.
*/
Expand Down Expand Up @@ -83,6 +85,7 @@ export const axeSourceInject = async (
client: Browser,
axeSource: string
): Promise<{ runPartialSupported: boolean }> => {
await assertFrameReady(client);
return promisify(
// Had to use executeAsync() because we could not use multiline statements in client.execute()
// we were able to return a single boolean in a line but not when assigned to a variable.
Expand All @@ -98,6 +101,39 @@ export const axeSourceInject = async (
);
};

async function assertFrameReady(client: Browser): Promise<void> {
// Wait so that we know there is an execution context.
// Assume that if we have an html node we have an execution context.
// Check if the page is loaded.
let pageReady = false;
try {
/*
When using the devtools protocol trying to call
client.execute() on an unloaded iframe would cause
the code to hang indefinitely since it is using
Puppeteer which freezes on unloaded iframes. Set a
race timeout in order to handle that. Code taken
from our @axe-core/puppeteer utils function.
@see https://github.com/dequelabs/axe-core-npm/issues/727
*/
const timeoutPromise = new Promise(resolve => {
setTimeout(() => {
resolve('timeout');
straker marked this conversation as resolved.
Show resolved Hide resolved
}, FRAME_LOAD_TIMEOUT)
});
const executePromise = client.execute(() => {
return document.readyState === 'complete'
});
const raceResults = await Promise.race([timeoutPromise, executePromise]);
pageReady = raceResults !== 'timeout';
} catch {
/* ignore */
}
if (!pageReady) {
throw new Error('Page/Frame is not ready');
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't part of the point of making these changes that we do not throw an exception?

Copy link
Contributor Author

@straker straker Jun 15, 2023

Choose a reason for hiding this comment

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

We decided that the quickest way to resolve the bug was to return these iframes as frame-tested issues. This timeout / throw behavior is already being handled by the code to return frame-tested results and actually lets the code complete now.

Here's how each driver handled unloaded iframes previously:

  • Puppeteer - hung indefinitely
  • Webdriverjs - waited for the default page/script timeouts (300s and 30s respectively) before responding with an error
  • Webdriverio - had both issues of puppeteer and webdriverjs depending on protocol

}
}

export const axeRunPartial = (
client: Browser,
context?: SerialContextObject,
Expand Down
34 changes: 34 additions & 0 deletions packages/webdriverio/test/axe-webdriverio.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -850,6 +850,40 @@ describe('@axe-core/webdriverio', () => {
normalResults.testEngine.name = legacyResults.testEngine.name;
assert.deepEqual(normalResults, legacyResults);
});

it('handles unloaded iframes (e.g. loading=lazy)', async () => {
await client.url(`${addr}/lazy-loaded-iframe.html`);
const title = await client.getTitle();

const results = await new AxeBuilder({client})
.options({ runOnly: ['label', 'frame-tested'] })
.analyze();

assert.notEqual(title, 'Error');
assert.equal(results.incomplete[0].id, 'frame-tested');
assert.lengthOf(results.incomplete[0].nodes, 1);
assert.deepEqual(results.incomplete[0].nodes[0].target, ['#ifr-lazy', '#lazy-iframe']);
assert.equal(results.violations[0].id, 'label');
assert.lengthOf(results.violations[0].nodes, 1);
assert.deepEqual(results.violations[0].nodes[0].target, [
'#ifr-lazy',
'#lazy-baz',
'input'
]);
});

it('resets pageLoad timeout to user setting', async () => {
await client.url(`${addr}/lazy-loaded-iframe.html`);
client.setTimeout({ pageLoad: 500 })
const title = await client.getTitle();

const results = await new AxeBuilder({client})
.options({ runOnly: ['label', 'frame-tested'] })
.analyze();

const timeout = await client.getTimeouts();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to reset this in an after call?

Copy link
Contributor Author

@straker straker Jun 15, 2023

Choose a reason for hiding this comment

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

I shouldn't need to as the driver is recreated each test.

assert.equal(timeout.pageLoad, 500);
});
});

describe('logOrRethrowError', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/webdriverjs/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions packages/webdriverjs/test/axe-webdriverjs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ describe('@axe-core/webdriverjs', () => {
});

it('skips unloaded iframes (e.g. loading=lazy)', async () => {
await driver.get(`${addr}/lazy-loaded-iframe.html`);
await driver.get(`${addr}/external/lazy-loaded-iframe.html`);
const title = await driver.getTitle();

const results = await new AxeBuilder(driver)
Expand All @@ -408,18 +408,18 @@ describe('@axe-core/webdriverjs', () => {
assert.notEqual(title, 'Error');
assert.equal(results.incomplete[0].id, 'frame-tested');
assert.lengthOf(results.incomplete[0].nodes, 1);
assert.deepEqual(results.incomplete[0].nodes[0].target, ['#parent', '#lazy-iframe']);
assert.deepEqual(results.incomplete[0].nodes[0].target, ['#ifr-lazy', '#lazy-iframe']);
assert.equal(results.violations[0].id, 'label');
assert.lengthOf(results.violations[0].nodes, 1);
assert.deepEqual(results.violations[0].nodes[0].target, [
'#parent',
'#child',
'#ifr-lazy',
'#lazy-baz',
'input'
]);
})

it('resets pageLoad timeout to user setting', async () => {
await driver.get(`${addr}/lazy-loaded-iframe.html`);
await driver.get(`${addr}/external/lazy-loaded-iframe.html`);
driver.manage().setTimeouts({ pageLoad: 500 })
const title = await driver.getTitle();

Expand Down
15 changes: 0 additions & 15 deletions packages/webdriverjs/test/fixtures/iframe-lazy-1.html

This file was deleted.

14 changes: 0 additions & 14 deletions packages/webdriverjs/test/fixtures/lazy-loaded-iframe.html

This file was deleted.