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

feat(elementHandle): add boxModel method #2256

Merged
merged 8 commits into from Mar 29, 2018

Conversation

yanivefraim
Copy link
Contributor

Fixes #1357 - expose element's boxModel.

I wasn't sure how exactly do we want to represent boxmodel data, so I just represented it "as is": an object containing quads, exactly as it is represented in chromedevtools

docs/api.md Outdated
@@ -2191,6 +2192,17 @@ The method evaluates the XPath expression relative to the elementHandle. If ther

This method returns the bounding box of the element (relative to the main frame), or `null` if the element is not visible.

#### elementHandle.boxModel()
- returns: <[Promise]<?[Object]>>
- content <[Array]<[number]>> Content box, represented as a [quad](https://chromedevtools.github.io/devtools-protocol/tot/DOM#type-Quad).
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have an array of objects with{x, y} - that will be a more friendly format to work with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const elementHandle = await page.$('div');
const box = await elementHandle.boxModel();

expect(box.content).toEqual([10, 10, 110, 10, 110, 110, 10, 110]);
Copy link
Contributor

Choose a reason for hiding this comment

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

In what coordinates are the returned numbers? Can we have a test with nested frames to make the coordinate system clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, 10x!

async boxModel() {
const result = await this._getBoxModel();

if (!result)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: to my taste, i'd replace lines 164-168 with a one-liner:

return result ? result.model : null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not relevant after my last change (moving to {x, y} representation)

if (!result)
return null;

return Object.keys(result.model).reduce((prev, key) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure this works, but I'm having hard times following the logic.
I'd rather have something more straight-forward here that outlines the shape of the returned object.

This will make it clear what we return (and also improves code grep'pability!)

return {
  content: fromProtocolQuad(result.content),
  padding: fromProtocolQuad(result.padding),
  // ...
}

function fromProtocolQuad(protocolQuad) {
  // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, 10x!

@yanivefraim
Copy link
Contributor Author

@aslushnikov - fixed everything. 10x!

Copy link
Contributor

@aslushnikov aslushnikov left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -52,6 +52,33 @@ module.exports.addTests = function({testRunner, expect}) {
});
});

describe('ElementHandle.boxModel', function() {
it('should work', async({page, server}) => {
const leftTop = {x: 28, y: 260};
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't validate these numbers from just looking at the test. However, I played with the API and the functionality works as expected.

Let's land this as-is, I'll re-write the test in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aslushnikov - agree. Maybe I can add back the previous test (in addition to this one). It had a div with specific dimensions (so you can actually tell that the sizes are good. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yanivefraim no worries, I came up with something I've just uploaded while playing with your patch. See #2287

@aslushnikov aslushnikov merged commit 41d5838 into puppeteer:master Mar 29, 2018
@schelkun
Copy link
Contributor

Nice! Thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants