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

Added select method to Page Class #779

Merged
merged 7 commits into from Sep 25, 2017

Conversation

Projects
None yet
4 participants
@alixaxel
Contributor

alixaxel commented Sep 14, 2017

This PR adds the Page.select method to interact with select elements on the page.

The accompanying tests and docs demonstrate the basic usage.

alixaxel added some commits Sep 14, 2017

docs/api.md Outdated
@@ -720,6 +721,18 @@ Shortcut for [`keyboard.down`](#keyboarddownkey-options) and [`keyboard.up`](#ke
- `omitBackground` <[boolean]> Hides default white background and allows capturing screenshots with transparency. Defaults to `false`.
- returns: <[Promise]<[Buffer]>> Promise which resolves to buffer with captured screenshot
#### page.select(selector, value)
- `selector` <[string]> A [selector] to query page for
- `value` <[string|string[]]> A string, or array strings in case of `multiple`, of options to select. If the provided value cannot be matched to the `option` value, it tries to match against the `option` label instead.

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Sep 14, 2017

Contributor

It seems this should be <[string]>|<[Array]<[string]>> according to args format here.

This comment has been minimized.

@alixaxel

alixaxel Sep 14, 2017

Contributor

@vsemozhetbyt I wasn't sure and the doc linter didn't complain so I thought was okay. I'll push a fix, thanks.

@aslushnikov

This comment has been minimized.

Contributor

aslushnikov commented Sep 14, 2017

Hey @alixaxel, thank you for the thorough PR.

It looks like the API is similar to nightmare's page.select, but it's hard to tell since the doc is not elaborate enough. Is this the case?

@alixaxel

This comment has been minimized.

Contributor

alixaxel commented Sep 14, 2017

Hi @aslushnikov! Nightmare's page.select only covers the most trivial selection scenario. My implementation is actually highly based on GhostJS's fill method, with a few changes, namely:

  • more reliable selector fallback for elements with multiple property
  • checking for standard label attribute of <option> (according to MDN)
lib/Page.js Outdated
await this.evaluate((element, value) => {
if (element.nodeName.toLowerCase() !== 'select')
return false;

This comment has been minimized.

@aslushnikov

aslushnikov Sep 14, 2017

Contributor

On line 702 you throw if there's no element matching selector.
In order to be consistent, you should also throw if there's an element that is not a <select>.

lib/Page.js Outdated
element.value = value[0];
if (element.value !== value[0])
[].some.call(element.options, option => option.selected = value[0] === option.label);

This comment has been minimized.

@aslushnikov

aslushnikov Sep 14, 2017

Contributor

let's convert options to array once to avoid the Array.prototype.call workaround:

const options = Array.from(element.options);
// ...
lib/Page.js Outdated
return false;
if (Array.isArray(value) !== true)
value = [value];

This comment has been minimized.

@aslushnikov

aslushnikov Sep 14, 2017

Contributor

nit: the value is always an array; let's call it values to make it clear

This comment has been minimized.

@alixaxel

alixaxel Sep 14, 2017

Contributor

This was how I first wrote, but since it can also be provided to the method as a string, I thought value was more appropriate in either case - but I'll change it. 👍

expect(await page.evaluate(() => result)).toBe('brown');
}));
it('should select multiple options', SX(async function() {

This comment has been minimized.

@aslushnikov

aslushnikov Sep 14, 2017

Contributor

how is this selecting multiple options?

This comment has been minimized.

@alixaxel

alixaxel Sep 14, 2017

Contributor

Bad wording, will clarify.

docs/api.md Outdated
@@ -720,6 +721,18 @@ Shortcut for [`keyboard.down`](#keyboarddownkey-options) and [`keyboard.up`](#ke
- `omitBackground` <[boolean]> Hides default white background and allows capturing screenshots with transparency. Defaults to `false`.
- returns: <[Promise]<[Buffer]>> Promise which resolves to buffer with captured screenshot
#### page.select(selector, value)
- `selector` <[string]> A [selector] to query page for
- `value` <[string]|[Array]<[string]>> A string, or array strings in case of `multiple`, of options to select. If the provided value cannot be matched to the `option` value, it tries to match against the `option` label instead.

This comment has been minimized.

@aslushnikov

aslushnikov Sep 14, 2017

Contributor

we usually do varargs to support both array and a single value. Check out ElementHandle.uploadFile

This comment has been minimized.

@aslushnikov

aslushnikov Sep 14, 2017

Contributor

Regarding the matching against value / label, is this an innovation or it's already implemented in some other automation library?

This comment has been minimized.

@alixaxel

alixaxel Sep 14, 2017

Contributor

Regarding the matching against value / label, first time I saw it was on CasperJS.
GhostJS also borrows the idea.

describe('Page.select', function() {
it('should select single option', SX(async function() {
await page.goto(PREFIX + '/input/select.html');
await page.select('select', '');

This comment has been minimized.

@aslushnikov

aslushnikov Sep 14, 2017

Contributor

what does empty string mean as an argument?

This comment has been minimized.

@alixaxel

alixaxel Sep 14, 2017

Contributor

A non-existent option, could be anything (that's not on the options). Will clarify.

lib/Page.js Outdated
});
}
['change', 'input'].forEach(trigger => {

This comment has been minimized.

@aslushnikov

aslushnikov Sep 14, 2017

Contributor

nit: let's explicitly emit two events

element.dispatchEvent(new Event('change'));
element.dispatchEvent(new Event('input'));
@alixaxel

This comment has been minimized.

Contributor

alixaxel commented Sep 15, 2017

Hey @aslushnikov, I pushed the fixes you requested - should I make any other changes?

@aslushnikov

@alixaxel thank you for working on this and sorry for the long reply! I'm traveling and will be slow for a while.

lib/Page.js Outdated
await this.evaluate((element, values) => {
if (element.nodeName.toLowerCase() !== 'select')
throw new Error('Element is not a <select> element.');

This comment has been minimized.

@aslushnikov

aslushnikov Sep 18, 2017

Contributor

in this case, the element handle will never be disposed (since the handle.dispose won't be reached).

If you use the this.$eval, this should be handled just fine. (and if it doesn't, then we should fix page.$eval)

This comment has been minimized.

@alixaxel

alixaxel Sep 18, 2017

Contributor

Good to know, I'll fix that.

lib/Page.js Outdated
const options = Array.from(element.options);
if (Array.isArray(values) !== true)

This comment has been minimized.

@aslushnikov

aslushnikov Sep 18, 2017

Contributor

this if could be removed - values are always an array now since you switched to varargs

lib/Page.js Outdated
if (Array.isArray(values) !== true)
values = [values];
if (element.multiple !== true) {

This comment has been minimized.

@aslushnikov

aslushnikov Sep 18, 2017

Contributor

nit: could you please switch this branch with the following one. I mean:

if (element.multiple) {
  // ...
} else {
}

it's easier to read assertions rather then negations.

lib/Page.js Outdated
if (element.value !== values[0])
options.some(option => option.selected = values[0] === option.label);
} else {
options.forEach(option => {

This comment has been minimized.

@aslushnikov

aslushnikov Sep 18, 2017

Contributor

nit: let's use for-of

for (let option of options)
  option.selected = values.includes(option.value) || values.includes(option.label);
let select = document.querySelector('select');
select.addEventListener('input', () => {

This comment has been minimized.

@aslushnikov

aslushnikov Sep 18, 2017

Contributor

can we also cover the 'change' event? And since the asset is getting heavier, can we reuse single HTML asset to test both multiple and single selection? The select.html can always return an array, and the multiple attribute could be set from-inside the test:

await page.goto(PREFIX + '/input/select.html');
await page.$eval('select', select => select.setAttribute('multiple', true));

This comment has been minimized.

@alixaxel

alixaxel Sep 18, 2017

Contributor

Aren't we mixing fixtures with the tests doing it this way? What guarantees do we have that select.setAttribute('multiple', true) will actually yield the expected result?

lib/Page.js Outdated
if (element.multiple !== true) {
element.value = values[0];
if (element.value !== values[0])

This comment has been minimized.

@aslushnikov

aslushnikov Sep 18, 2017

Contributor

I'm concerned about falling back to the value of label. Depending on the combination of value and label attributes on every option, the matching will happen against either value, label or node's text content. The outcome is hard for me to predict.

Can we start with matching against values only? When/if there's a strong need to match against labels, I'd prefer it to be stated explicitly with options object.

This comment has been minimized.

@alixaxel

alixaxel Sep 18, 2017

Contributor

@aslushnikov For me this is a important feature, in fact for my specific use case I would specialize it even more. Often times, the option values have little resemblance to the label(s) you're targeting.

Nightmare.js has a convenient action method that can be used to implement custom behaviors in a global browser scope without repeating code and, AFAIK, puppeteer only has exposeFunction on a page scope.

Your options suggestion would still be limiting, what are your thoughts on having a page.select(selector, callback) signature, where the callback is a resolver function that gets the option value, label, textContent, index as arguments and has to return true to have that option selected, false otherwise?

A few scenarios I'm facing that could benefit from this (maybe applies to others too), e.g.:

<select id="country">
	<option value="250">France</option>
	<option value="276">Deutschland</option>
	<option value="620">Portugal</option>
	<option value="724">España</option>
	<option value="840">United States of America</option>
</select>

You could select based on value directly:

page.select('#country', '724');

Or implement advanced "resolvers", for instance ISO 3166-1 numeric to Alpha 2:

page.select('#country', 'DE', (target, value, label, options) => {
  let resolved = null;

  switch (value) {
    case 250:
      resolved = 'FR';
    break;

    case 276:
      resolved = 'DE';
    break;

    case 620:
      resolved = 'PT';
    break;

    case 724:
      resolved = 'ES';
    break;

    case 840:
      resolved = 'US';
    break;
  }

  return target === resolved;
});

Or even go fuzzy if needed be:

page.select('#country', 'Spain', (target, value, label, options) => {
  /**
   * Get all `options`, and find the one with the lowest
   * string distance from either the `value` or the `label`.
   *
   * In this case, "Spain" would be resolved to the value of "España".
   */
});

Reading other issues, this could also be helpful to resolve implementation inconsistencies between in libraries and frameworks (ReactJS Virtual DOM was mentioned for instance).

This comment has been minimized.

@aslushnikov

aslushnikov Sep 18, 2017

Contributor

Nightmare.js has a convenient action method that can be used to implement custom behaviors in a global browser scope without repeating code and, AFAIK, puppeteer only has exposeFunction on a page scope.

If I understand Nightmare's action correctly, the analogue in puppeteer would be adding new methods to the page prototype:

Page.prototype.size = function(done) {
  return this.evaluate(() => {
    const w = Math.max(document.documentElement.clientWidth, window.innerWidth || 0)
    const h = Math.max(document.documentElement.clientHeight, window.innerHeight || 0)
    return {
      height: h,
      width: w
    }
  });
})

It looks like it has nothing to do with page.exposeFunction.

what are your thoughts on having a page.select(selector, callback) signature, where the callback is a resolver function that gets the option value, label, textContent, index as arguments and has to return true to have that option selected, false otherwise?

I think it's not a simple API.

If you can select by values, then you can implement custom resolver with page.evaluate.
For example, selecting all options that contain letter 'e' in their label:

const values = await page.evaluate(() => {
  const options = Array.from(document.querySelectorAll('select > option'));
  const optionsWithE = options.filter(option => option.label.includes('e'));
  return optionsWithE.map(option => option.value);
  });
});
await page.select('select', ...values);

This comment has been minimized.

@alixaxel

alixaxel Sep 23, 2017

Contributor

@aslushnikov Extending the Page prototype would actually be super useful to me!

But I'm not sure how to go about accessing the Page class, it appears that it's not exported?

This comment has been minimized.

@aslushnikov

aslushnikov Sep 25, 2017

Contributor

@alixaxel ah right. You can "hack" it with a manual require:

const Page = require('puppeteer/lib/Page');

This is not safe of course, but this magic require should not change much over the time.

lib/Page.js Outdated
}
element.dispatchEvent(new Event('change'));
element.dispatchEvent(new Event('input'));

This comment has been minimized.

@aslushnikov

aslushnikov Sep 18, 2017

Contributor

do we need to dispatch 'input'? Nightmare gets away without it

This comment has been minimized.

@alixaxel

alixaxel Sep 18, 2017

Contributor

I think so, even listeners could only be listening to input, and I think those are also the events that the browser sends on a typical mouse-selection scenario.

This comment has been minimized.

@Garbee

Garbee Sep 18, 2017

Contributor

input is fired on select changes. So it should be fired to best reflect what browsers do.

This comment has been minimized.

@alixaxel

alixaxel Sep 23, 2017

Contributor

I'll keep dispatching change and input events, since that's what browsers do.

lib/Page.js Outdated
console.assert(handle, 'No node found for selector: ' + selector);
await this.evaluate((element, values) => {
if (element.nodeName.toLowerCase() !== 'select')

This comment has been minimized.

@Garbee

Garbee Sep 18, 2017

Contributor

Why call toLowerCase here? Checking against 'SELECT' is just fine and saves extra work from happening.

This comment has been minimized.

@alixaxel

alixaxel Sep 23, 2017

Contributor

@Garbee According to https://johnresig.com/blog/nodename-case-sensitivity/:

  1. The node names of HTML elements are always uppercase, even if they’re explicitly created using lowercase characters. will result in a .nodeName === "HTML" (see the HTML 5 draft).
  2. The node names of XML elements are always in the original case, as specified when they’re created. will result in a .nodeName === "data", will result in a .nodeName === "DATA".

I think it's not a big thing to leave toLowerCase just for the sake of reliability.

lib/Page.js Outdated
}
element.dispatchEvent(new Event('change'));
element.dispatchEvent(new Event('input'));

This comment has been minimized.

@Garbee

Garbee Sep 18, 2017

Contributor

input is fired on select changes. So it should be fired to best reflect what browsers do.

@alixaxel

This comment has been minimized.

Contributor

alixaxel commented Sep 23, 2017

@aslushnikov Sorry for the delay in pushing all the requested changes, but I've had a very busy week. 💯

Please let me know if anything else is needed.

@aslushnikov

Thank you for the CL - this is great!
There's one typo in docs and we can have this merged.

docs/api.md Outdated
```js
page.select('select#colors', 'blue'); // single selection
page.select('select#colors', ['red', 'green', 'blue']); // multiple selections

This comment has been minimized.

@aslushnikov

aslushnikov Sep 25, 2017

Contributor

no need for the array:

page.select('select#colors', 'red', 'green', 'blue');
@alixaxel

This comment has been minimized.

Contributor

alixaxel commented Sep 25, 2017

@aslushnikov Fixed typo. :)

@aslushnikov aslushnikov merged commit 45f2640 into GoogleChrome:master Sep 25, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment