From 615954b2852691602c9fea77264336e175ade971 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Tue, 19 Jan 2021 11:27:05 -0800 Subject: [PATCH] fix(dom): make selectOption wait for options (#5036) --- docs/src/api/class-elementhandle.md | 2 + docs/src/api/class-frame.md | 2 + docs/src/api/class-page.md | 2 + src/server/dom.ts | 11 +-- src/server/injected/injectedScript.ts | 99 ++++++++++++++++----------- test/page-fill.spec.ts | 11 +++ test/page-select-option.spec.ts | 68 +++++++++++++++++- types/types.d.ts | 6 ++ 8 files changed, 156 insertions(+), 45 deletions(-) diff --git a/docs/src/api/class-elementhandle.md b/docs/src/api/class-elementhandle.md index 76709eb0f7..6a0320b05d 100644 --- a/docs/src/api/class-elementhandle.md +++ b/docs/src/api/class-elementhandle.md @@ -545,6 +545,8 @@ Returns the array of option values that have been successfully selected. Triggers a `change` and `input` event once all the provided options have been selected. If element is not a `` element. + ```js // single selection matching the value handle.selectOption('blue'); diff --git a/docs/src/api/class-frame.md b/docs/src/api/class-frame.md index 38f22f3152..1ed7abc7b2 100644 --- a/docs/src/api/class-frame.md +++ b/docs/src/api/class-frame.md @@ -842,6 +842,8 @@ Returns the array of option values that have been successfully selected. Triggers a `change` and `input` event once all the provided options have been selected. If there's no `` element. + ```js // single selection matching the value frame.selectOption('select#colors', 'blue'); diff --git a/docs/src/api/class-page.md b/docs/src/api/class-page.md index c9fc53ab99..b868df5006 100644 --- a/docs/src/api/class-page.md +++ b/docs/src/api/class-page.md @@ -1869,6 +1869,8 @@ Returns the array of option values that have been successfully selected. Triggers a `change` and `input` event once all the provided options have been selected. If there's no `` element. + ```js // single selection matching the value page.selectOption('select#colors', 'blue'); diff --git a/src/server/dom.ts b/src/server/dom.ts index b2f525cad7..7b209eacf8 100644 --- a/src/server/dom.ts +++ b/src/server/dom.ts @@ -446,9 +446,12 @@ export class ElementHandle extends js.JSHandle { const selectOptions = [...elements, ...values]; return this._page._frameManager.waitForSignalsCreatedBy(progress, options.noWaitAfter, async () => { progress.throwIfAborted(); // Avoid action that has side-effects. - const value = await this._evaluateInUtility(([injected, node, selectOptions]) => injected.selectOptions(node, selectOptions), selectOptions); + progress.log(' selecting specified option(s)'); + const poll = await this._evaluateHandleInUtility(([injected, node, selectOptions]) => injected.waitForOptionsAndSelect(node, selectOptions), selectOptions); + const pollHandler = new InjectedScriptPollHandler(progress, poll); + const result = throwFatalDOMError(await pollHandler.finish()); await this._page._doSlowMo(); - return throwFatalDOMError(value); + return result; }); } @@ -817,7 +820,7 @@ export class InjectedScriptPollHandler { async finishHandle(): Promise> { try { - const result = await this._poll!.evaluateHandle(poll => poll.result); + const result = await this._poll!.evaluateHandle(poll => poll.run()); await this._finishInternal(); return result; } finally { @@ -827,7 +830,7 @@ export class InjectedScriptPollHandler { async finish(): Promise { try { - const result = await this._poll!.evaluate(poll => poll.result); + const result = await this._poll!.evaluate(poll => poll.run()); await this._finishInternal(); return result; } finally { diff --git a/src/server/injected/injectedScript.ts b/src/server/injected/injectedScript.ts index ee8c173ab1..d24506128a 100644 --- a/src/server/injected/injectedScript.ts +++ b/src/server/injected/injectedScript.ts @@ -31,7 +31,7 @@ export type InjectedScriptProgress = { }; export type InjectedScriptPoll = { - result: Promise, + run: () => Promise, // Takes more logs, waiting until at least one message is available. takeNextLogs: () => Promise, // Takes all current logs without waiting. @@ -242,19 +242,23 @@ export class InjectedScript { }, }; - const result = task(progress); + const run = () => { + const result = task(progress); - // After the task has finished, there should be no more logs. - // Release any pending `takeNextLogs` call, and do not block any future ones. - // This prevents non-finished protocol evaluation calls and memory leaks. - result.finally(() => { - taskFinished = true; - logReady(); - }); + // After the task has finished, there should be no more logs. + // Release any pending `takeNextLogs` call, and do not block any future ones. + // This prevents non-finished protocol evaluation calls and memory leaks. + result.finally(() => { + taskFinished = true; + logReady(); + }); + + return result; + }; return { takeNextLogs, - result, + run, cancel: () => { progress.aborted = true; }, takeLastLogs: () => unsentLogs, }; @@ -267,35 +271,52 @@ export class InjectedScript { return { left: parseInt(style.borderLeftWidth || '', 10), top: parseInt(style.borderTopWidth || '', 10) }; } - selectOptions(node: Node, optionsToSelect: (Node | { value?: string, label?: string, index?: number })[]): string[] | 'error:notconnected' | FatalDOMError { - const element = this.findLabelTarget(node as Element); - if (!element || !element.isConnected) - return 'error:notconnected'; - if (element.nodeName.toLowerCase() !== 'select') - return 'error:notselect'; - const select = element as HTMLSelectElement; - const options = Array.from(select.options); - select.value = undefined as any; - for (let index = 0; index < options.length; index++) { - const option = options[index]; - option.selected = optionsToSelect.some(optionToSelect => { - if (optionToSelect instanceof Node) - return option === optionToSelect; - let matches = true; - if (optionToSelect.value !== undefined) - matches = matches && optionToSelect.value === option.value; - if (optionToSelect.label !== undefined) - matches = matches && optionToSelect.label === option.label; - if (optionToSelect.index !== undefined) - matches = matches && optionToSelect.index === index; - return matches; - }); - if (option.selected && !select.multiple) - break; - } - select.dispatchEvent(new Event('input', { 'bubbles': true })); - select.dispatchEvent(new Event('change', { 'bubbles': true })); - return options.filter(option => option.selected).map(option => option.value); + waitForOptionsAndSelect(node: Node, optionsToSelect: (Node | { value?: string, label?: string, index?: number })[]): InjectedScriptPoll { + return this.pollRaf((progress, continuePolling) => { + const element = this.findLabelTarget(node as Element); + if (!element || !element.isConnected) + return 'error:notconnected'; + if (element.nodeName.toLowerCase() !== 'select') + return 'error:notselect'; + const select = element as HTMLSelectElement; + const options = Array.from(select.options); + const selectedOptions = []; + let remainingOptionsToSelect = optionsToSelect.slice(); + for (let index = 0; index < options.length; index++) { + const option = options[index]; + const filter = (optionToSelect: Node | { value?: string, label?: string, index?: number }) => { + if (optionToSelect instanceof Node) + return option === optionToSelect; + let matches = true; + if (optionToSelect.value !== undefined) + matches = matches && optionToSelect.value === option.value; + if (optionToSelect.label !== undefined) + matches = matches && optionToSelect.label === option.label; + if (optionToSelect.index !== undefined) + matches = matches && optionToSelect.index === index; + return matches; + }; + if (!remainingOptionsToSelect.some(filter)) + continue; + selectedOptions.push(option); + if (select.multiple) { + remainingOptionsToSelect = remainingOptionsToSelect.filter(o => !filter(o)); + } else { + remainingOptionsToSelect = []; + break; + } + } + if (remainingOptionsToSelect.length) { + progress.logRepeating(' did not find some options - waiting... '); + return continuePolling; + } + select.value = undefined as any; + selectedOptions.forEach(option => option.selected = true); + progress.log(' selected specified option(s)'); + select.dispatchEvent(new Event('input', { 'bubbles': true })); + select.dispatchEvent(new Event('change', { 'bubbles': true })); + return selectedOptions.map(option => option.value); + }); } waitForEnabledAndFill(node: Node, value: string): InjectedScriptPoll { diff --git a/test/page-fill.spec.ts b/test/page-fill.spec.ts index cbb3d4fb32..1f83ed9e37 100644 --- a/test/page-fill.spec.ts +++ b/test/page-fill.spec.ts @@ -288,3 +288,14 @@ it('should be able to clear', async ({page, server}) => { await page.fill('input', ''); expect(await page.evaluate(() => window['result'])).toBe(''); }); + +it('should not throw when fill causes navigation', async ({page, server}) => { + await page.goto(server.PREFIX + '/input/textarea.html'); + await page.setContent(''); + await page.$eval('input', select => select.addEventListener('input', () => window.location.href = '/empty.html')); + await Promise.all([ + page.fill('input', '2020-03-02'), + page.waitForNavigation(), + ]); + expect(page.url()).toContain('empty.html'); +}); diff --git a/test/page-select-option.spec.ts b/test/page-select-option.spec.ts index c3587c3c58..91cc66c23c 100644 --- a/test/page-select-option.spec.ts +++ b/test/page-select-option.spec.ts @@ -17,6 +17,12 @@ import { it, expect } from './fixtures'; + +async function giveItAChanceToResolve(page) { + for (let i = 0; i < 5; i++) + await page.evaluate(() => new Promise(f => requestAnimationFrame(() => requestAnimationFrame(f)))); +} + it('should select single option', async ({page, server}) => { await page.goto(server.PREFIX + '/input/select.html'); await page.selectOption('select', 'blue'); @@ -61,7 +67,12 @@ it('should select single option by multiple attributes', async ({page, server}) it('should not select single option when some attributes do not match', async ({page, server}) => { await page.goto(server.PREFIX + '/input/select.html'); - await page.selectOption('select', { value: 'green', label: 'Brown' }); + await page.$eval('select', s => s.value = undefined); + try { + await page.selectOption('select', { value: 'green', label: 'Brown' }, {timeout: 300}); + } catch (e) { + expect(e.message).toContain('Timeout'); + } expect(await page.evaluate(() => document.querySelector('select').value)).toEqual(''); }); @@ -134,7 +145,7 @@ it('should throw when element is not a ` element * matching `selector`, the method throws an error. * + * Will wait until all specified options are present in the `` element * matching `selector`, the method throws an error. * + * Will wait until all specified options are present in the `` * element, the method throws an error. * + * Will wait until all specified options are present in the `