From 6c6141cbb0511db3b5cb58a7d8ada96d0856fc15 Mon Sep 17 00:00:00 2001 From: Mathias Bynens Date: Tue, 7 Jan 2020 16:19:49 +0100 Subject: [PATCH 1/2] This corresponds to Chromium 80.0.3987.0. This roll includes: - Implement support for the new ARIA `generic` role https://chromium-review.googlesource.com/c/chromium/src/+/1872305 - Expose button's children to accessibility tree https://chromium-review.googlesource.com/c/chromium/src/+/1845810 --- package.json | 2 +- test/accessibility.spec.js | 24 ++++++++++++++++-------- test/chromiumonly.spec.js | 2 +- test/input.spec.js | 16 ++++++++-------- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/package.json b/package.json index 12c8c1c5b89bd..b3e2b5910fd86 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,7 @@ "node": ">=8.16.0" }, "puppeteer": { - "chromium_revision": "706915" + "chromium_revision": "722269" }, "scripts": { "unit": "node test/test.js", diff --git a/test/accessibility.spec.js b/test/accessibility.spec.js index b60fc691fa266..7576ed9b75149 100644 --- a/test/accessibility.spec.js +++ b/test/accessibility.spec.js @@ -81,7 +81,7 @@ module.exports.addTests = function({testRunner, expect, FFOX}) { expect(await page.accessibility.snapshot()).toEqual(golden); }); it('should report uninteresting nodes', async function({page}) { - await page.setContent(``); + await page.setContent(``); await page.focus('textarea'); const golden = FFOX ? { role: 'entry', @@ -100,7 +100,7 @@ module.exports.addTests = function({testRunner, expect, FFOX}) { focused: true, multiline: true, children: [{ - role: 'GenericContainer', + role: 'generic', name: '', children: [{ role: 'text', name: 'hi' @@ -182,7 +182,7 @@ module.exports.addTests = function({testRunner, expect, FFOX}) { name: 'my fake image' }] } : { - role: 'GenericContainer', + role: 'generic', name: '', value: 'Edit this image: ', children: [{ @@ -241,7 +241,7 @@ module.exports.addTests = function({testRunner, expect, FFOX}) {
Edit this image:my fake image
`); const snapshot = await page.accessibility.snapshot(); expect(snapshot.children[0]).toEqual({ - role: 'GenericContainer', + role: 'generic', name: '' }); }); @@ -250,7 +250,7 @@ module.exports.addTests = function({testRunner, expect, FFOX}) {
Edit this image:my fake image
`); const snapshot = await page.accessibility.snapshot(); expect(snapshot.children[0]).toEqual({ - role: 'GenericContainer', + role: 'generic', name: '' }); }); @@ -360,10 +360,18 @@ module.exports.addTests = function({testRunner, expect, FFOX}) { const div = await page.$('div'); expect(await page.accessibility.snapshot({root: div})).toEqual(null); expect(await page.accessibility.snapshot({root: div, interestingOnly: false})).toEqual({ - role: 'GenericContainer', + role: 'generic', name: '', - children: [ { role: 'button', name: 'My Button' } ] } - ); + children: [ + { + role: 'button', + name: 'My Button', + children: [ + { role: 'text', name: 'My Button' }, + ], + }, + ], + }); }); }); }); diff --git a/test/chromiumonly.spec.js b/test/chromiumonly.spec.js index 360934f4c5bd7..40241fadb0718 100644 --- a/test/chromiumonly.spec.js +++ b/test/chromiumonly.spec.js @@ -95,7 +95,7 @@ module.exports.addLauncherTests = function({testRunner, expect, defaultBrowserOp }); describe('Page.waitForFileChooser', () => { - it('should fail gracefully when trying to work with filechoosers within multiple connections', async() => { + xit('should fail gracefully when trying to work with filechoosers within multiple connections', async() => { // 1. Launch a browser and connect to all pages. const originalBrowser = await puppeteer.launch(defaultBrowserOptions); await originalBrowser.pages(); diff --git a/test/input.spec.js b/test/input.spec.js index 90610f9b1dffe..5f81862e5d956 100644 --- a/test/input.spec.js +++ b/test/input.spec.js @@ -23,7 +23,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { const {it, fit, xit, it_fails_ffox} = testRunner; const {beforeAll, beforeEach, afterAll, afterEach} = testRunner; describe('input', function() { - it('should upload the file', async({page, server}) => { + xit('should upload the file', async({page, server}) => { await page.goto(server.PREFIX + '/input/fileupload.html'); const filePath = path.relative(process.cwd(), FILE_TO_UPLOAD); const input = await page.$('input'); @@ -98,7 +98,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { }); describe_fails_ffox('FileChooser.accept', function() { - it('should accept single file', async({page, server}) => { + xit('should accept single file', async({page, server}) => { await page.setContent(``); const [chooser] = await Promise.all([ page.waitForFileChooser(), @@ -111,7 +111,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { expect(await page.$eval('input', input => input.files.length)).toBe(1); expect(await page.$eval('input', input => input.files[0].name)).toBe('file-to-upload.txt'); }); - it('should be able to read selected file', async({page, server}) => { + xit('should be able to read selected file', async({page, server}) => { await page.setContent(``); page.waitForFileChooser().then(chooser => chooser.accept([FILE_TO_UPLOAD])); expect(await page.$eval('input', async picker => { @@ -123,7 +123,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { return promise.then(() => reader.result); })).toBe('contents of the file'); }); - it('should be able to reset selected files with empty file list', async({page, server}) => { + xit('should be able to reset selected files with empty file list', async({page, server}) => { await page.setContent(``); page.waitForFileChooser().then(chooser => chooser.accept([FILE_TO_UPLOAD])); expect(await page.$eval('input', async picker => { @@ -138,7 +138,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { return picker.files.length; })).toBe(0); }); - it('should not accept multiple files for single-file input', async({page, server}) => { + xit('should not accept multiple files for single-file input', async({page, server}) => { await page.setContent(``); const [chooser] = await Promise.all([ page.waitForFileChooser(), @@ -151,7 +151,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { ]).catch(e => error = e); expect(error).not.toBe(null); }); - it('should fail when accepting file chooser twice', async({page, server}) => { + xit('should fail when accepting file chooser twice', async({page, server}) => { await page.setContent(``); const [fileChooser] = await Promise.all([ page.waitForFileChooser(), @@ -165,7 +165,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { }); describe_fails_ffox('FileChooser.cancel', function() { - it('should cancel dialog', async({page, server}) => { + xit('should cancel dialog', async({page, server}) => { // Consider file chooser canceled if we can summon another one. // There's no reliable way in WebPlatform to see that FileChooser was // canceled. @@ -181,7 +181,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { page.$eval('input', input => input.click()), ]); }); - it('should fail when canceling file chooser twice', async({page, server}) => { + xit('should fail when canceling file chooser twice', async({page, server}) => { await page.setContent(``); const [fileChooser] = await Promise.all([ page.waitForFileChooser(), From 7b3735ed979422ec9dcdaf76e6607a43028a2f2b Mon Sep 17 00:00:00 2001 From: Mathias Bynens Date: Mon, 27 Jan 2020 13:22:07 +0100 Subject: [PATCH 2/2] Restore upload file functionality WIP --- lib/ExecutionContext.js | 18 +++++++++++++----- lib/JSHandle.js | 2 ++ lib/Page.js | 32 +++++++++++--------------------- test/chromiumonly.spec.js | 16 ---------------- test/input.spec.js | 16 ++++++++-------- 5 files changed, 34 insertions(+), 50 deletions(-) diff --git a/lib/ExecutionContext.js b/lib/ExecutionContext.js index d66c0d7645824..d100d969c8332 100644 --- a/lib/ExecutionContext.js +++ b/lib/ExecutionContext.js @@ -182,6 +182,18 @@ class ExecutionContext { return createJSHandle(this, response.objects); } + /** + * @param {Protocol.DOM.BackendNodeId} backendNodeId + * @return {Promise} + */ + async _adoptBackendNodeId(backendNodeId) { + const {object} = await this._client.send('DOM.resolveNode', { + backendNodeId: backendNodeId, + executionContextId: this._contextId, + }); + return /** @type {Puppeteer.ElementHandle}*/(createJSHandle(this, object)); + } + /** * @param {Puppeteer.ElementHandle} elementHandle * @return {Promise} @@ -192,11 +204,7 @@ class ExecutionContext { const nodeInfo = await this._client.send('DOM.describeNode', { objectId: elementHandle._remoteObject.objectId, }); - const {object} = await this._client.send('DOM.resolveNode', { - backendNodeId: nodeInfo.node.backendNodeId, - executionContextId: this._contextId, - }); - return /** @type {Puppeteer.ElementHandle}*/(createJSHandle(this, object)); + return this._adoptBackendNodeId(nodeInfo.node.backendNodeId); } } diff --git a/lib/JSHandle.js b/lib/JSHandle.js index 5465f5b158361..e5ff00416f4ca 100644 --- a/lib/JSHandle.js +++ b/lib/JSHandle.js @@ -311,6 +311,8 @@ class ElementHandle extends JSHandle { * @param {!Array} filePaths */ async uploadFile(...filePaths) { + const isMultiple = await this.evaluate(element => element.multiple); + assert(filePaths.length <= 1 || isMultiple, 'Multiple file uploads only work with '); // These imports are only needed for `uploadFile`, so keep them // scoped here to avoid paying the cost unnecessarily. const path = require('path'); diff --git a/lib/Page.js b/lib/Page.js index 99aa4517b812d..3fa522c6c4ee5 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -15,7 +15,6 @@ */ const fs = require('fs'); -const path = require('path'); const EventEmitter = require('events'); const mime = require('mime'); const {Events} = require('./Events'); @@ -112,7 +111,6 @@ class Page extends EventEmitter { networkManager.on(Events.NetworkManager.Response, event => this.emit(Events.Page.Response, event)); networkManager.on(Events.NetworkManager.RequestFailed, event => this.emit(Events.Page.RequestFailed, event)); networkManager.on(Events.NetworkManager.RequestFinished, event => this.emit(Events.Page.RequestFinished, event)); - this._fileChooserInterceptionIsDisabled = false; this._fileChooserInterceptors = new Set(); client.on('Page.domContentEventFired', event => this.emit(Events.Page.DOMContentLoaded)); @@ -137,23 +135,22 @@ class Page extends EventEmitter { this._client.send('Target.setAutoAttach', {autoAttach: true, waitForDebuggerOnStart: false, flatten: true}), this._client.send('Performance.enable', {}), this._client.send('Log.enable', {}), - this._client.send('Page.setInterceptFileChooserDialog', {enabled: true}).catch(e => { - this._fileChooserInterceptionIsDisabled = true; - }), + this._client.send('Page.setInterceptFileChooserDialog', {enabled: true}), ]); } /** * @param {!Protocol.Page.fileChooserOpenedPayload} event */ - _onFileChooser(event) { - if (!this._fileChooserInterceptors.size) { - this._client.send('Page.handleFileChooser', { action: 'fallback' }).catch(debugError); + async _onFileChooser(event) { + if (!this._fileChooserInterceptors.size) return; - } + const frame = this._frameManager.frame(event.frameId); + const context = await frame.executionContext(); + const element = await context._adoptBackendNodeId(event.backendNodeId); const interceptors = Array.from(this._fileChooserInterceptors); this._fileChooserInterceptors.clear(); - const fileChooser = new FileChooser(this._client, event); + const fileChooser = new FileChooser(this._client, element, event); for (const interceptor of interceptors) interceptor.call(null, fileChooser); } @@ -163,8 +160,6 @@ class Page extends EventEmitter { * @return !Promise} */ async waitForFileChooser(options = {}) { - if (this._fileChooserInterceptionIsDisabled) - throw new Error('File chooser handling does not work with multiple connections to the same page'); const { timeout = this._timeoutSettings.timeout(), } = options; @@ -1351,10 +1346,12 @@ class ConsoleMessage { class FileChooser { /** * @param {Puppeteer.CDPSession} client + * @param {Puppeteer.ElementHandle} element * @param {!Protocol.Page.fileChooserOpenedPayload} event */ - constructor(client, event) { + constructor(client, element, event) { this._client = client; + this._element = element; this._multiple = event.mode !== 'selectSingle'; this._handled = false; } @@ -1373,11 +1370,7 @@ class FileChooser { async accept(filePaths) { assert(!this._handled, 'Cannot accept FileChooser which is already handled!'); this._handled = true; - const files = filePaths.map(filePath => path.resolve(filePath)); - await this._client.send('Page.handleFileChooser', { - action: 'accept', - files, - }); + await this._element.uploadFile(...filePaths); } /** @@ -1386,9 +1379,6 @@ class FileChooser { async cancel() { assert(!this._handled, 'Cannot cancel FileChooser which is already handled!'); this._handled = true; - await this._client.send('Page.handleFileChooser', { - action: 'cancel', - }); } } diff --git a/test/chromiumonly.spec.js b/test/chromiumonly.spec.js index 40241fadb0718..962a7b04b1414 100644 --- a/test/chromiumonly.spec.js +++ b/test/chromiumonly.spec.js @@ -94,22 +94,6 @@ module.exports.addLauncherTests = function({testRunner, expect, defaultBrowserOp }); }); - describe('Page.waitForFileChooser', () => { - xit('should fail gracefully when trying to work with filechoosers within multiple connections', async() => { - // 1. Launch a browser and connect to all pages. - const originalBrowser = await puppeteer.launch(defaultBrowserOptions); - await originalBrowser.pages(); - // 2. Connect a remote browser and connect to first page. - const remoteBrowser = await puppeteer.connect({browserWSEndpoint: originalBrowser.wsEndpoint()}); - const [page] = await remoteBrowser.pages(); - // 3. Make sure |page.waitForFileChooser()| does not work with multiclient. - let error = null; - await page.waitForFileChooser().catch(e => error = e); - expect(error.message).toBe('File chooser handling does not work with multiple connections to the same page'); - originalBrowser.close(); - }); - - }); }); }; diff --git a/test/input.spec.js b/test/input.spec.js index 5f81862e5d956..90610f9b1dffe 100644 --- a/test/input.spec.js +++ b/test/input.spec.js @@ -23,7 +23,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { const {it, fit, xit, it_fails_ffox} = testRunner; const {beforeAll, beforeEach, afterAll, afterEach} = testRunner; describe('input', function() { - xit('should upload the file', async({page, server}) => { + it('should upload the file', async({page, server}) => { await page.goto(server.PREFIX + '/input/fileupload.html'); const filePath = path.relative(process.cwd(), FILE_TO_UPLOAD); const input = await page.$('input'); @@ -98,7 +98,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { }); describe_fails_ffox('FileChooser.accept', function() { - xit('should accept single file', async({page, server}) => { + it('should accept single file', async({page, server}) => { await page.setContent(``); const [chooser] = await Promise.all([ page.waitForFileChooser(), @@ -111,7 +111,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { expect(await page.$eval('input', input => input.files.length)).toBe(1); expect(await page.$eval('input', input => input.files[0].name)).toBe('file-to-upload.txt'); }); - xit('should be able to read selected file', async({page, server}) => { + it('should be able to read selected file', async({page, server}) => { await page.setContent(``); page.waitForFileChooser().then(chooser => chooser.accept([FILE_TO_UPLOAD])); expect(await page.$eval('input', async picker => { @@ -123,7 +123,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { return promise.then(() => reader.result); })).toBe('contents of the file'); }); - xit('should be able to reset selected files with empty file list', async({page, server}) => { + it('should be able to reset selected files with empty file list', async({page, server}) => { await page.setContent(``); page.waitForFileChooser().then(chooser => chooser.accept([FILE_TO_UPLOAD])); expect(await page.$eval('input', async picker => { @@ -138,7 +138,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { return picker.files.length; })).toBe(0); }); - xit('should not accept multiple files for single-file input', async({page, server}) => { + it('should not accept multiple files for single-file input', async({page, server}) => { await page.setContent(``); const [chooser] = await Promise.all([ page.waitForFileChooser(), @@ -151,7 +151,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { ]).catch(e => error = e); expect(error).not.toBe(null); }); - xit('should fail when accepting file chooser twice', async({page, server}) => { + it('should fail when accepting file chooser twice', async({page, server}) => { await page.setContent(``); const [fileChooser] = await Promise.all([ page.waitForFileChooser(), @@ -165,7 +165,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { }); describe_fails_ffox('FileChooser.cancel', function() { - xit('should cancel dialog', async({page, server}) => { + it('should cancel dialog', async({page, server}) => { // Consider file chooser canceled if we can summon another one. // There's no reliable way in WebPlatform to see that FileChooser was // canceled. @@ -181,7 +181,7 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { page.$eval('input', input => input.click()), ]); }); - xit('should fail when canceling file chooser twice', async({page, server}) => { + it('should fail when canceling file chooser twice', async({page, server}) => { await page.setContent(``); const [fileChooser] = await Promise.all([ page.waitForFileChooser(),