From fe5c9dad4d4d5d9ed7ec3c96a2141018348e2336 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 19 Jun 2023 15:22:26 -0700 Subject: [PATCH] fix(locators): allow identical frameLocators inside and/or/has (#23740) So, the following will work: ``` page.frameLocator('iframe').locator('span').or(page.frameLoactor('iframe').locator('div')) ``` The following will not work, because frame locators are not exactly the same: ``` page.frameLocator('#iframe1').locator('span').or(page.frameLoactor('#iframe2').locator('div')) ``` Also improve the error message to be more readable and include the locator. Fixes #23697. --- .../src/server/frameSelectors.ts | 12 ++++- .../src/server/injected/injectedScript.ts | 10 ++--- .../playwright-core/src/server/selectors.ts | 7 +-- .../src/utils/isomorphic/selectorParser.ts | 44 +++++++++++-------- tests/page/locator-query.spec.ts | 11 +++++ 5 files changed, 57 insertions(+), 27 deletions(-) diff --git a/packages/playwright-core/src/server/frameSelectors.ts b/packages/playwright-core/src/server/frameSelectors.ts index 072460bbbe..7a11f8564c 100644 --- a/packages/playwright-core/src/server/frameSelectors.ts +++ b/packages/playwright-core/src/server/frameSelectors.ts @@ -16,10 +16,11 @@ import type { Frame } from './frames'; import type * as types from './types'; -import { stringifySelector, type ParsedSelector, splitSelectorByFrame } from '../utils/isomorphic/selectorParser'; +import { stringifySelector, type ParsedSelector, splitSelectorByFrame, InvalidSelectorError, visitAllSelectorParts } from '../utils/isomorphic/selectorParser'; import type { FrameExecutionContext, ElementHandle } from './dom'; import type { JSHandle } from './javascript'; import type { InjectedScript } from './injected/injectedScript'; +import { asLocator } from '../utils/isomorphic/locatorGenerators'; export type SelectorInfo = { parsed: ParsedSelector, @@ -111,6 +112,15 @@ export class FrameSelectors { let frame: Frame = this.frame; const frameChunks = splitSelectorByFrame(selector); + for (const chunk of frameChunks) { + visitAllSelectorParts(chunk, (part, nested) => { + if (nested && part.name === 'internal:control' && part.body === 'enter-frame') { + const locator = asLocator(this.frame._page.attribution.playwright.options.sdkLanguage, selector); + throw new InvalidSelectorError(`Frame locators are not allowed inside composite locators, while querying "${locator}"`); + } + }); + } + for (let i = 0; i < frameChunks.length - 1; ++i) { const info = this._parseSelector(frameChunks[i], options); const context = await frame._context(info.world); diff --git a/packages/playwright-core/src/server/injected/injectedScript.ts b/packages/playwright-core/src/server/injected/injectedScript.ts index 41a4846c42..752bab77db 100644 --- a/packages/playwright-core/src/server/injected/injectedScript.ts +++ b/packages/playwright-core/src/server/injected/injectedScript.ts @@ -21,7 +21,7 @@ import { VueEngine } from './vueSelectorEngine'; import { createRoleEngine } from './roleSelectorEngine'; import { parseAttributeSelector } from '../../utils/isomorphic/selectorParser'; import type { NestedSelectorBody, ParsedSelector, ParsedSelectorPart } from '../../utils/isomorphic/selectorParser'; -import { allEngineNames, parseSelector, stringifySelector } from '../../utils/isomorphic/selectorParser'; +import { visitAllSelectorParts, parseSelector, stringifySelector } from '../../utils/isomorphic/selectorParser'; import { type TextMatcher, elementMatchesText, elementText, type ElementText } from './selectorUtils'; import { SelectorEvaluatorImpl, sortInDOMOrder } from './selectorEvaluator'; import { enclosingShadowRootOrDocument, isElementVisible, parentElementOrShadowHost } from './domUtils'; @@ -146,10 +146,10 @@ export class InjectedScript { parseSelector(selector: string): ParsedSelector { const result = parseSelector(selector); - for (const name of allEngineNames(result)) { - if (!this._engines.has(name)) - throw this.createStacklessError(`Unknown engine "${name}" while parsing selector ${selector}`); - } + visitAllSelectorParts(result, part => { + if (!this._engines.has(part.name)) + throw this.createStacklessError(`Unknown engine "${part.name}" while parsing selector ${selector}`); + }); return result; } diff --git a/packages/playwright-core/src/server/selectors.ts b/packages/playwright-core/src/server/selectors.ts index 85ecd5e8c2..f0dc4dbec3 100644 --- a/packages/playwright-core/src/server/selectors.ts +++ b/packages/playwright-core/src/server/selectors.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { allEngineNames, InvalidSelectorError, type ParsedSelector, parseSelector, stringifySelector } from '../utils/isomorphic/selectorParser'; +import { visitAllSelectorParts, InvalidSelectorError, type ParsedSelector, parseSelector, stringifySelector } from '../utils/isomorphic/selectorParser'; import { createGuid } from '../utils'; export class Selectors { @@ -73,7 +73,8 @@ export class Selectors { parseSelector(selector: string | ParsedSelector, strict: boolean) { const parsed = typeof selector === 'string' ? parseSelector(selector) : selector; let needsMainWorld = false; - for (const name of allEngineNames(parsed)) { + visitAllSelectorParts(parsed, part => { + const name = part.name; const custom = this._engines.get(name); if (!custom && !this._builtinEngines.has(name)) throw new InvalidSelectorError(`Unknown engine "${name}" while parsing selector ${stringifySelector(parsed)}`); @@ -81,7 +82,7 @@ export class Selectors { needsMainWorld = true; if (this._builtinEnginesInMainWorld.has(name)) needsMainWorld = true; - } + }); return { parsed, world: needsMainWorld ? 'main' as const : 'utility' as const, diff --git a/packages/playwright-core/src/utils/isomorphic/selectorParser.ts b/packages/playwright-core/src/utils/isomorphic/selectorParser.ts index 1708672cec..1f713a732d 100644 --- a/packages/playwright-core/src/utils/isomorphic/selectorParser.ts +++ b/packages/playwright-core/src/utils/isomorphic/selectorParser.ts @@ -41,17 +41,19 @@ type ParsedSelectorStrings = { export const customCSSNames = new Set(['not', 'is', 'where', 'has', 'scope', 'light', 'visible', 'text', 'text-matches', 'text-is', 'has-text', 'above', 'below', 'right-of', 'left-of', 'near', 'nth-match']); export function parseSelector(selector: string): ParsedSelector { - const result = parseSelectorString(selector); - const parts: ParsedSelectorPart[] = result.parts.map(part => { + const parsedStrings = parseSelectorString(selector); + const parts: ParsedSelectorPart[] = []; + for (const part of parsedStrings.parts) { if (part.name === 'css' || part.name === 'css:light') { if (part.name === 'css:light') part.body = ':light(' + part.body + ')'; const parsedCSS = parseCSS(part.body, customCSSNames); - return { + parts.push({ name: 'css', body: parsedCSS.selector, source: part.body - }; + }); + continue; } if (kNestedSelectorNames.has(part.name)) { let innerSelector: string; @@ -69,17 +71,21 @@ export function parseSelector(selector: string): ParsedSelector { } catch (e) { throw new InvalidSelectorError(`Malformed selector: ${part.name}=` + part.body); } - const result = { name: part.name, source: part.body, body: { parsed: parseSelector(innerSelector), distance } }; - if (result.body.parsed.parts.some(part => part.name === 'internal:control' && part.body === 'enter-frame')) - throw new InvalidSelectorError(`Frames are not allowed inside "${part.name}" selectors`); - return result; + const nested = { name: part.name, source: part.body, body: { parsed: parseSelector(innerSelector), distance } }; + const lastFrame = [...nested.body.parsed.parts].reverse().find(part => part.name === 'internal:control' && part.body === 'enter-frame'); + const lastFrameIndex = lastFrame ? nested.body.parsed.parts.indexOf(lastFrame) : -1; + // Allow nested selectors to start with the same frame selector. + if (lastFrameIndex !== -1 && selectorPartsEqual(nested.body.parsed.parts.slice(0, lastFrameIndex + 1), parts.slice(0, lastFrameIndex + 1))) + nested.body.parsed.parts.splice(0, lastFrameIndex + 1); + parts.push(nested); + continue; } - return { ...part, source: part.body }; - }); + parts.push({ ...part, source: part.body }); + } if (kNestedSelectorNames.has(parts[0].name)) throw new InvalidSelectorError(`"${parts[0].name}" selector cannot be first`); return { - capture: result.capture, + capture: parsedStrings.capture, parts }; } @@ -113,6 +119,10 @@ export function splitSelectorByFrame(selectorText: string): ParsedSelector[] { return result; } +function selectorPartsEqual(list1: ParsedSelectorPart[], list2: ParsedSelectorPart[]) { + return stringifySelector({ parts: list1 }) === stringifySelector({ parts: list2 }); +} + export function stringifySelector(selector: string | ParsedSelector): string { if (typeof selector === 'string') return selector; @@ -122,17 +132,15 @@ export function stringifySelector(selector: string | ParsedSelector): string { }).join(' >> '); } -export function allEngineNames(selector: ParsedSelector): Set { - const result = new Set(); - const visit = (selector: ParsedSelector) => { +export function visitAllSelectorParts(selector: ParsedSelector, visitor: (part: ParsedSelectorPart, nested: boolean) => void) { + const visit = (selector: ParsedSelector, nested: boolean) => { for (const part of selector.parts) { - result.add(part.name); + visitor(part, nested); if (kNestedSelectorNames.has(part.name)) - visit((part.body as NestedSelectorBody).parsed); + visit((part.body as NestedSelectorBody).parsed, true); } }; - visit(selector); - return result; + visit(selector, false); } function parseSelectorString(selector: string): ParsedSelectorStrings { diff --git a/tests/page/locator-query.spec.ts b/tests/page/locator-query.spec.ts index 2e266931ab..7631b0d20b 100644 --- a/tests/page/locator-query.spec.ts +++ b/tests/page/locator-query.spec.ts @@ -189,6 +189,17 @@ it('should support locator.or', async ({ page }) => { await expect(page.locator('span').or(page.locator('article'))).toHaveText('world'); }); +it('should allow some, but not all nested frameLocators', async ({ page }) => { + await page.setContent(`hello`); + await expect(page.frameLocator('iframe').locator('span').or(page.frameLocator('iframe').locator('article'))).toHaveText('world'); + await expect(page.frameLocator('iframe').locator('article').or(page.frameLocator('iframe').locator('span'))).toHaveText('world'); + await expect(page.frameLocator('iframe').locator('span').and(page.frameLocator('iframe').locator('#target'))).toHaveText('world'); + const error1 = await expect(page.frameLocator('iframe').locator('div').or(page.frameLocator('#iframe').locator('span'))).toHaveText('world').catch(e => e); + expect(error1.message).toContain(`Frame locators are not allowed inside composite locators, while querying "frameLocator('iframe').locator('div').or(frameLocator('#iframe').locator('span'))`); + const error2 = await expect(page.frameLocator('iframe').locator('div').and(page.frameLocator('#iframe').locator('span'))).toHaveText('world').catch(e => e); + expect(error2.message).toContain(`Frame locators are not allowed inside composite locators, while querying "frameLocator('iframe').locator('div').and(frameLocator('#iframe').locator('span'))`); +}); + it('should enforce same frame for has/leftOf/rightOf/above/below/near', async ({ page, server }) => { await page.goto(server.PREFIX + '/frames/two-frames.html'); const child = page.frames()[1];