Skip to content

Commit 817e172

Browse files
pavelfeldmanpavelfeldman
authored
chore: do not escape css selectors for .css syntax (microsoft#36081)
1 parent d1eb958 commit 817e172

4 files changed

Lines changed: 69 additions & 55 deletions

File tree

‎packages/injected/src/selectorGenerator.ts‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
* limitations under the License.
1515
*/
1616

17-
import { cssEscape, escapeForAttributeSelector, escapeForTextSelector, escapeRegExp, quoteCSSAttributeValue } from '@isomorphic/stringUtils';
17+
import { escapeForAttributeSelector, escapeForTextSelector, escapeRegExp, quoteCSSAttributeValue } from '@isomorphic/stringUtils';
1818

1919
import { closestCrossShadow, isElementVisible, isInsideScope, parentElementOrShadowHost } from './domUtils';
2020
import { beginAriaCaches, endAriaCaches, getAriaRole, getElementAccessibleName } from './roleUtils';
@@ -245,13 +245,13 @@ function buildNoTextCandidates(injectedScript: InjectedScript, element: Element,
245245
candidates.push({ engine: 'css', selector: makeSelectorForId(idAttr), score: kCSSIdScore });
246246
}
247247

248-
candidates.push({ engine: 'css', selector: cssEscape(element.nodeName.toLowerCase()), score: kCSSTagNameScore });
248+
candidates.push({ engine: 'css', selector: escapeNodeName(element), score: kCSSTagNameScore });
249249
}
250250

