Skip to content

Commit

Permalink
fix(locator): locator(locator) method uses internal:chain instead…
Browse files Browse the repository at this point in the history
… of `>>` (#24235)

Usually, we can just chain two locators with `>>` to implement
`Locator.locator(locator)`. However, this does not play nicely with more
advanced inner locators like `or` and `and`:

```ts
const child = page.locator('input').or(page.locator('button'));
page.locator('parent').locator(child);
```

One would expect the above to locate "input or button" inside a
"parent". However, currently it locates "input inside a parent" or
"button", because it's translated to `parent >> input >>
internal:or="button"`.

To fix this, we have to wrap inner locator into `internal:chain` and
query it separately from the parent.

Fixes #23724.
  • Loading branch information
dgozman committed Jul 14, 2023
1 parent 1b1cf87 commit 97d55e2
Show file tree
Hide file tree
Showing 9 changed files with 58 additions and 5 deletions.
2 changes: 1 addition & 1 deletion packages/playwright-core/src/client/locator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export class Locator implements api.Locator {
return new Locator(this._frame, this._selector + ' >> ' + selectorOrLocator, options);
if (selectorOrLocator._frame !== this._frame)
throw new Error(`Locators must belong to the same frame.`);
return new Locator(this._frame, this._selector + ' >> ' + selectorOrLocator._selector, options);
return new Locator(this._frame, this._selector + ' >> internal:chain=' + JSON.stringify(selectorOrLocator._selector), options);
}

getByTestId(testId: string | RegExp): Locator {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ export class InjectedScript {
this._engines.set('internal:has-not', this._createHasNotEngine());
this._engines.set('internal:and', { queryAll: () => [] });
this._engines.set('internal:or', { queryAll: () => [] });
this._engines.set('internal:chain', this._createInternalChainEngine());
this._engines.set('internal:label', this._createInternalLabelEngine());
this._engines.set('internal:text', this._createTextEngine(true, true));
this._engines.set('internal:has-text', this._createInternalHasTextEngine());
Expand Down Expand Up @@ -399,6 +400,13 @@ export class InjectedScript {
return { queryAll };
}

private _createInternalChainEngine(): SelectorEngine {
const queryAll = (root: SelectorRoot, body: NestedSelectorBody) => {
return this.querySelectorAll(body.parsed, root);
};
return { queryAll };
}

extend(source: string, params: any): any {
const constrFunction = this.window.eval(`
(() => {
Expand Down
2 changes: 1 addition & 1 deletion packages/playwright-core/src/server/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export class Selectors {
'nth', 'visible', 'internal:control',
'internal:has', 'internal:has-not',
'internal:has-text', 'internal:has-not-text',
'internal:and', 'internal:or',
'internal:and', 'internal:or', 'internal:chain',
'role', 'internal:attr', 'internal:label', 'internal:text', 'internal:role', 'internal:testid',
]);
this._builtinEnginesInMainWorld = new Set([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { type NestedSelectorBody, parseAttributeSelector, parseSelector, stringi
import type { ParsedSelector } from './selectorParser';

export type Language = 'javascript' | 'python' | 'java' | 'csharp' | 'jsonl';
export type LocatorType = 'default' | 'role' | 'text' | 'label' | 'placeholder' | 'alt' | 'title' | 'test-id' | 'nth' | 'first' | 'last' | 'has-text' | 'has-not-text' | 'has' | 'hasNot' | 'frame' | 'and' | 'or';
export type LocatorType = 'default' | 'role' | 'text' | 'label' | 'placeholder' | 'alt' | 'title' | 'test-id' | 'nth' | 'first' | 'last' | 'has-text' | 'has-not-text' | 'has' | 'hasNot' | 'frame' | 'and' | 'or' | 'chain';
export type LocatorBase = 'page' | 'locator' | 'frame-locator';

type LocatorOptions = {
Expand Down Expand Up @@ -120,6 +120,11 @@ function innerAsLocators(factory: LocatorFactory, parsed: ParsedSelector, isFram
tokens.push(inners.map(inner => factory.generateLocator(base, 'or', inner)));
continue;
}
if (part.name === 'internal:chain') {
const inners = innerAsLocators(factory, (part.body as NestedSelectorBody).parsed, false, maxOutputSize);
tokens.push(inners.map(inner => factory.generateLocator(base, 'chain', inner)));
continue;
}
if (part.name === 'internal:label') {
const { exact, text } = detectExact(part.body as string);
tokens.push([factory.generateLocator(base, 'label', text, { exact })]);
Expand Down Expand Up @@ -285,6 +290,8 @@ export class JavaScriptLocatorFactory implements LocatorFactory {
return `and(${body})`;
case 'or':
return `or(${body})`;
case 'chain':
return `locator(${body})`;
case 'test-id':
return `getByTestId(${this.toTestIdValue(body)})`;
case 'text':
Expand Down Expand Up @@ -375,6 +382,8 @@ export class PythonLocatorFactory implements LocatorFactory {
return `and_(${body})`;
case 'or':
return `or_(${body})`;
case 'chain':
return `locator(${body})`;
case 'test-id':
return `get_by_test_id(${this.toTestIdValue(body)})`;
case 'text':
Expand Down Expand Up @@ -474,6 +483,8 @@ export class JavaLocatorFactory implements LocatorFactory {
return `and(${body})`;
case 'or':
return `or(${body})`;
case 'chain':
return `locator(${body})`;
case 'test-id':
return `getByTestId(${this.toTestIdValue(body)})`;
case 'text':
Expand Down Expand Up @@ -567,6 +578,8 @@ export class CSharpLocatorFactory implements LocatorFactory {
return `And(${body})`;
case 'or':
return `Or(${body})`;
case 'chain':
return `Locator(${body})`;
case 'test-id':
return `GetByTestId(${this.toTestIdValue(body)})`;
case 'text':
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ function shiftParams(template: string, sub: number) {

function transform(template: string, params: TemplateParams, testIdAttributeName: string): string {
// Recursively handle filter(has=, hasnot=, sethas(), sethasnot()).
// TODO: handle and(locator), or(locator), locator(has=, hasnot=, sethas(), sethasnot()).
// TODO: handle and(locator), or(locator), locator(locator), locator(has=, hasnot=, sethas(), sethasnot()).
while (true) {
const hasMatch = template.match(/filter\(,?(has=|hasnot=|sethas\(|sethasnot\()/);
if (!hasMatch)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { InvalidSelectorError, parseCSS } from './cssParser';
export { InvalidSelectorError, isInvalidSelectorError } from './cssParser';

export type NestedSelectorBody = { parsed: ParsedSelector, distance?: number };
const kNestedSelectorNames = new Set(['internal:has', 'internal:has-not', 'internal:and', 'internal:or', 'left-of', 'right-of', 'above', 'below', 'near']);
const kNestedSelectorNames = new Set(['internal:has', 'internal:has-not', 'internal:and', 'internal:or', 'internal:chain', 'left-of', 'right-of', 'above', 'below', 'near']);
const kNestedSelectorNamesWithDistance = new Set(['left-of', 'right-of', 'above', 'below', 'near']);

export type ParsedSelectorPart = {
Expand Down
7 changes: 7 additions & 0 deletions tests/library/locator-generator.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,13 @@ it('asLocator internal:or', async () => {
expect.soft(asLocator('csharp', 'div >> internal:or="span >> article"', false)).toBe(`Locator("div").Or(Locator("span").Locator("article"))`);
});

it('asLocator internal:chain', async () => {
expect.soft(asLocator('javascript', 'div >> internal:chain="span >> article"', false)).toBe(`locator('div').locator(locator('span').locator('article'))`);
expect.soft(asLocator('python', 'div >> internal:chain="span >> article"', false)).toBe(`locator("div").locator(locator("span").locator("article"))`);
expect.soft(asLocator('java', 'div >> internal:chain="span >> article"', false)).toBe(`locator("div").locator(locator("span").locator("article"))`);
expect.soft(asLocator('csharp', 'div >> internal:chain="span >> article"', false)).toBe(`Locator("div").Locator(Locator("span").Locator("article"))`);
});

it('parse locators strictly', () => {
const selector = 'div >> internal:has-text=\"Goodbye world\"i >> span';

Expand Down
15 changes: 15 additions & 0 deletions tests/page/locator-query.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,21 @@ it('should support locator.or', async ({ page }) => {
await expect(page.locator('span').or(page.locator('article'))).toHaveText('world');
});

it('should support locator.locator with and/or', async ({ page }) => {
await page.setContent(`
<div>one <span>two</span> <button>three</button> </div>
<span>four</span>
<button>five</button>
`);

await expect(page.locator('div').locator(page.locator('button'))).toHaveText(['three']);
await expect(page.locator('div').locator(page.locator('button').or(page.locator('span')))).toHaveText(['two', 'three']);
await expect(page.locator('button').or(page.locator('span'))).toHaveText(['two', 'three', 'four', 'five']);

await expect(page.locator('div').locator(page.locator('button').and(page.getByRole('button')))).toHaveText(['three']);
await expect(page.locator('button').and(page.getByRole('button'))).toHaveText(['three', 'five']);
});

it('should allow some, but not all nested frameLocators', async ({ page }) => {
await page.setContent(`<iframe srcdoc="<span id=target>world</span>"></iframe><span>hello</span>`);
await expect(page.frameLocator('iframe').locator('span').or(page.frameLocator('iframe').locator('article'))).toHaveText('world');
Expand Down
10 changes: 10 additions & 0 deletions tests/page/selectors-misc.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,16 @@ it('should work with internal:or=', async ({ page, server }) => {
expect(await page.locator(`span >> internal:or="article"`).textContent()).toBe('world');
});

it('should work with internal:chain=', async ({ page, server }) => {
await page.setContent(`
<div>one <span>two</span> <button>three</button> </div>
<span>four</span>
<button>five</button>
`);
expect(await page.$$eval(`div >> internal:chain="button"`, els => els.map(e => e.textContent))).toEqual(['three']);
expect(await page.$$eval(`div >> internal:chain="span >> internal:or=\\"button\\""`, els => els.map(e => e.textContent))).toEqual(['two', 'three']);
});

it('chaining should work with large DOM @smoke', async ({ page, server }) => {
await page.evaluate(() => {
let last = document.body;
Expand Down

0 comments on commit 97d55e2

Please sign in to comment.