Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(codegen): fix unselect issue #32127

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/playwright-core/src/server/codegen/java.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export class JavaLanguageGenerator implements LanguageGenerator {
case 'navigate':
return `${subject}.navigate(${quote(action.url)});`;
case 'select':
return `${subject}.${this._asLocator(action.selector, inFrameLocator)}.selectOption(${formatSelectOption(action.options.length > 1 ? action.options : action.options[0])});`;
return `${subject}.${this._asLocator(action.selector, inFrameLocator)}.selectOption(${formatSelectOption(action.options.length === 1 ? action.options[0] : action.options)});`;
case 'assertText':
return `assertThat(${subject}.${this._asLocator(action.selector, inFrameLocator)}).${action.substring ? 'containsText' : 'hasText'}(${quote(action.text)});`;
case 'assertChecked':
Expand Down
2 changes: 1 addition & 1 deletion packages/playwright-core/src/server/codegen/javascript.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export class JavaScriptLanguageGenerator implements LanguageGenerator {
case 'navigate':
return `await ${subject}.goto(${quote(action.url)});`;
case 'select':
return `await ${subject}.${this._asLocator(action.selector)}.selectOption(${formatObject(action.options.length > 1 ? action.options : action.options[0])});`;
return `await ${subject}.${this._asLocator(action.selector)}.selectOption(${formatObject(action.options.length === 1 ? action.options[0] : action.options)});`;
case 'assertText':
return `${this._isTest ? '' : '// '}await expect(${subject}.${this._asLocator(action.selector)}).${action.substring ? 'toContainText' : 'toHaveText'}(${quote(action.text)});`;
case 'assertChecked':
Expand Down
89 changes: 88 additions & 1 deletion tests/library/inspector/cli-codegen-1.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import { test, expect } from './inspectorTest';
import type { ConsoleMessage } from 'playwright';
import type { ConsoleMessage, Locator } from 'playwright';

test.describe('cli codegen', () => {
test.skip(({ mode }) => mode !== 'default');
Expand Down Expand Up @@ -682,6 +682,93 @@ await page.Locator(\"#age\").SelectOptionAsync(new[] { \"2\" });`);
expect(message.text()).toBe('2');
});

const clickMultipleSelectOption = async (locator: Locator, withCtrlOrMeta = false) => {
const page = locator.page();

// Webkit can't click multiple select options
// https://github.com/microsoft/playwright/issues/32126
if (page.context().browser().browserType().name() === 'webkit') {
const elem = await locator.elementHandle();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

element handles are discouraged, there is a way to get bounding box from locator in one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boundingBox returns null as it's assumed as not visible. That's why I use eval thing here

const rect = await elem!.evaluate(e => {
return e.getBoundingClientRect()!;
});
if (withCtrlOrMeta)
await page.keyboard.down('ControlOrMeta');

await page.mouse.click(rect.x + rect.width / 2, rect.y + rect.height / 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While investigating test failure on webkit, I noticed locator.click for multiple select option doesn't work.

I think it's another problem so I've created an issue

#32126

if (withCtrlOrMeta)
await page.keyboard.up('ControlOrMeta');

} else {
await locator.click({ modifiers: withCtrlOrMeta ? ['ControlOrMeta'] : [] });
}
};

test('should select with multiple attribute', async ({ openRecorder }) => {
const { page, recorder } = await openRecorder();

await recorder.setContentAndWait(`<select id="age" multiple onchange="console.log('[' + [...age.selectedOptions].map(x => x.value).join(',') + ']')"><option value="1">1</option><option value="2">2</option></select>`);

const locator = await recorder.hoverOverElement('select');
expect(locator).toBe(`locator('#age')`);
await clickMultipleSelectOption(page.getByRole('option', { name: '1' }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope clicking the select instead of option with offset to pick an element would work across the browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavelfeldman
Um, I'm not sure what kind of operation you're referring to. Could you show me a code example?


const [message, sources] = await Promise.all([
page.waitForEvent('console', msg => msg.type() !== 'error' && msg.text().includes('2')),
recorder.waitForOutput('JavaScript', 'selectOption(['),
clickMultipleSelectOption(page.getByRole('option', { name: '2' }), true)
]);

expect(sources.get('JavaScript')!.text).toContain(`
await page.locator('#age').selectOption(['1', '2']);`);

expect(sources.get('Java')!.text).toContain(`
page.locator("#age").selectOption(new String[] {"1", "2"});`);

expect(sources.get('Python')!.text).toContain(`
page.locator("#age").select_option(["1", "2"])`);

expect(sources.get('Python Async')!.text).toContain(`
await page.locator("#age").select_option(["1", "2"])`);

expect(sources.get('C#')!.text).toContain(`
await page.Locator("#age").SelectOptionAsync(new[] { "1", "2" });`);

expect(message.text()).toBe('[1,2]');
});

test('should unselect with multiple attribute', async ({ openRecorder }) => {
const { page, recorder } = await openRecorder();

await recorder.setContentAndWait(`<select id="age" multiple onchange="console.log('[' + [...age.selectedOptions].map(x => x.value).join(',') + ']')"><option value="1">1</option><option value="2">2</option></select>`);
const locator = await recorder.hoverOverElement('select');
expect(locator).toBe(`locator('#age')`);
await clickMultipleSelectOption(page.getByRole('option', { name: '1' }));

const [message, sources] = await Promise.all([
page.waitForEvent('console', msg => msg.type() !== 'error' && msg.text() === '[]'),
recorder.waitForOutput('JavaScript', 'selectOption(['),
clickMultipleSelectOption(page.getByRole('option', { name: '1' }), true)
]);

expect(sources.get('JavaScript')!.text).toContain(`
await page.locator('#age').selectOption([]);`);

expect(sources.get('Java')!.text).toContain(`
page.locator("#age").selectOption(new String[0]);`);

expect(sources.get('Python')!.text).toContain(`
page.locator("#age").select_option([])`);

expect(sources.get('Python Async')!.text).toContain(`
await page.locator("#age").select_option([])`);

expect(sources.get('C#')!.text).toContain(`
await page.Locator("#age").SelectOptionAsync(new[] { });`);

expect(message.text()).toBe('[]');
});

test('should await popup', async ({ openRecorder }) => {
const { page, recorder } = await openRecorder();
await recorder.setContentAndWait('<a target=_blank rel=noopener href="about:blank">link</a>');
Expand Down