251251
if (element.nodeName === 'IFRAME') {
252252
for (const attribute of ['name', 'title']) {
253253
if (element.getAttribute(attribute))
254-
candidates.push({ engine: 'css', selector: `${cssEscape(element.nodeName.toLowerCase())}[${attribute}=${quoteCSSAttributeValue(element.getAttribute(attribute)!)}]`, score: kIframeByAttributeScore });
254+
candidates.push({ engine: 'css', selector: `${escapeNodeName(element)}[${attribute}=${quoteCSSAttributeValue(element.getAttribute(attribute)!)}]`, score: kIframeByAttributeScore });
255255
}
256256

257257
// Locate by testId via CSS selector.
@@ -288,15 +288,15 @@ function buildNoTextCandidates(injectedScript: InjectedScript, element: Element,
288288
candidates.push({ engine: 'internal:role', selector: ariaRole, score: kRoleWithoutNameScore });
289289

290290
if (element.getAttribute('name') && ['BUTTON', 'FORM', 'FIELDSET', 'FRAME', 'IFRAME', 'INPUT', 'KEYGEN', 'OBJECT', 'OUTPUT', 'SELECT', 'TEXTAREA', 'MAP', 'META', 'PARAM'].includes(element.nodeName))
291-
candidates.push({ engine: 'css', selector: `${cssEscape(element.nodeName.toLowerCase())}[name=${quoteCSSAttributeValue(element.getAttribute('name')!)}]`, score: kCSSInputTypeNameScore });
291+
candidates.push({ engine: 'css', selector: `${escapeNodeName(element)}[name=${quoteCSSAttributeValue(element.getAttribute('name')!)}]`, score: kCSSInputTypeNameScore });
292292

293293
if (['INPUT', 'TEXTAREA'].includes(element.nodeName) && element.getAttribute('type') !== 'hidden') {
294294
if (element.getAttribute('type'))
295-
candidates.push({ engine: 'css', selector: `${cssEscape(element.nodeName.toLowerCase())}[type=${quoteCSSAttributeValue(element.getAttribute('type')!)}]`, score: kCSSInputTypeNameScore });
295+
candidates.push({ engine: 'css', selector: `${escapeNodeName(element)}[type=${quoteCSSAttributeValue(element.getAttribute('type')!)}]`, score: kCSSInputTypeNameScore });
296296
}
297297

298298
if (['INPUT', 'TEXTAREA', 'SELECT'].includes(element.nodeName) && element.getAttribute('type') !== 'hidden')
299-
candidates.push({ engine: 'css', selector: cssEscape(element.nodeName.toLowerCase()), score: kCSSInputTypeNameScore + 1 });
299+
candidates.push({ engine: 'css', selector: escapeNodeName(element), score: kCSSInputTypeNameScore + 1 });
300300

301301
penalizeScoreForLength([candidates]);
302302
return candidates;
@@ -330,7 +330,7 @@ function buildTextCandidates(injectedScript: InjectedScript, element: Element, i
330330
for (const alternative of textAlternatives)
331331
candidates.push([{ engine: 'internal:text', selector: escapeForTextSelector(alternative.text, false), score: kTextScore - alternative.scoreBonus }]);
332332
}
333-
const cssToken: SelectorToken = { engine: 'css', selector: cssEscape(element.nodeName.toLowerCase()), score: kCSSTagNameScore };
333+
const cssToken: SelectorToken = { engine: 'css', selector: escapeNodeName(element), score: kCSSTagNameScore };
334334
for (const alternative of textAlternatives)
335335
candidates.push([cssToken, { engine: 'internal:has-text', selector: escapeForTextSelector(alternative.text, false), score: kTextScore - alternative.scoreBonus }]);
336336
if (text.length <= 80) {
@@ -363,7 +363,7 @@ function buildTextCandidates(injectedScript: InjectedScript, element: Element, i
363363
}
364364

365365
function makeSelectorForId(id: string) {
366-
return /^[a-zA-Z][a-zA-Z0-9\-\_]+$/.test(id) ? '#' + id : `[id="${cssEscape(id)}"]`;
366+
return /^[a-zA-Z][a-zA-Z0-9\-\_]+$/.test(id) ? '#' + id : `[id=${quoteCSSAttributeValue(id)}]`;
367367
}
368368

369369
function hasCSSIdToken(tokens: SelectorToken[]) {
@@ -395,8 +395,6 @@ function cssFallback(injectedScript: InjectedScript, targetElement: Element, opt
395395
}
396396

397397
for (let element: Element | undefined = targetElement; element && element !== root; element = parentElementOrShadowHost(element)) {
398-
const nodeName = element.nodeName.toLowerCase();
399-
400398
let bestTokenForLevel: string = '';
401399

402400
// Element ID is the strongest signal, use it.
@@ -411,9 +409,9 @@ function cssFallback(injectedScript: InjectedScript, targetElement: Element, opt
411409
const parent = element.parentNode as (Element | ShadowRoot);
412410

413411
// Combine class names until unique.
414-
const classes = [...element.classList];
412+
const classes = [...element.classList].map(escapeClassName);
415413
for (let i = 0; i < classes.length; ++i) {
416-
const token = '.' + cssEscape(classes.slice(0, i + 1).join('.'));
414+
const token = '.' + classes.slice(0, i + 1).join('.');
417415
const selector = uniqueCSSSelector(token);
418416
if (selector)
419417
return makeStrict(selector);
@@ -428,15 +426,16 @@ function cssFallback(injectedScript: InjectedScript, targetElement: Element, opt
428426
// Ordinal is the weakest signal.
429427
if (parent) {
430428
const siblings = [...parent.children];
431-
const sameTagSiblings = siblings.filter(sibling => (sibling).nodeName.toLowerCase() === nodeName);
432-
const token = sameTagSiblings.indexOf(element) === 0 ? cssEscape(nodeName) : `${cssEscape(nodeName)}:nth-child(${1 + siblings.indexOf(element)})`;
429+
const nodeName = element.nodeName;
430+
const sameTagSiblings = siblings.filter(sibling => sibling.nodeName === nodeName);
431+
const token = sameTagSiblings.indexOf(element) === 0 ? escapeNodeName(element) : `${escapeNodeName(element)}:nth-child(${1 + siblings.indexOf(element)})`;
433432
const selector = uniqueCSSSelector(token);
434433
if (selector)
435434
return makeStrict(selector);
436435
if (!bestTokenForLevel)
437436
bestTokenForLevel = token;
438437
} else if (!bestTokenForLevel) {
439-
bestTokenForLevel = cssEscape(nodeName);
438+
bestTokenForLevel = escapeNodeName(element);
440439
}
441440
tokens.unshift(bestTokenForLevel);
442441
}
@@ -572,3 +571,13 @@ function suitableTextAlternatives(text: string) {
572571

573572
return result;
574573
}
574+
575+
function escapeNodeName(node: Node): string {
576+
// We are escaping it for document.querySelectorAll, not for usage in CSS file.
577+
return node.nodeName.toLocaleLowerCase().replace(/[:\.]/g, char => '\\' + char);
578+
}
579+
580+
function escapeClassName(className: string): string {
581+
// We are escaping it for document.querySelectorAll, not for usage in CSS file.
582+
return className.replace(/[:\.]/g, char => '\\' + char);
583+
}

‎packages/playwright-core/src/utils/isomorphic/stringUtils.ts‎

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -47,31 +47,8 @@ export function toSnakeCase(name: string): string {
4747
return name.replace(/([a-z0-9])([A-Z])/g, '$1_$2').replace(/([A-Z])([A-Z][a-z])/g, '$1_$2').toLowerCase();
4848
}
4949

50-
export function cssEscape(s: string): string {
51-
let result = '';
52-
for (let i = 0; i < s.length; i++)
53-
result += cssEscapeOne(s, i);
54-
return result;
55-
}
56-
5750
export function quoteCSSAttributeValue(text: string): string {
58-
return `"${cssEscape(text).replace(/\\ /g, ' ')}"`;
59-
}
60-
61-
function cssEscapeOne(s: string, i: number): string {
62-
// https://drafts.csswg.org/cssom/#serialize-an-identifier
63-
const c = s.charCodeAt(i);
64-
if (c === 0x0000)
65-
return '\uFFFD';
66-
if ((c >= 0x0001 && c <= 0x001f) ||
67-
(c >= 0x0030 && c <= 0x0039 && (i === 0 || (i === 1 && s.charCodeAt(0) === 0x002d))))
68-
return '\\' + c.toString(16) + ' ';
69-
if (i === 0 && c === 0x002d && s.length === 1)
70-
return '\\' + s.charAt(i);
71-
if (c >= 0x0080 || c === 0x002d || c === 0x005f || (c >= 0x0030 && c <= 0x0039) ||
72-
(c >= 0x0041 && c <= 0x005a) || (c >= 0x0061 && c <= 0x007a))
73-
return s.charAt(i);
74-
return '\\' + s.charAt(i);
51+
return `"${text.replace(/["\\]/g, char => '\\' + char)}"`;
7552
}
7653

7754
let normalizedWhitespaceCache: Map<string, string> | undefined;

‎tests/library/inspector/cli-codegen-3.spec.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -316,19 +316,19 @@ await page.Locator("#frame1").ContentFrame.Locator("iframe").ContentFrame.Locato
316316
page.locator('iframe[name="foo<bar\'\\"`>"]').contentFrame().getByRole('button', { name: 'Click me' }).click(),
317317
]);
318318
expect.soft(sources.get('JavaScript')!.text).toContain(`
319-
await page.locator('iframe[name="foo\\\\<bar\\\\\\'\\\\"\\\\\`\\\\>"]').contentFrame().getByRole('button', { name: 'Click me' }).click();`);
319+
await page.locator('iframe[name="foo<bar\\'\\\\\"\`>"]').contentFrame().getByRole('button', { name: 'Click me' }).click()`);
320320

321321
expect.soft(sources.get('Java')!.text).toContain(`
322-
page.locator("iframe[name=\\"foo\\\\<bar\\\\'\\\\\\"\\\\\`\\\\>\\"]").contentFrame().getByRole(AriaRole.BUTTON, new FrameLocator.GetByRoleOptions().setName("Click me")).click()`);
322+
page.locator("iframe[name=\\"foo<bar'\\\\\\"\`>\\"]").contentFrame().getByRole(AriaRole.BUTTON, new FrameLocator.GetByRoleOptions().setName("Click me")).click()`);
323323

324324
expect.soft(sources.get('Python')!.text).toContain(`
325-
page.locator("iframe[name=\\"foo\\\\<bar\\\\'\\\\\\"\\\\\`\\\\>\\"]").content_frame.get_by_role("button", name="Click me").click()`);
325+
page.locator("iframe[name=\\"foo<bar'\\\\\\"\`>\\"]").content_frame.get_by_role("button", name="Click me").click()`);
326326

327327
expect.soft(sources.get('Python Async')!.text).toContain(`
328-
await page.locator("iframe[name=\\"foo\\\\<bar\\\\'\\\\\\"\\\\\`\\\\>\\"]").content_frame.get_by_role("button", name="Click me").click()`);
328+
await page.locator("iframe[name=\\"foo<bar'\\\\\\"\`>\\"]").content_frame.get_by_role("button", name="Click me").click()`);
329329

330330
expect.soft(sources.get('C#')!.text).toContain(`
331-
await page.Locator("iframe[name=\\"foo\\\\<bar\\\\'\\\\\\"\\\\\`\\\\>\\"]").ContentFrame.GetByRole(AriaRole.Button, new() { Name = "Click me" }).ClickAsync();`);
331+
await page.Locator("iframe[name=\\"foo<bar'\\\\\\"\`>\\"]").ContentFrame.GetByRole(AriaRole.Button, new() { Name = "Click me" }).ClickAsync()`);
332332
});
333333

334334
test('should generate frame locators with title attribute', async ({ openRecorder, server }) => {

‎tests/library/selector-generator.spec.ts‎

Lines changed: 39 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,15 @@
1717
import { contextTest as it, expect } from '../config/browserTest';
1818
import type { Page, Frame } from 'playwright-core';
1919

20-
async function generate(pageOrFrame: Page | Frame, target: string): Promise<string> {
21-
return pageOrFrame.$eval(target, e => (window as any).playwright.selector(e));
20+
async function generate(pageOrFrame: Page | Frame, target: string, expected?: string): Promise<string> {
21+
return pageOrFrame.$eval(target, (e, expected) => {
22+
const playwright = (window as any).playwright;
23+
const selector = playwright.selector(e);
24+
const expectedTarget = expected ? playwright.$(expected) : e;
25+
if (playwright.$(selector) === expectedTarget)
26+
return selector;
27+
return 'FAILED: ' + selector;
28+
}, expected);
2229
}
2330

2431
async function generateMultiple(pageOrFrame: Page | Frame, target: string): Promise<string> {
@@ -38,12 +45,12 @@ it.describe('selector generator', () => {
3845

3946
it('should prefer button over inner span', async ({ page }) => {
4047
await page.setContent(`<button><span>text</span></button>`);
41-
expect(await generate(page, 'span')).toBe('internal:role=button[name="text"i]');
48+
expect(await generate(page, 'span', 'button')).toBe('internal:role=button[name="text"i]');
4249
});
4350

4451
it('should prefer role=button over inner span', async ({ page }) => {
4552
await page.setContent(`<div role=button><span>text</span></div>`);
46-
expect(await generate(page, 'span')).toBe('internal:role=button[name="text"i]');
53+
expect(await generate(page, 'span', 'div')).toBe('internal:role=button[name="text"i]');
4754
});
4855

4956
it('should not prefer zero-sized button over inner span', async ({ page }) => {
@@ -296,6 +303,23 @@ it.describe('selector generator', () => {
296303
expect(await generate(page, 'c[mark="1"]')).toBe('b:nth-child(2) > c');
297304
});
298305

306+
it('should prefer class to ordinal', async ({ page }) => {
307+
await page.setContent(`
308+
<div><c></c><c></c><c></c><c></c><c></c><b></b></div>
309+
<div>
310+
<b class="foo">
311+
<c>
312+
</c>
313+
</b>
314+
<b class="foo bar.baz">
315+
<c mark=1></c>
316+
</b>
317+
</div>
318+
<div><b class="foo"></b></div>
319+
`);
320+
expect(await generate(page, 'c[mark="1"]')).toBe('.foo.bar\\.baz > c');
321+
});
322+
299323
it('should properly join child selectors under nested ordinals', async ({ page }) => {
300324
await page.setContent(`
301325
<div><c></c><c></c><c></c><c></c><c></c><b></b></div>
@@ -423,19 +447,18 @@ it.describe('selector generator', () => {
423447

424448
it('should work with tricky attributes', async ({ page }) => {
425449
await page.setContent(`<button id="this:is-my-tricky.id"><span></span></button>`);
426-
expect(await generate(page, 'button')).toBe('[id="this\\:is-my-tricky\\.id"]');
450+
expect(await generate(page, 'button')).toBe('[id="this:is-my-tricky.id"]');
427451

428452
await page.setContent(`<ng:switch><span></span></ng:switch>`);
429453
expect(await generate(page, 'ng\\:switch')).toBe('ng\\:switch');
430454

431455
await page.setContent(`<button><span></span></button><button></button>`);
432-
await page.$eval('span', span => span.textContent = `!#'!?:`);
433-
expect(await generate(page, 'button')).toBe(`internal:role=button[name="!#'!?:"i]`);
434-
expect(await page.$(`role=button[name="!#'!?:"]`)).toBeTruthy();
456+
await page.$eval('span', span => span.textContent = `!#'!?"\\:`);
457+
expect(await generate(page, 'button')).toBe(`internal:role=button[name="!#'!?\\"\\\\:"i]`);
435458

436459
await page.setContent(`<div><span></span></div>`);
437-
await page.$eval('div', div => div.id = `!#'!?:`);
438-
expect(await generate(page, 'div')).toBe("[id=\"\\!\\#\\'\\!\\?\\:\"]");
460+
await page.$eval('div', div => div.id = `!#'!?"\\:`);
461+
expect(await generate(page, 'div')).toBe(`[id="!#'!?\\"\\\\:"]`);
439462
});
440463

441464
it('should work without CSS.escape', async ({ page }) => {
@@ -444,7 +467,12 @@ it.describe('selector generator', () => {
444467
delete window.CSS.escape;
445468
button.setAttribute('name', '-tricky\u0001name');
446469
});
447-
expect(await generate(page, 'button')).toBe(`button[name="-tricky\\1 name"]`);
470+
expect(await generate(page, 'button')).toBe(`button[name="-tricky\u0001name"]`);
471+
});
472+
473+
it('should not over-escape for CSS syntax', async ({ page }) => {
474+
await page.setContent(`<button aria-hidden="false" name="123"></button><div role="button"></div>`);
475+
expect(await generate(page, 'button')).toBe(`button[name="123"]`);
448476
});
449477

450478
it('should ignore empty aria-label for candidate consideration', async ({ page }) => {

0 commit comments

Comments
 (0)