From c45dacc834303458ec0685f5f6b63a82bb8e2083 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 4 Feb 2022 07:34:23 -0800 Subject: [PATCH] feat(codegen): make selector generator strict (#11856) This is required to migrate to locators. --- .../src/server/injected/injectedScript.ts | 2 +- .../src/server/injected/selectorGenerator.ts | 54 ++++++++++--------- .../server/supplements/injected/consoleApi.ts | 2 +- .../server/supplements/injected/recorder.ts | 4 +- tests/page/page-strict.spec.ts | 4 +- tests/selector-generator.spec.ts | 14 ++--- 6 files changed, 43 insertions(+), 37 deletions(-) diff --git a/packages/playwright-core/src/server/injected/injectedScript.ts b/packages/playwright-core/src/server/injected/injectedScript.ts index 259e94cdeb..1068977387 100644 --- a/packages/playwright-core/src/server/injected/injectedScript.ts +++ b/packages/playwright-core/src/server/injected/injectedScript.ts @@ -848,7 +848,7 @@ export class InjectedScript { strictModeViolationError(selector: ParsedSelector, matches: Element[]): Error { const infos = matches.slice(0, 10).map(m => ({ preview: this.previewNode(m), - selector: generateSelector(this, m).selector + selector: generateSelector(this, m, true).selector })); const lines = infos.map((info, i) => `\n ${i + 1}) ${info.preview} aka playwright.$("${info.selector}")`); if (infos.length < matches.length) diff --git a/packages/playwright-core/src/server/injected/selectorGenerator.ts b/packages/playwright-core/src/server/injected/selectorGenerator.ts index 39e44a3c34..f83d93fba0 100644 --- a/packages/playwright-core/src/server/injected/selectorGenerator.ts +++ b/packages/playwright-core/src/server/injected/selectorGenerator.ts @@ -25,6 +25,7 @@ type SelectorToken = { const cacheAllowText = new Map(); const cacheDisallowText = new Map(); +const kNthScore = 1000; export function querySelector(injectedScript: InjectedScript, selector: string, ownerDocument: Document): { selector: string, elements: Element[] } { try { @@ -41,12 +42,12 @@ export function querySelector(injectedScript: InjectedScript, selector: string, } } -export function generateSelector(injectedScript: InjectedScript, targetElement: Element): { selector: string, elements: Element[] } { +export function generateSelector(injectedScript: InjectedScript, targetElement: Element, strict: boolean): { selector: string, elements: Element[] } { injectedScript._evaluator.begin(); try { targetElement = targetElement.closest('button,select,input,[role=button],[role=checkbox],[role=radio]') || targetElement; - const targetTokens = generateSelectorFor(injectedScript, targetElement); - const bestTokens = targetTokens || [cssFallback(injectedScript, targetElement)]; + const targetTokens = generateSelectorFor(injectedScript, targetElement, strict); + const bestTokens = targetTokens || cssFallback(injectedScript, targetElement, strict); const selector = joinTokens(bestTokens); const parsedSelector = injectedScript.parseSelector(selector); return { @@ -65,7 +66,7 @@ function filterRegexTokens(textCandidates: SelectorToken[][]): SelectorToken[][] return textCandidates.filter(c => c[0].selector[0] !== '/'); } -function generateSelectorFor(injectedScript: InjectedScript, targetElement: Element): SelectorToken[] | null { +function generateSelectorFor(injectedScript: InjectedScript, targetElement: Element, strict: boolean): SelectorToken[] | null { if (targetElement.ownerDocument.documentElement === targetElement) return [{ engine: 'css', selector: 'html', score: 1 }]; @@ -80,7 +81,7 @@ function generateSelectorFor(injectedScript: InjectedScript, targetElement: Elem const noTextCandidates = buildCandidates(injectedScript, element).map(token => [token]); // First check all text and non-text candidates for the element. - let result = chooseFirstSelector(injectedScript, targetElement.ownerDocument, element, [...textCandidates, ...noTextCandidates], allowNthMatch); + let result = chooseFirstSelector(injectedScript, targetElement.ownerDocument, element, [...textCandidates, ...noTextCandidates], allowNthMatch, strict); // Do not use regex for chained selectors (for performance). textCandidates = filterRegexTokens(textCandidates); @@ -110,7 +111,7 @@ function generateSelectorFor(injectedScript: InjectedScript, targetElement: Elem if (result && combineScores([...parentTokens, ...bestPossibleInParent]) >= combineScores(result)) continue; // Update the best candidate that finds "element" in the "parent". - bestPossibleInParent = chooseFirstSelector(injectedScript, parent, element, candidates, allowNthMatch); + bestPossibleInParent = chooseFirstSelector(injectedScript, parent, element, candidates, allowNthMatch, strict); if (!bestPossibleInParent) return; const combined = [...parentTokens, ...bestPossibleInParent]; @@ -214,7 +215,7 @@ function makeSelectorForId(id: string) { return /^[a-zA-Z][a-zA-Z0-9\-\_]+$/.test(id) ? '#' + id : `[id="${cssEscape(id)}"]`; } -function cssFallback(injectedScript: InjectedScript, targetElement: Element): SelectorToken { +function cssFallback(injectedScript: InjectedScript, targetElement: Element, strict: boolean): SelectorToken[] { const kFallbackScore = 10000000; const root: Node = targetElement.ownerDocument; const tokens: string[] = []; @@ -229,6 +230,18 @@ function cssFallback(injectedScript: InjectedScript, targetElement: Element): Se return node === targetElement ? selector : undefined; } + function makeStrict(selector: string): SelectorToken[] { + const token = { engine: 'css', selector, score: kFallbackScore }; + if (!strict) + return [token]; + const parsedSelector = injectedScript.parseSelector(selector); + const elements = injectedScript.querySelectorAll(parsedSelector, targetElement.ownerDocument); + if (elements.length === 1) + return [token]; + const nth = { engine: 'nth', selector: String(elements.indexOf(targetElement)), score: kNthScore }; + return [token, nth]; + } + for (let element: Element | null = targetElement; element && element !== root; element = parentElementOrShadowHost(element)) { const nodeName = element.nodeName.toLowerCase(); @@ -238,7 +251,7 @@ function cssFallback(injectedScript: InjectedScript, targetElement: Element): Se const token = makeSelectorForId(element.id); const selector = uniqueCSSSelector(token); if (selector) - return { engine: 'css', selector, score: kFallbackScore }; + return makeStrict(selector); bestTokenForLevel = token; } @@ -250,7 +263,7 @@ function cssFallback(injectedScript: InjectedScript, targetElement: Element): Se const token = '.' + classes.slice(0, i + 1).join('.'); const selector = uniqueCSSSelector(token); if (selector) - return { engine: 'css', selector, score: kFallbackScore }; + return makeStrict(selector); // Even if not unique, does this subset of classes uniquely identify node as a child? if (!bestTokenForLevel && parent) { const sameClassSiblings = parent.querySelectorAll(token); @@ -266,7 +279,7 @@ function cssFallback(injectedScript: InjectedScript, targetElement: Element): Se const token = sameTagSiblings.indexOf(element) === 0 ? cssEscape(nodeName) : `${cssEscape(nodeName)}:nth-child(${1 + siblings.indexOf(element)})`; const selector = uniqueCSSSelector(token); if (selector) - return { engine: 'css', selector, score: kFallbackScore }; + return makeStrict(selector); if (!bestTokenForLevel) bestTokenForLevel = token; } else if (!bestTokenForLevel) { @@ -274,7 +287,7 @@ function cssFallback(injectedScript: InjectedScript, targetElement: Element): Se } tokens.unshift(bestTokenForLevel); } - return { engine: 'css', selector: uniqueCSSSelector()!, score: kFallbackScore }; + return makeStrict(uniqueCSSSelector()!); } function escapeForRegex(text: string): string { @@ -307,7 +320,7 @@ function combineScores(tokens: SelectorToken[]): number { return score; } -function chooseFirstSelector(injectedScript: InjectedScript, scope: Element | Document, targetElement: Element, selectors: SelectorToken[][], allowNthMatch: boolean): SelectorToken[] | null { +function chooseFirstSelector(injectedScript: InjectedScript, scope: Element | Document, targetElement: Element, selectors: SelectorToken[][], allowNthMatch: boolean, strict: boolean): SelectorToken[] | null { const joined = selectors.map(tokens => ({ tokens, score: combineScores(tokens) })); joined.sort((a, b) => a.score - b.score); @@ -315,26 +328,19 @@ function chooseFirstSelector(injectedScript: InjectedScript, scope: Element | Do for (const { tokens } of joined) { const parsedSelector = injectedScript.parseSelector(joinTokens(tokens)); const result = injectedScript.querySelectorAll(parsedSelector, scope); + const isStrictEnough = !strict || result.length === 1; const index = result.indexOf(targetElement); - if (index === 0) { + if (index === 0 && isStrictEnough) { // We are the first match - found the best selector. return tokens; } - // Otherwise, perhaps we can get nth-match? + // Otherwise, perhaps we can use nth=? if (!allowNthMatch || bestWithIndex || index === -1 || result.length > 5) continue; - // To use nth-match, we must convert everything to css. - const allCss = tokens.map(token => { - if (token.engine !== 'text') - return token; - if (token.selector.startsWith('/') && token.selector.endsWith('/')) - return { engine: 'css', selector: `:text-matches("${token.selector.substring(1, token.selector.length - 1)}")`, score: token.score }; - return { engine: 'css', selector: `:text("${token.selector}")`, score: token.score }; - }); - const combined = joinTokens(allCss); - bestWithIndex = [{ engine: 'css', selector: `:nth-match(${combined}, ${index + 1})`, score: combineScores(allCss) + 1000 }]; + const nth: SelectorToken = { engine: 'nth', selector: String(index), score: kNthScore }; + bestWithIndex = [...tokens, nth]; } return bestWithIndex; } diff --git a/packages/playwright-core/src/server/supplements/injected/consoleApi.ts b/packages/playwright-core/src/server/supplements/injected/consoleApi.ts index 2f818eadd2..2ef763bfa3 100644 --- a/packages/playwright-core/src/server/supplements/injected/consoleApi.ts +++ b/packages/playwright-core/src/server/supplements/injected/consoleApi.ts @@ -104,7 +104,7 @@ export class ConsoleAPI { private _selector(element: Element) { if (!(element instanceof Element)) throw new Error(`Usage: playwright.selector(element).`); - return generateSelector(this._injectedScript, element).selector; + return generateSelector(this._injectedScript, element, true).selector; } private _resume() { diff --git a/packages/playwright-core/src/server/supplements/injected/recorder.ts b/packages/playwright-core/src/server/supplements/injected/recorder.ts index 9461cd422f..4487241b34 100644 --- a/packages/playwright-core/src/server/supplements/injected/recorder.ts +++ b/packages/playwright-core/src/server/supplements/injected/recorder.ts @@ -237,7 +237,7 @@ export class Recorder { private _onFocus() { const activeElement = this._deepActiveElement(document); - const result = activeElement ? generateSelector(this._injectedScript, activeElement) : null; + const result = activeElement ? generateSelector(this._injectedScript, activeElement, true) : null; this._activeModel = result && result.selector ? result : null; if (this._params.isUnderTest) console.error('Highlight updated for test: ' + (result ? result.selector : null)); // eslint-disable-line no-console @@ -250,7 +250,7 @@ export class Recorder { return; } const hoveredElement = this._hoveredElement; - const { selector, elements } = generateSelector(this._injectedScript, hoveredElement); + const { selector, elements } = generateSelector(this._injectedScript, hoveredElement, true); if ((this._hoveredModel && this._hoveredModel.selector === selector) || this._hoveredElement !== hoveredElement) return; this._hoveredModel = selector ? { selector, elements } : null; diff --git a/tests/page/page-strict.spec.ts b/tests/page/page-strict.spec.ts index 8d0e12721a..fc64cc1914 100644 --- a/tests/page/page-strict.spec.ts +++ b/tests/page/page-strict.spec.ts @@ -34,7 +34,7 @@ it('should fail page.fill in strict mode', async ({ page }) => { await page.setContent(`
`); const error = await page.fill('input', 'text', { strict: true }).catch(e => e); expect(error.message).toContain('strict mode violation'); - expect(error.message).toContain('1) aka playwright.$("input")'); + expect(error.message).toContain('1) aka playwright.$("input >> nth=0")'); expect(error.message).toContain('2) aka playwright.$("div input")'); }); @@ -54,6 +54,6 @@ it('should fail page.dispatchEvent in strict mode', async ({ page }) => { await page.setContent(`
`); const error = await page.dispatchEvent('span', 'click', {}, { strict: true }).catch(e => e); expect(error.message).toContain('strict mode violation'); - expect(error.message).toContain('1) aka playwright.$("span")'); + expect(error.message).toContain('1) aka playwright.$("span >> nth=0")'); expect(error.message).toContain('2) aka playwright.$("div span")'); }); diff --git a/tests/selector-generator.spec.ts b/tests/selector-generator.spec.ts index 7c5da574ac..b1ce30a419 100644 --- a/tests/selector-generator.spec.ts +++ b/tests/selector-generator.spec.ts @@ -73,12 +73,12 @@ it.describe('selector generator', () => { `); - expect(await generate(page, '[mark="1"]')).toBe(':nth-match(select, 2)'); + expect(await generate(page, '[mark="1"]')).toBe('select >> nth=1'); }); it('should use ordinal for identical nodes', async ({ page }) => { await page.setContent(`
Text
Text
Text
Text
`); - expect(await generate(page, 'div[mark="1"]')).toBe(`:nth-match(:text("Text"), 3)`); + expect(await generate(page, 'div[mark="1"]')).toBe(`text=Text >> nth=2`); }); it('should prefer data-testid', async ({ page }) => { @@ -94,7 +94,7 @@ it.describe('selector generator', () => {
Text
`); - expect(await generate(page, 'div[mark="1"]')).toBe('[data-testid="a"]'); + expect(await generate(page, 'div[mark="1"]')).toBe('[data-testid="a"] >> nth=0'); }); it('should handle second non-unique data-testid', async ({ page }) => { @@ -105,7 +105,7 @@ it.describe('selector generator', () => {
Text
`); - expect(await generate(page, 'div[mark="1"]')).toBe(`:nth-match([data-testid="a"], 2)`); + expect(await generate(page, 'div[mark="1"]')).toBe(`[data-testid="a"] >> nth=1`); }); it('should use readable id', async ({ page }) => { @@ -121,7 +121,7 @@ it.describe('selector generator', () => {
`); - expect(await generate(page, 'div[mark="1"]')).toBe(`:nth-match(div, 2)`); + expect(await generate(page, 'div[mark="1"]')).toBe(`div >> nth=1`); }); it('should use has-text', async ({ page }) => { @@ -195,7 +195,7 @@ it.describe('selector generator', () => { `); - expect(await generate(page, 'input[mark="1"]')).toBe(':nth-match(input, 2)'); + expect(await generate(page, 'input[mark="1"]')).toBe('input >> nth=1'); }); it.describe('should prioritise input element attributes correctly', () => { @@ -249,7 +249,7 @@ it.describe('selector generator', () => { input2.setAttribute('value', 'foo'); shadowRoot2.appendChild(input2); }); - expect(await generate(page, 'input[value=foo]')).toBe(':nth-match(input, 3)'); + expect(await generate(page, 'input[value=foo]')).toBe('input >> nth=2'); }); it('should work in dynamic iframes without navigation', async ({ page }) => {