From 4c66d07cd7f98d750c100b72de55a44cf121eac1 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 10 Jan 2023 14:48:06 -0800 Subject: [PATCH 1/5] core(fr): preserve scroll position in gatherers --- core/gather/gatherers/accessibility.js | 11 +++++- core/gather/gatherers/seo/tap-targets.js | 7 ++++ core/legacy/gather/driver.js | 17 ---------- core/legacy/gather/gather-runner.js | 6 ---- core/test/legacy/gather/fake-driver.js | 10 ------ core/test/legacy/gather/gather-runner-test.js | 34 ------------------- 6 files changed, 17 insertions(+), 68 deletions(-) diff --git a/core/gather/gatherers/accessibility.js b/core/gather/gatherers/accessibility.js index 1dfdc62d9a12..1b7dd1508d94 100644 --- a/core/gather/gatherers/accessibility.js +++ b/core/gather/gatherers/accessibility.js @@ -15,6 +15,11 @@ import {pageFunctions} from '../../lib/page-functions.js'; */ /* c8 ignore start */ async function runA11yChecks() { + const originalScrollPosition = { + x: window.scrollX, + y: window.scrollY, + }; + /** @type {import('axe-core/axe')} */ // @ts-expect-error - axe defined by axeLibSource const axe = window.axe; @@ -72,13 +77,17 @@ async function runA11yChecks() { // are relative to the top of the page document.documentElement.scrollTop = 0; - return { + const result = { violations: axeResults.violations.map(createAxeRuleResultArtifact), incomplete: axeResults.incomplete.map(createAxeRuleResultArtifact), notApplicable: axeResults.inapplicable.map(result => ({id: result.id})), // FYI: inapplicable => notApplicable! passes: axeResults.passes.map(result => ({id: result.id})), version: axeResults.testEngine.version, }; + + window.scrollTo(originalScrollPosition.x, originalScrollPosition.y); + + return result; } /** diff --git a/core/gather/gatherers/seo/tap-targets.js b/core/gather/gatherers/seo/tap-targets.js index 3aecd272efc7..eafdd0f10686 100644 --- a/core/gather/gatherers/seo/tap-targets.js +++ b/core/gather/gatherers/seo/tap-targets.js @@ -195,6 +195,11 @@ function gatherTapTargets(tapTargetsSelector, className) { /** @type {LH.Artifacts.TapTarget[]} */ const targets = []; + const originalScrollPosition = { + x: window.scrollX, + y: window.scrollY, + }; + // Capture element positions relative to the top of the page window.scrollTo(0, 0); @@ -279,6 +284,8 @@ function gatherTapTargets(tapTargetsSelector, className) { reenableFixedAndStickyElementPointerEvents(); + window.scrollTo(originalScrollPosition.x, originalScrollPosition.y); + return targets; } /* c8 ignore stop */ diff --git a/core/legacy/gather/driver.js b/core/legacy/gather/driver.js index 24b634825c5e..de42f0044aa2 100644 --- a/core/legacy/gather/driver.js +++ b/core/legacy/gather/driver.js @@ -405,23 +405,6 @@ class Driver { return fetchResponseBodyFromCache(this.defaultSession, requestId, timeout); } - /** - * @param {{x: number, y: number}} position - * @return {Promise} - */ - scrollTo(position) { - const scrollExpression = `window.scrollTo(${position.x}, ${position.y})`; - return this.executionContext.evaluateAsync(scrollExpression, {useIsolation: true}); - } - - /** - * @return {Promise<{x: number, y: number}>} - */ - getScrollPosition() { - return this.executionContext.evaluateAsync(`({x: window.scrollX, y: window.scrollY})`, - {useIsolation: true}); - } - /** * @param {{additionalTraceCategories?: string|null}=} settings * @return {Promise} diff --git a/core/legacy/gather/gather-runner.js b/core/legacy/gather/gather-runner.js index 3f4cf86e99dc..4d0045a684c1 100644 --- a/core/legacy/gather/gather-runner.js +++ b/core/legacy/gather/gather-runner.js @@ -313,17 +313,12 @@ class GatherRunner { * @return {Promise} */ static async afterPass(passContext, loadData, gathererResults) { - const driver = passContext.driver; const config = passContext.passConfig; const gatherers = config.gatherers; const apStatus = {msg: `Running afterPass methods`, id: `lh:gather:afterPass`}; log.time(apStatus, 'verbose'); - // Some gatherers scroll the page which can cause unexpected results for other gatherers. - // We reset the scroll position in between each gatherer. - const scrollPosition = await driver.getScrollPosition(); - for (const gathererDefn of gatherers) { const gatherer = gathererDefn.instance; const status = { @@ -339,7 +334,6 @@ class GatherRunner { gathererResult.push(artifactPromise); gathererResults[gatherer.name] = gathererResult; await artifactPromise.catch(() => {}); - await driver.scrollTo(scrollPosition); log.timeEnd(status); } log.timeEnd(apStatus); diff --git a/core/test/legacy/gather/fake-driver.js b/core/test/legacy/gather/fake-driver.js index 300dd6d50069..ae7dba99efe5 100644 --- a/core/test/legacy/gather/fake-driver.js +++ b/core/test/legacy/gather/fake-driver.js @@ -10,8 +10,6 @@ import {fnAny, readJson} from '../../test-utils.js'; * @param {{protocolGetVersionResponse: LH.CrdpCommands['Browser.getVersion']['returnType']}} param0 */ function makeFakeDriver({protocolGetVersionResponse}) { - let scrollPosition = {x: 0, y: 0}; - return { get fetcher() { return {}; @@ -56,14 +54,6 @@ function makeFakeDriver({protocolGetVersionResponse}) { return Promise.resolve(); }, }, - /** @param {{x: number, y: number}} position */ - scrollTo(position) { - scrollPosition = position; - return Promise.resolve(); - }, - getScrollPosition() { - return Promise.resolve(scrollPosition); - }, beginTrace() { return Promise.resolve(); }, diff --git a/core/test/legacy/gather/gather-runner-test.js b/core/test/legacy/gather/gather-runner-test.js index 0bba26f3117f..77a53995423a 100644 --- a/core/test/legacy/gather/gather-runner-test.js +++ b/core/test/legacy/gather/gather-runner-test.js @@ -687,40 +687,6 @@ describe('GatherRunner', function() { }); }); - it('resets scroll position between every gatherer', async () => { - class ScrollMcScrollyGatherer extends TestGatherer { - /** @param {{driver: Driver}} context */ - afterPass(context) { - context.driver.scrollTo({x: 1000, y: 1000}); - } - } - - const url = 'https://example.com'; - const driver = Object.assign({}, fakeDriver); - const scrollToSpy = jestMock.spyOn(driver, 'scrollTo'); - - const passConfig = { - recordTrace: true, - gatherers: [ - {instance: new ScrollMcScrollyGatherer()}, - {instance: new TestGatherer()}, - ], - }; - - /** @type {any} Using Test-only gatherer. */ - const gathererResults = { - TestGatherer: [], - }; - const passContext = {url, driver, passConfig, computedCache: new Map()}; - await GatherRunner.afterPass(passContext, {}, gathererResults); - // One time for the afterPass of ScrollMcScrolly, two times for the resets of the two gatherers. - expect(scrollToSpy.mock.calls).toEqual([ - [{x: 1000, y: 1000}], - [{x: 0, y: 0}], - [{x: 0, y: 0}], - ]); - }); - it('does as many passes as are required', async () => { const t1 = new TestGatherer(); const t2 = new TestGatherer(); From efef677d59226a062a8125a88208a0058e1ac37b Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 11 Jan 2023 09:47:17 -0800 Subject: [PATCH 2/5] try finally --- core/gather/gatherers/accessibility.js | 132 +++++++++--------- core/gather/gatherers/seo/tap-targets.js | 166 ++++++++++++----------- 2 files changed, 150 insertions(+), 148 deletions(-) diff --git a/core/gather/gatherers/accessibility.js b/core/gather/gatherers/accessibility.js index 1b7dd1508d94..2d731963d306 100644 --- a/core/gather/gatherers/accessibility.js +++ b/core/gather/gatherers/accessibility.js @@ -20,74 +20,74 @@ async function runA11yChecks() { y: window.scrollY, }; - /** @type {import('axe-core/axe')} */ - // @ts-expect-error - axe defined by axeLibSource - const axe = window.axe; - const application = `lighthouse-${Math.random()}`; - axe.configure({ - branding: { - application, - }, - noHtml: true, - }); - const axeResults = await axe.run(document, { - elementRef: true, - runOnly: { - type: 'tag', - values: [ - 'wcag2a', - 'wcag2aa', - ], - }, - // resultTypes doesn't limit the output of the axeResults object. Instead, if it's defined, - // some expensive element identification is done only for the respective types. https://github.com/dequelabs/axe-core/blob/f62f0cf18f7b69b247b0b6362cf1ae71ffbf3a1b/lib/core/reporters/helpers/process-aggregate.js#L61-L97 - resultTypes: ['violations', 'inapplicable'], - rules: { - // Consider http://go/prcpg for expert review of the aXe rules. - 'tabindex': {enabled: true}, - 'accesskeys': {enabled: true}, - 'heading-order': {enabled: true}, - 'meta-viewport': {enabled: true}, - 'duplicate-id': {enabled: false}, - 'table-fake-caption': {enabled: false}, - 'td-has-header': {enabled: false}, - 'marquee': {enabled: false}, - 'area-alt': {enabled: false}, - 'html-xml-lang-mismatch': {enabled: false}, - 'blink': {enabled: false}, - 'server-side-image-map': {enabled: false}, - 'identical-links-same-purpose': {enabled: false}, - 'no-autoplay-audio': {enabled: false}, - 'svg-img-alt': {enabled: false}, - 'audio-caption': {enabled: false}, - 'aria-treeitem-name': {enabled: true}, - // https://github.com/dequelabs/axe-core/issues/2958 - 'nested-interactive': {enabled: false}, - 'frame-focusable-content': {enabled: false}, - 'aria-roledescription': {enabled: false}, - 'scrollable-region-focusable': {enabled: false}, - // TODO(paulirish): create audits and enable these 3. - 'input-button-name': {enabled: false}, - 'role-img-alt': {enabled: false}, - 'select-name': {enabled: false}, - }, - }); - - // axe just scrolled the page, scroll back to the top of the page so that element positions - // are relative to the top of the page - document.documentElement.scrollTop = 0; - - const result = { - violations: axeResults.violations.map(createAxeRuleResultArtifact), - incomplete: axeResults.incomplete.map(createAxeRuleResultArtifact), - notApplicable: axeResults.inapplicable.map(result => ({id: result.id})), // FYI: inapplicable => notApplicable! - passes: axeResults.passes.map(result => ({id: result.id})), - version: axeResults.testEngine.version, - }; + try { + /** @type {import('axe-core/axe')} */ + // @ts-expect-error - axe defined by axeLibSource + const axe = window.axe; + const application = `lighthouse-${Math.random()}`; + axe.configure({ + branding: { + application, + }, + noHtml: true, + }); + const axeResults = await axe.run(document, { + elementRef: true, + runOnly: { + type: 'tag', + values: [ + 'wcag2a', + 'wcag2aa', + ], + }, + // resultTypes doesn't limit the output of the axeResults object. Instead, if it's defined, + // some expensive element identification is done only for the respective types. https://github.com/dequelabs/axe-core/blob/f62f0cf18f7b69b247b0b6362cf1ae71ffbf3a1b/lib/core/reporters/helpers/process-aggregate.js#L61-L97 + resultTypes: ['violations', 'inapplicable'], + rules: { + // Consider http://go/prcpg for expert review of the aXe rules. + 'tabindex': {enabled: true}, + 'accesskeys': {enabled: true}, + 'heading-order': {enabled: true}, + 'meta-viewport': {enabled: true}, + 'duplicate-id': {enabled: false}, + 'table-fake-caption': {enabled: false}, + 'td-has-header': {enabled: false}, + 'marquee': {enabled: false}, + 'area-alt': {enabled: false}, + 'html-xml-lang-mismatch': {enabled: false}, + 'blink': {enabled: false}, + 'server-side-image-map': {enabled: false}, + 'identical-links-same-purpose': {enabled: false}, + 'no-autoplay-audio': {enabled: false}, + 'svg-img-alt': {enabled: false}, + 'audio-caption': {enabled: false}, + 'aria-treeitem-name': {enabled: true}, + // https://github.com/dequelabs/axe-core/issues/2958 + 'nested-interactive': {enabled: false}, + 'frame-focusable-content': {enabled: false}, + 'aria-roledescription': {enabled: false}, + 'scrollable-region-focusable': {enabled: false}, + // TODO(paulirish): create audits and enable these 3. + 'input-button-name': {enabled: false}, + 'role-img-alt': {enabled: false}, + 'select-name': {enabled: false}, + }, + }); - window.scrollTo(originalScrollPosition.x, originalScrollPosition.y); + // axe just scrolled the page, scroll back to the top of the page so that element positions + // are relative to the top of the page + document.documentElement.scrollTop = 0; - return result; + return { + violations: axeResults.violations.map(createAxeRuleResultArtifact), + incomplete: axeResults.incomplete.map(createAxeRuleResultArtifact), + notApplicable: axeResults.inapplicable.map(result => ({id: result.id})), // FYI: inapplicable => notApplicable! + passes: axeResults.passes.map(result => ({id: result.id})), + version: axeResults.testEngine.version, + }; + } finally { + window.scrollTo(originalScrollPosition.x, originalScrollPosition.y); + } } /** diff --git a/core/gather/gatherers/seo/tap-targets.js b/core/gather/gatherers/seo/tap-targets.js index eafdd0f10686..671550c9640d 100644 --- a/core/gather/gatherers/seo/tap-targets.js +++ b/core/gather/gatherers/seo/tap-targets.js @@ -192,101 +192,103 @@ function disableFixedAndStickyElementPointerEvents(className) { */ /* c8 ignore start */ function gatherTapTargets(tapTargetsSelector, className) { - /** @type {LH.Artifacts.TapTarget[]} */ - const targets = []; - const originalScrollPosition = { x: window.scrollX, y: window.scrollY, }; - // Capture element positions relative to the top of the page - window.scrollTo(0, 0); - - /** @type {HTMLElement[]} */ - // @ts-expect-error - getElementsInDocument put into scope via stringification - const tapTargetElements = getElementsInDocument(tapTargetsSelector); - - /** @type {{ - tapTargetElement: Element, - clientRects: LH.Artifacts.Rect[] - }[]} */ - const tapTargetsWithClientRects = []; - tapTargetElements.forEach(tapTargetElement => { - // Filter out tap targets that are likely to cause false failures: - if (elementHasAncestorTapTarget(tapTargetElement, tapTargetsSelector)) { - // This is usually intentional, either the tap targets trigger the same action - // or there's a child with a related action (like a delete button for an item) - return; - } - if (elementIsInTextBlock(tapTargetElement)) { - // Links inside text blocks cause a lot of failures, and there's also an exception for them - // in the Web Content Accessibility Guidelines https://www.w3.org/TR/WCAG21/#target-size - return; - } - if (!elementIsVisible(tapTargetElement)) { - return; - } - - tapTargetsWithClientRects.push({ - tapTargetElement, - clientRects: getClientRects(tapTargetElement), - }); - }); - - // Disable pointer events so that tap targets below them don't get - // detected as non-tappable (they are tappable, just not while the viewport - // is at the current scroll position) - const reenableFixedAndStickyElementPointerEvents = - disableFixedAndStickyElementPointerEvents(className); - - /** @type {{ - tapTargetElement: Element, - visibleClientRects: LH.Artifacts.Rect[] - }[]} */ - const tapTargetsWithVisibleClientRects = []; - // We use separate loop here to get visible client rects because that involves - // scrolling around the page for elementCenterIsAtZAxisTop, which would affect the - // client rect positions. - tapTargetsWithClientRects.forEach(({tapTargetElement, clientRects}) => { - // Filter out empty client rects - let visibleClientRects = clientRects.filter(cr => cr.width !== 0 && cr.height !== 0); - - // Filter out client rects that are invisible, e.g because they are in a position absolute element - // with a lower z-index than the main content. - // This will also filter out all position fixed or sticky tap targets elements because we disable pointer - // events on them before running this. That's the correct behavior because whether a position fixed/stick - // element overlaps with another tap target depends on the scroll position. - visibleClientRects = visibleClientRects.filter(rect => { - // Just checking the center can cause false failures for large partially hidden tap targets, - // but that should be a rare edge case - // @ts-expect-error - put into scope via stringification - const rectCenterPoint = getRectCenterPoint(rect); - return elementCenterIsAtZAxisTop(tapTargetElement, rectCenterPoint); + try { + /** @type {LH.Artifacts.TapTarget[]} */ + const targets = []; + + // Capture element positions relative to the top of the page + window.scrollTo(0, 0); + + /** @type {HTMLElement[]} */ + // @ts-expect-error - getElementsInDocument put into scope via stringification + const tapTargetElements = getElementsInDocument(tapTargetsSelector); + + /** @type {{ + tapTargetElement: Element, + clientRects: LH.Artifacts.Rect[] + }[]} */ + const tapTargetsWithClientRects = []; + tapTargetElements.forEach(tapTargetElement => { + // Filter out tap targets that are likely to cause false failures: + if (elementHasAncestorTapTarget(tapTargetElement, tapTargetsSelector)) { + // This is usually intentional, either the tap targets trigger the same action + // or there's a child with a related action (like a delete button for an item) + return; + } + if (elementIsInTextBlock(tapTargetElement)) { + // Links inside text blocks cause a lot of failures, and there's also an exception for them + // in the Web Content Accessibility Guidelines https://www.w3.org/TR/WCAG21/#target-size + return; + } + if (!elementIsVisible(tapTargetElement)) { + return; + } + + tapTargetsWithClientRects.push({ + tapTargetElement, + clientRects: getClientRects(tapTargetElement), + }); }); - if (visibleClientRects.length > 0) { - tapTargetsWithVisibleClientRects.push({ - tapTargetElement, - visibleClientRects, + // Disable pointer events so that tap targets below them don't get + // detected as non-tappable (they are tappable, just not while the viewport + // is at the current scroll position) + const reenableFixedAndStickyElementPointerEvents = + disableFixedAndStickyElementPointerEvents(className); + + /** @type {{ + tapTargetElement: Element, + visibleClientRects: LH.Artifacts.Rect[] + }[]} */ + const tapTargetsWithVisibleClientRects = []; + // We use separate loop here to get visible client rects because that involves + // scrolling around the page for elementCenterIsAtZAxisTop, which would affect the + // client rect positions. + tapTargetsWithClientRects.forEach(({tapTargetElement, clientRects}) => { + // Filter out empty client rects + let visibleClientRects = clientRects.filter(cr => cr.width !== 0 && cr.height !== 0); + + // Filter out client rects that are invisible, e.g because they are in a position absolute element + // with a lower z-index than the main content. + // This will also filter out all position fixed or sticky tap targets elements because we disable pointer + // events on them before running this. That's the correct behavior because whether a position fixed/stick + // element overlaps with another tap target depends on the scroll position. + visibleClientRects = visibleClientRects.filter(rect => { + // Just checking the center can cause false failures for large partially hidden tap targets, + // but that should be a rare edge case + // @ts-expect-error - put into scope via stringification + const rectCenterPoint = getRectCenterPoint(rect); + return elementCenterIsAtZAxisTop(tapTargetElement, rectCenterPoint); }); - } - }); - for (const {tapTargetElement, visibleClientRects} of tapTargetsWithVisibleClientRects) { - targets.push({ - clientRects: visibleClientRects, - href: /** @type {HTMLAnchorElement} */(tapTargetElement)['href'] || '', - // @ts-expect-error - getNodeDetails put into scope via stringification - node: getNodeDetails(tapTargetElement), + if (visibleClientRects.length > 0) { + tapTargetsWithVisibleClientRects.push({ + tapTargetElement, + visibleClientRects, + }); + } }); - } - reenableFixedAndStickyElementPointerEvents(); + for (const {tapTargetElement, visibleClientRects} of tapTargetsWithVisibleClientRects) { + targets.push({ + clientRects: visibleClientRects, + href: /** @type {HTMLAnchorElement} */(tapTargetElement)['href'] || '', + // @ts-expect-error - getNodeDetails put into scope via stringification + node: getNodeDetails(tapTargetElement), + }); + } - window.scrollTo(originalScrollPosition.x, originalScrollPosition.y); + reenableFixedAndStickyElementPointerEvents(); - return targets; + return targets; + } finally { + window.scrollTo(originalScrollPosition.x, originalScrollPosition.y); + } } /* c8 ignore stop */ From 3d3f3bc3f70c93bd82186027b6ad588a182b78e0 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 11 Jan 2023 13:52:19 -0800 Subject: [PATCH 3/5] reorg --- core/gather/gatherers/accessibility.js | 135 +++++++-------- core/gather/gatherers/seo/tap-targets.js | 199 ++++++++++++----------- 2 files changed, 175 insertions(+), 159 deletions(-) diff --git a/core/gather/gatherers/accessibility.js b/core/gather/gatherers/accessibility.js index 2d731963d306..44d54024e17d 100644 --- a/core/gather/gatherers/accessibility.js +++ b/core/gather/gatherers/accessibility.js @@ -15,76 +15,80 @@ import {pageFunctions} from '../../lib/page-functions.js'; */ /* c8 ignore start */ async function runA11yChecks() { + /** @type {import('axe-core/axe')} */ + // @ts-expect-error - axe defined by axeLibSource + const axe = window.axe; + const application = `lighthouse-${Math.random()}`; + axe.configure({ + branding: { + application, + }, + noHtml: true, + }); + const axeResults = await axe.run(document, { + elementRef: true, + runOnly: { + type: 'tag', + values: [ + 'wcag2a', + 'wcag2aa', + ], + }, + // resultTypes doesn't limit the output of the axeResults object. Instead, if it's defined, + // some expensive element identification is done only for the respective types. https://github.com/dequelabs/axe-core/blob/f62f0cf18f7b69b247b0b6362cf1ae71ffbf3a1b/lib/core/reporters/helpers/process-aggregate.js#L61-L97 + resultTypes: ['violations', 'inapplicable'], + rules: { + // Consider http://go/prcpg for expert review of the aXe rules. + 'tabindex': {enabled: true}, + 'accesskeys': {enabled: true}, + 'heading-order': {enabled: true}, + 'meta-viewport': {enabled: true}, + 'duplicate-id': {enabled: false}, + 'table-fake-caption': {enabled: false}, + 'td-has-header': {enabled: false}, + 'marquee': {enabled: false}, + 'area-alt': {enabled: false}, + 'html-xml-lang-mismatch': {enabled: false}, + 'blink': {enabled: false}, + 'server-side-image-map': {enabled: false}, + 'identical-links-same-purpose': {enabled: false}, + 'no-autoplay-audio': {enabled: false}, + 'svg-img-alt': {enabled: false}, + 'audio-caption': {enabled: false}, + 'aria-treeitem-name': {enabled: true}, + // https://github.com/dequelabs/axe-core/issues/2958 + 'nested-interactive': {enabled: false}, + 'frame-focusable-content': {enabled: false}, + 'aria-roledescription': {enabled: false}, + 'scrollable-region-focusable': {enabled: false}, + // TODO(paulirish): create audits and enable these 3. + 'input-button-name': {enabled: false}, + 'role-img-alt': {enabled: false}, + 'select-name': {enabled: false}, + }, + }); + + // axe just scrolled the page, scroll back to the top of the page so that element positions + // are relative to the top of the page + document.documentElement.scrollTop = 0; + + return { + violations: axeResults.violations.map(createAxeRuleResultArtifact), + incomplete: axeResults.incomplete.map(createAxeRuleResultArtifact), + notApplicable: axeResults.inapplicable.map(result => ({id: result.id})), // FYI: inapplicable => notApplicable! + passes: axeResults.passes.map(result => ({id: result.id})), + version: axeResults.testEngine.version, + }; +} + +function runA11yChecksAndResetScroll() { const originalScrollPosition = { x: window.scrollX, y: window.scrollY, }; try { - /** @type {import('axe-core/axe')} */ - // @ts-expect-error - axe defined by axeLibSource - const axe = window.axe; - const application = `lighthouse-${Math.random()}`; - axe.configure({ - branding: { - application, - }, - noHtml: true, - }); - const axeResults = await axe.run(document, { - elementRef: true, - runOnly: { - type: 'tag', - values: [ - 'wcag2a', - 'wcag2aa', - ], - }, - // resultTypes doesn't limit the output of the axeResults object. Instead, if it's defined, - // some expensive element identification is done only for the respective types. https://github.com/dequelabs/axe-core/blob/f62f0cf18f7b69b247b0b6362cf1ae71ffbf3a1b/lib/core/reporters/helpers/process-aggregate.js#L61-L97 - resultTypes: ['violations', 'inapplicable'], - rules: { - // Consider http://go/prcpg for expert review of the aXe rules. - 'tabindex': {enabled: true}, - 'accesskeys': {enabled: true}, - 'heading-order': {enabled: true}, - 'meta-viewport': {enabled: true}, - 'duplicate-id': {enabled: false}, - 'table-fake-caption': {enabled: false}, - 'td-has-header': {enabled: false}, - 'marquee': {enabled: false}, - 'area-alt': {enabled: false}, - 'html-xml-lang-mismatch': {enabled: false}, - 'blink': {enabled: false}, - 'server-side-image-map': {enabled: false}, - 'identical-links-same-purpose': {enabled: false}, - 'no-autoplay-audio': {enabled: false}, - 'svg-img-alt': {enabled: false}, - 'audio-caption': {enabled: false}, - 'aria-treeitem-name': {enabled: true}, - // https://github.com/dequelabs/axe-core/issues/2958 - 'nested-interactive': {enabled: false}, - 'frame-focusable-content': {enabled: false}, - 'aria-roledescription': {enabled: false}, - 'scrollable-region-focusable': {enabled: false}, - // TODO(paulirish): create audits and enable these 3. - 'input-button-name': {enabled: false}, - 'role-img-alt': {enabled: false}, - 'select-name': {enabled: false}, - }, - }); - - // axe just scrolled the page, scroll back to the top of the page so that element positions - // are relative to the top of the page - document.documentElement.scrollTop = 0; - - return { - violations: axeResults.violations.map(createAxeRuleResultArtifact), - incomplete: axeResults.incomplete.map(createAxeRuleResultArtifact), - notApplicable: axeResults.inapplicable.map(result => ({id: result.id})), // FYI: inapplicable => notApplicable! - passes: axeResults.passes.map(result => ({id: result.id})), - version: axeResults.testEngine.version, - }; + return runA11yChecks(); } finally { window.scrollTo(originalScrollPosition.x, originalScrollPosition.y); } @@ -177,13 +181,14 @@ class Accessibility extends FRGatherer { getArtifact(passContext) { const driver = passContext.driver; - return driver.executionContext.evaluate(runA11yChecks, { + return driver.executionContext.evaluate(runA11yChecksAndResetScroll, { args: [], useIsolation: true, deps: [ axeSource, pageFunctions.getNodeDetails, createAxeRuleResultArtifact, + runA11yChecks, ], }); } diff --git a/core/gather/gatherers/seo/tap-targets.js b/core/gather/gatherers/seo/tap-targets.js index 671550c9640d..3474408c0da2 100644 --- a/core/gather/gatherers/seo/tap-targets.js +++ b/core/gather/gatherers/seo/tap-targets.js @@ -192,100 +192,109 @@ function disableFixedAndStickyElementPointerEvents(className) { */ /* c8 ignore start */ function gatherTapTargets(tapTargetsSelector, className) { - const originalScrollPosition = { - x: window.scrollX, - y: window.scrollY, - }; + /** @type {LH.Artifacts.TapTarget[]} */ + const targets = []; - try { - /** @type {LH.Artifacts.TapTarget[]} */ - const targets = []; + // Capture element positions relative to the top of the page + window.scrollTo(0, 0); - // Capture element positions relative to the top of the page - window.scrollTo(0, 0); + /** @type {HTMLElement[]} */ + // @ts-expect-error - getElementsInDocument put into scope via stringification + const tapTargetElements = getElementsInDocument(tapTargetsSelector); - /** @type {HTMLElement[]} */ - // @ts-expect-error - getElementsInDocument put into scope via stringification - const tapTargetElements = getElementsInDocument(tapTargetsSelector); - - /** @type {{ + /** @type {{ tapTargetElement: Element, clientRects: LH.Artifacts.Rect[] }[]} */ - const tapTargetsWithClientRects = []; - tapTargetElements.forEach(tapTargetElement => { - // Filter out tap targets that are likely to cause false failures: - if (elementHasAncestorTapTarget(tapTargetElement, tapTargetsSelector)) { - // This is usually intentional, either the tap targets trigger the same action - // or there's a child with a related action (like a delete button for an item) - return; - } - if (elementIsInTextBlock(tapTargetElement)) { - // Links inside text blocks cause a lot of failures, and there's also an exception for them - // in the Web Content Accessibility Guidelines https://www.w3.org/TR/WCAG21/#target-size - return; - } - if (!elementIsVisible(tapTargetElement)) { - return; - } - - tapTargetsWithClientRects.push({ - tapTargetElement, - clientRects: getClientRects(tapTargetElement), - }); + const tapTargetsWithClientRects = []; + tapTargetElements.forEach(tapTargetElement => { + // Filter out tap targets that are likely to cause false failures: + if (elementHasAncestorTapTarget(tapTargetElement, tapTargetsSelector)) { + // This is usually intentional, either the tap targets trigger the same action + // or there's a child with a related action (like a delete button for an item) + return; + } + if (elementIsInTextBlock(tapTargetElement)) { + // Links inside text blocks cause a lot of failures, and there's also an exception for them + // in the Web Content Accessibility Guidelines https://www.w3.org/TR/WCAG21/#target-size + return; + } + if (!elementIsVisible(tapTargetElement)) { + return; + } + + tapTargetsWithClientRects.push({ + tapTargetElement, + clientRects: getClientRects(tapTargetElement), }); + }); - // Disable pointer events so that tap targets below them don't get - // detected as non-tappable (they are tappable, just not while the viewport - // is at the current scroll position) - const reenableFixedAndStickyElementPointerEvents = + // Disable pointer events so that tap targets below them don't get + // detected as non-tappable (they are tappable, just not while the viewport + // is at the current scroll position) + const reenableFixedAndStickyElementPointerEvents = disableFixedAndStickyElementPointerEvents(className); - /** @type {{ + /** @type {{ tapTargetElement: Element, visibleClientRects: LH.Artifacts.Rect[] }[]} */ - const tapTargetsWithVisibleClientRects = []; - // We use separate loop here to get visible client rects because that involves - // scrolling around the page for elementCenterIsAtZAxisTop, which would affect the - // client rect positions. - tapTargetsWithClientRects.forEach(({tapTargetElement, clientRects}) => { - // Filter out empty client rects - let visibleClientRects = clientRects.filter(cr => cr.width !== 0 && cr.height !== 0); - - // Filter out client rects that are invisible, e.g because they are in a position absolute element - // with a lower z-index than the main content. - // This will also filter out all position fixed or sticky tap targets elements because we disable pointer - // events on them before running this. That's the correct behavior because whether a position fixed/stick - // element overlaps with another tap target depends on the scroll position. - visibleClientRects = visibleClientRects.filter(rect => { - // Just checking the center can cause false failures for large partially hidden tap targets, - // but that should be a rare edge case - // @ts-expect-error - put into scope via stringification - const rectCenterPoint = getRectCenterPoint(rect); - return elementCenterIsAtZAxisTop(tapTargetElement, rectCenterPoint); - }); - - if (visibleClientRects.length > 0) { - tapTargetsWithVisibleClientRects.push({ - tapTargetElement, - visibleClientRects, - }); - } + const tapTargetsWithVisibleClientRects = []; + // We use separate loop here to get visible client rects because that involves + // scrolling around the page for elementCenterIsAtZAxisTop, which would affect the + // client rect positions. + tapTargetsWithClientRects.forEach(({tapTargetElement, clientRects}) => { + // Filter out empty client rects + let visibleClientRects = clientRects.filter(cr => cr.width !== 0 && cr.height !== 0); + + // Filter out client rects that are invisible, e.g because they are in a position absolute element + // with a lower z-index than the main content. + // This will also filter out all position fixed or sticky tap targets elements because we disable pointer + // events on them before running this. That's the correct behavior because whether a position fixed/stick + // element overlaps with another tap target depends on the scroll position. + visibleClientRects = visibleClientRects.filter(rect => { + // Just checking the center can cause false failures for large partially hidden tap targets, + // but that should be a rare edge case + // @ts-expect-error - put into scope via stringification + const rectCenterPoint = getRectCenterPoint(rect); + return elementCenterIsAtZAxisTop(tapTargetElement, rectCenterPoint); }); - for (const {tapTargetElement, visibleClientRects} of tapTargetsWithVisibleClientRects) { - targets.push({ - clientRects: visibleClientRects, - href: /** @type {HTMLAnchorElement} */(tapTargetElement)['href'] || '', - // @ts-expect-error - getNodeDetails put into scope via stringification - node: getNodeDetails(tapTargetElement), + if (visibleClientRects.length > 0) { + tapTargetsWithVisibleClientRects.push({ + tapTargetElement, + visibleClientRects, }); } + }); - reenableFixedAndStickyElementPointerEvents(); + for (const {tapTargetElement, visibleClientRects} of tapTargetsWithVisibleClientRects) { + targets.push({ + clientRects: visibleClientRects, + href: /** @type {HTMLAnchorElement} */(tapTargetElement)['href'] || '', + // @ts-expect-error - getNodeDetails put into scope via stringification + node: getNodeDetails(tapTargetElement), + }); + } + + reenableFixedAndStickyElementPointerEvents(); - return targets; + return targets; +} + +/** + * @param {string} tapTargetsSelector + * @param {string} className + * @return {LH.Artifacts.TapTarget[]} + */ +function gatherTapTargetsAndResetScroll(tapTargetsSelector, className) { + const originalScrollPosition = { + x: window.scrollX, + y: window.scrollY, + }; + + try { + return gatherTapTargets(tapTargetsSelector, className); } finally { window.scrollTo(originalScrollPosition.x, originalScrollPosition.y); } @@ -346,25 +355,27 @@ class TapTargets extends FRGatherer { const className = 'lighthouse-disable-pointer-events'; const styleSheetId = await this.addStyleRule(session, className); - const tapTargets = await passContext.driver.executionContext.evaluate(gatherTapTargets, { - args: [tapTargetsSelector, className], - useIsolation: true, - deps: [ - pageFunctions.getNodeDetails, - pageFunctions.getElementsInDocument, - disableFixedAndStickyElementPointerEvents, - elementIsVisible, - elementHasAncestorTapTarget, - elementCenterIsAtZAxisTop, - getClientRects, - hasTextNodeSiblingsFormingTextBlock, - elementIsInTextBlock, - RectHelpers.getRectCenterPoint, - pageFunctions.getNodePath, - pageFunctions.getNodeSelector, - pageFunctions.getNodeLabel, - ], - }); + const tapTargets = + await passContext.driver.executionContext.evaluate(gatherTapTargetsAndResetScroll, { + args: [tapTargetsSelector, className], + useIsolation: true, + deps: [ + pageFunctions.getNodeDetails, + pageFunctions.getElementsInDocument, + disableFixedAndStickyElementPointerEvents, + elementIsVisible, + elementHasAncestorTapTarget, + elementCenterIsAtZAxisTop, + getClientRects, + hasTextNodeSiblingsFormingTextBlock, + elementIsInTextBlock, + RectHelpers.getRectCenterPoint, + pageFunctions.getNodePath, + pageFunctions.getNodeSelector, + pageFunctions.getNodeLabel, + gatherTapTargets, + ], + }); await this.removeStyleRule(session, styleSheetId); From 4e40f6199066adeebe63041ade8a695ded07b8b1 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 11 Jan 2023 13:54:32 -0800 Subject: [PATCH 4/5] ope --- core/gather/gatherers/seo/tap-targets.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/core/gather/gatherers/seo/tap-targets.js b/core/gather/gatherers/seo/tap-targets.js index 3474408c0da2..093883343e63 100644 --- a/core/gather/gatherers/seo/tap-targets.js +++ b/core/gather/gatherers/seo/tap-targets.js @@ -203,9 +203,9 @@ function gatherTapTargets(tapTargetsSelector, className) { const tapTargetElements = getElementsInDocument(tapTargetsSelector); /** @type {{ - tapTargetElement: Element, - clientRects: LH.Artifacts.Rect[] - }[]} */ + tapTargetElement: Element, + clientRects: LH.Artifacts.Rect[] + }[]} */ const tapTargetsWithClientRects = []; tapTargetElements.forEach(tapTargetElement => { // Filter out tap targets that are likely to cause false failures: @@ -233,12 +233,12 @@ function gatherTapTargets(tapTargetsSelector, className) { // detected as non-tappable (they are tappable, just not while the viewport // is at the current scroll position) const reenableFixedAndStickyElementPointerEvents = - disableFixedAndStickyElementPointerEvents(className); + disableFixedAndStickyElementPointerEvents(className); /** @type {{ - tapTargetElement: Element, - visibleClientRects: LH.Artifacts.Rect[] - }[]} */ + tapTargetElement: Element, + visibleClientRects: LH.Artifacts.Rect[] + }[]} */ const tapTargetsWithVisibleClientRects = []; // We use separate loop here to get visible client rects because that involves // scrolling around the page for elementCenterIsAtZAxisTop, which would affect the From fb9ae5252253d7732a6b3e0b6a3a40dafa81247e Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 12 Jan 2023 17:49:02 -0800 Subject: [PATCH 5/5] await --- core/gather/gatherers/accessibility.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/gather/gatherers/accessibility.js b/core/gather/gatherers/accessibility.js index 44d54024e17d..3b6d6d43621d 100644 --- a/core/gather/gatherers/accessibility.js +++ b/core/gather/gatherers/accessibility.js @@ -81,14 +81,14 @@ async function runA11yChecks() { }; } -function runA11yChecksAndResetScroll() { +async function runA11yChecksAndResetScroll() { const originalScrollPosition = { x: window.scrollX, y: window.scrollY, }; try { - return runA11yChecks(); + return await runA11yChecks(); } finally { window.scrollTo(originalScrollPosition.x, originalScrollPosition.y); }