Skip to content

Commit

Permalink
fix(aria snapshot): assorted fixes
Browse files Browse the repository at this point in the history
- RegExp parsing
- text value escaping
- limit string lengths in `longestCommonSubstring`
- failing tests for unresolved edge cases
  • Loading branch information
dgozman committed Nov 8, 2024
1 parent 7073f80 commit a3778b5
Show file tree
Hide file tree
Showing 5 changed files with 133 additions and 14 deletions.
25 changes: 18 additions & 7 deletions packages/playwright-core/src/server/ariaSnapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,18 @@ export class KeyParser {
return this._pos >= this._length;
}

private _isWhitespace() {
return !this._eof() && /\s/.test(this._peek());
}

private _skipWhitespace() {
while (!this._eof() && /\s/.test(this._peek()))
while (this._isWhitespace())
this._pos++;
}

private _readIdentifier(): string {
private _readIdentifier(type: 'role' | 'attribute'): string {
if (this._eof())
this._throwError('Unexpected end of input when expecting identifier');
this._throwError(`Unexpected end of input when expecting ${type}`);
const start = this._pos;
while (!this._eof() && /[a-zA-Z]/.test(this._peek()))
this._pos++;
Expand Down Expand Up @@ -184,6 +188,7 @@ export class KeyParser {
private _readRegex(): string {
let result = '';
let escaped = false;
let insideClass = false;
while (!this._eof()) {
const ch = this._next();
if (escaped) {
Expand All @@ -192,8 +197,14 @@ export class KeyParser {
} else if (ch === '\\') {
escaped = true;
result += ch;
} else if (ch === '/') {
} else if (ch === '/' && !insideClass) {
return result;
} else if (ch === '[') {
insideClass = true;
result += ch;
} else if (ch === ']' && insideClass) {
result += ch;
insideClass = false;
} else {
result += ch;
}
Expand Down Expand Up @@ -223,13 +234,13 @@ export class KeyParser {
if (this._peek() === '[') {
this._next();
this._skipWhitespace();
const flagName = this._readIdentifier();
const flagName = this._readIdentifier('attribute');
this._skipWhitespace();
let flagValue = '';
if (this._peek() === '=') {
this._next();
this._skipWhitespace();
while (this._peek() !== ']' && !this._eof())
while (this._peek() !== ']' && !this._isWhitespace() && !this._eof())
flagValue += this._next();
}
this._skipWhitespace();
Expand All @@ -248,7 +259,7 @@ export class KeyParser {
_parse(): AriaTemplateNode {
this._skipWhitespace();

const role = this._readIdentifier() as AriaTemplateRoleNode['role'];
const role = this._readIdentifier('role') as AriaTemplateRoleNode['role'];
this._skipWhitespace();
const name = this._readStringOrRegex() || '';
const result: AriaTemplateRoleNode = { kind: 'role', role, name };
Expand Down
6 changes: 3 additions & 3 deletions packages/playwright-core/src/server/injected/ariaSnapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ export function renderAriaTree(ariaNode: AriaNode, options?: { mode?: 'raw' | 'r
if (typeof ariaNode === 'string') {
if (parentAriaNode && !includeText(parentAriaNode, ariaNode))
return;
const text = renderString(ariaNode);
const text = yamlEscapeValueIfNeeded(renderString(ariaNode));
if (text)
lines.push(indent + '- text: ' + text);
return;
Expand Down Expand Up @@ -416,8 +416,8 @@ function textContributesInfo(node: AriaNode, text: string): boolean {
if (node.name.length > text.length)
return false;

// Figure out if text adds any value.
const substr = longestCommonSubstring(text, node.name);
// Figure out if text adds any value. "longestCommonSubstring" is expensive, so limit strings length.
const substr = (text.length <= 200 && node.name.length <= 200) ? longestCommonSubstring(text, node.name) : '';
let filtered = text;
while (substr && filtered.includes(substr))
filtered = filtered.replace(substr, '');
Expand Down
4 changes: 4 additions & 0 deletions packages/playwright-core/src/server/injected/yaml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ function yamlStringNeedsQuotes(str: string): boolean {
if (/^[>|]/.test(str))
return true;

// Strings starting with quotes need quotes
if (/^["']/.test(str))
return true;

// Strings containing special characters that could cause ambiguity
if (/[{}`]/.test(str))
return true;
Expand Down
33 changes: 33 additions & 0 deletions tests/page/page-aria-snapshot.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -459,3 +459,36 @@ it('should be ok with circular ownership', async ({ page }) => {
- region: Hello
`);
});

it('should escape yaml text in text nodes', async ({ page }) => {
await page.setContent(`
<details>
<summary>one: <a href="#">link1</a> "two <a href="#">link2</a> 'three <a href="#">link3</a> \`four</summary>
</details>
`);

await checkAndMatchSnapshot(page.locator('body'), `
- group:
- text: "one:"
- link "link1"
- text: "\\\"two"
- link "link2"
- text: "'three"
- link "link3"
- text: "\`four"
`);
});

it.fixme('should handle long strings', async ({ page }) => {
await page.setContent(`
<a href='about:blank'>
<div role='region'>${'a'.repeat(100000)}</div>
</a>
`);

const trimmed = 'a'.repeat(1000);
await checkAndMatchSnapshot(page.locator('body'), `
- link "${trimmed}":
- region: "${trimmed}"
`);
});
79 changes: 75 additions & 4 deletions tests/page/to-match-aria-snapshot.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,30 @@ test('should match complex', async ({ page }) => {
});

test('should match regex', async ({ page }) => {
await page.setContent(`<h1>Issues 12</h1>`);
await expect(page.locator('body')).toMatchAriaSnapshot(`
- heading ${/Issues \d+/}
`);
{
await page.setContent(`<h1>Issues 12</h1>`);
await expect(page.locator('body')).toMatchAriaSnapshot(`
- heading ${/Issues \d+/}
`);
}
{
await page.setContent(`<h1>Issues 1/2</h1>`);
await expect(page.locator('body')).toMatchAriaSnapshot(`
- heading ${/Issues 1[/]2/}
`);
}
{
await page.setContent(`<h1>Issues 1[</h1>`);
await expect(page.locator('body')).toMatchAriaSnapshot(`
- heading ${/Issues 1\[/}
`);
}
{
await page.setContent(`<h1>Issues 1]]2</h1>`);
await expect(page.locator('body')).toMatchAriaSnapshot(`
- heading ${/Issues 1[\]]]2/}
`);
}
});

test('should allow text nodes', async ({ page }) => {
Expand Down Expand Up @@ -472,6 +492,26 @@ test('should unpack escaped names', async ({ page }) => {
- 'button "Click '' me"'
`);
}

{
await page.setContent(`
<h1>heading "name" [level=1]</h1>
`);
await expect(page.locator('body')).toMatchAriaSnapshot(`
- heading "heading \\"name\\" [level=1]" [level=1]
`);
}

{
await page.setContent(`
<h1>heading \\" [level=2]</h1>
`);
await expect(page.locator('body')).toMatchAriaSnapshot(`
- |
heading "heading \\\\\\" [level=2]" [
level = 1 ]
`);
}
});

test('should report error in YAML', async ({ page }) => {
Expand Down Expand Up @@ -579,3 +619,34 @@ test('call log should contain actual snapshot', async ({ page }) => {

expect(stripAnsi(error.message)).toContain(`- unexpected value "- heading "todos" [level=1]"`);
});

test.fixme('should normalize whitespace when matching accessible name', async ({ page }) => {
await page.setContent(`
<button>hello world</button>
`);
await expect(page.locator('body')).toMatchAriaSnapshot(`
- |
button "hello
world"
`);
});

test('should parse attributes', async ({ page }) => {
{
await page.setContent(`
<button aria-pressed="mixed">hello world</button>
`);
await expect(page.locator('body')).toMatchAriaSnapshot(`
- button [pressed=mixed ]
`);
}

{
await page.setContent(`
<h2>hello world</h2>
`);
await expect(page.locator('body')).not.toMatchAriaSnapshot(`
- heading [level = -3 ]
`);
}
});

0 comments on commit a3778b5

Please sign in to comment.