Add get bounding box to ElementHandle for easier element screenshot #445

elisherer · 2017-08-21T13:36:50Z

~~This will add the ability to capture a screenshot based on a selector (as opposed to using the clip object).~~
~~The implementation is on top of the clip option to make things simple:~~

~~await page.screenshot({~~
~~clip: '.box:nth-of-type(43)'~~
~~});~~

*The example above is from the unit test (see output image below in the affected files).

EDIT: Changed to adding a method: elementHandle.boundingBox() which will return the rectangle needed for screenshot's clip option.

… clip option)

ebidel · 2017-08-21T18:02:14Z

FWIW, I posted a pr that shows how to do this in user-code. It's just a few lines but gets even smaller after #382 lands.

That said, this might be common enough that it's something we should support directly in the API.

aslushnikov · 2017-08-21T18:49:21Z

That's nice. Let's make elementHandle.screenshot(options) method instead to keep Page lean.

elisherer · 2017-08-21T21:13:12Z

The challenge is that all of the screenshot code is written on the Page class (together with a screenshotTaskQueue).
Also, ElementHandle does not have a reference to the creating page (maybe it should?)
Is elementHandle.screenshot(page, options) acceptable?

googlebot · 2017-08-21T21:13:14Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

elisherer · 2017-08-21T21:36:58Z

I have tried to resolve the cla check by adding an alternative email of mine...

googlebot · 2017-08-21T21:37:00Z

CLAs look good, thanks!

aslushnikov · 2017-08-21T23:03:49Z

@elisherer ah, I see. Let's do the follwing:

we'll introduce the elementHandle.ownerFrame() shortly (there's some work upstream to make it possible)
then you'll be able to add a frame.page() and use page.screenshot to drive element screenshots.

elisherer · 2017-08-22T05:05:36Z

@aslushnikov, how about we put the screenshot taking logic into one common place.
I noticed the page is only important for using the fullPage option (usint its viewport), which elements' screenshots don't need.
Also, clipping should not be possible for elements.
So each of the cases have different "predefined" clip providing options.
In addition, the screenshot task queue is stored on the browser and passed to each of the pages. In the same way it could be passed to each element as a handle for screenshot capturing.
WDYT?

aslushnikov · 2017-08-22T06:42:35Z

@elisherer this approach would work. However, I don't like it because it touches quite a bit of code with no good excuse.

I see the elementHandle.screenshot as a convenience method for page.screenshot: the feature doesn't add any new capabilities and It's easy to polyfill with current puppeteer API:

// Polyfill for elementHandle.screenshot
async function screenshotElement(page, selector, options)  {
  const handle = await page.$(selector);
  const box = await getElementCoordinates(handle); // roughly return element's bounding box
  options.clip = box;
  return await page.screenshot(options);
}

Convenience methods should be implemented atop of core, with as little footprint as possible. This will minimize the maintenance cost.

In addition, the screenshot task queue is stored on the browser and passed to each of the pages. In the same way it could be passed to each element as a handle for screenshot capturing.

The screenshotTaskQueue is a shabby detail of screenshot machinery we have today, it's unfortunate to have it spreading further into subsystems.

…an element (unit tests and documentation, with example, were updated)

JoelEinbinder · 2017-08-22T08:17:19Z

test/test.js

+    it('should work', SX(async function() {
+      await page.setViewport({width: 500, height: 500});
+      await page.goto(PREFIX + '/grid.html');
+      const fourtySecondBoxElement = await page.$('.box:nth-of-type(42)');


Nit: fortySecondBoxElement

Needs explaining here... You don't like the long variable name?

JoelEinbinder · 2017-08-22T08:21:11Z

lib/ElementHandle.js

+   * @return {!Object}
+   */
+  async boundingBox() {
+    const boxModel = await this._client.send('DOM.getBoxModel', { objectId: this._remoteObject.objectId });


The bounding box of the element could be partially off-screen. The element might also not be scrolled into view. something to consider when taking screenshots. If you look above at _visibleCenter, we use scrollIntoViewIfNeeded and clip based on the window bounds. Because we are already in JavaScript, we use getBoundingClientRect and avoid introducing DOM.getBoxModel.

I don't expect element.boundingBox() to scroll the element into view, but I can imagine that it would be intended if boundingBox is used to take screenshots.

Would you recommend documenting this consideration?

aslushnikov · 2017-08-22T08:59:26Z

lib/ElementHandle.js

+   * @return {!Object}
+   */
+  async boundingBox() {
+    const boxModel = await this._client.send('DOM.getBoxModel', { objectId: this._remoteObject.objectId });


Values in DOM.getBoxModel are affected by the box-sizing css property. AFAIR they also include page scale (@wwwillchen might remember better).

getBoundingClientRect might work better for this use case.

Would you recommend documenting this consideration?

Once this lands, #452 should work as a good-enough documentation.

aslushnikov · 2017-08-22T17:14:48Z

test/test.js

@@ -1116,6 +1116,16 @@ describe('Page', function() {
    }));
  });

+  describe('ElementHandle.boundingBox', function() {
+    fit('should work', SX(async function() {


let's do it instead of fit.

aslushnikov · 2017-08-22T17:22:57Z

lib/ElementHandle.js

+        height: rect.bottom - rect.top
+      };
+    });
+    if (!box)


let's return nullable box without throwing

aslushnikov · 2017-08-22T17:23:04Z

lib/ElementHandle.js

@@ -73,7 +73,7 @@ class ElementHandle {
      };
    });
    if (!center)
-      throw new Error('No node found for selector: ' + selector);
+      throw new Error('No node found');


Nice catch!

Could you please change this into a more elaborate message while we're here:

throw new Error('Element is detached from DOM');

aslushnikov · 2017-08-22T17:38:27Z

lib/ElementHandle.js

+      return {
+        x: rect.left,
+        y: rect.top,
+        width: rect.right - rect.left,


Is there any reason why you don't use the rect.width and rect.height here?

aslushnikov · 2017-08-22T18:06:34Z

lib/ElementHandle.js

+    const box = await this.evaluate(element => {
+      if (!element.ownerDocument.contains(element))
+        return null;
+      const rect = element.getBoundingClientRect();


This returns bounding box in viewport, whereas screenshot accepts coordinates relative to the main frame. To make this usable with page.screenshot, you need to convert coordinates to the window.

That's how we do this in DevTools front-end: DOMExtension.js

Let's add a test that verifies screenshots with clipping, something along the lines:

describe('Page.screenshot', function() { ... it('should work with elementHandle.boundingBox', SX(async function() { await page.setViewport({width: 500, height: 500}); await page.goto(PREFIX + '/grid.html'); await page.evaluate(() => window.scrollBy(0, 100)); const box = await page.$('div:nth-child(3)'); const screenshot = await page.screenshot({clip: await box.boundingBox()}); expect(screenshot).toBeGolden('screenshot-element-bounding-box.png'); })); ...

I've added the test above, but I ended up using the code from document-offset for getting the absolute offset.

Thanks for adding a test!

but I ended up using the code from document-offset for getting the absolute offset.

In case of nested iframes, this will return the bounding box inside the iframe's document, not the main frame document. (can we have a test for this too?)

Since we are running JavaScript we are are bound to CORS rules, accessing parent frame and getting its position might not be possible (it even happens when ran from the file system).
So the DevTools approach might be better to accomplish this. I'm trying to get this to run:

fit('should handle nested frames', SX(async function() { await page.setViewport({width: 500, height: 500}); await page.goto(PREFIX + '/frames/nested-frames.html'); const nestedFrame = page.frames()[0].childFrames()[1]; const elementHandle = await nestedFrame.$('div'); const box = await elementHandle.boundingBox(); expect(box).toEqual({ x: 28, y: 253, width: 284, height: 18 }); }));

Currently errors with:

Message: Expected $.x = 8 to equal 28. Expected $.y = 8 to equal 253.

That being said, I'm guessing that click and hover won't work with iframes either...

UPDATE: I tried again with DOM.getBoxModel and the content quad returned is the position of the box relative to the main frame. I will update once both tests are running (scroll on main frame and nested in iframe)

aslushnikov · 2017-08-22T18:08:09Z

docs/api.md

+    - width <[number]> the width of the element in pixels.
+    - height <[number]> the height of the element in pixels.
+
+This method returns the bounding box of the element (relative to the main frame).


(relative to the main frame), or null if element is detached from dom.

…ent, add unit test for being used together with screenshot when not visible

…me), unit test added for nested frames.

aslushnikov · 2017-08-23T21:17:32Z

lib/ElementHandle.js

+   */
+  async boundingBox() {
+    const layoutMetrics = await this._client.send('Page.getLayoutMetrics');
+    const boxModel = await this._client.send('DOM.getBoxModel', { objectId: this._remoteObject.objectId });


Let's not use DOM.getBoxModel - it misbehaves with page zoom. It's also an experimental protocol method (and we try to minimize our use of experimental methods so that it would be easier to stabilize them in future).

The logic here is implementable with DOM API, e.g. like this. Do you have any concerns about this approach?

Yes, as you pointed out, we need to consider elements inside iframes and this logic you suggest applies only to the frame containing the element. Using a while loop to get the parent frame for offsets might not work because of CORS policy which makes the method inconsistant.
On the other hand using the DevTools API gives us the ability to inspect the window without browser JavaScript constraints.
So it's a matter of deciding if we want to support nested element bounding boxes or not (or we do but only for same domain frames).

Using a while loop to get the parent frame for offsets might not work because of CORS policy which makes the method inconsistant.

Yes, you're right.

We're thinking about introducing the elementHandle.ownerFrame() method in #433. This would allow you to iterate frame structure avoiding CORS and use bullet-proof element.getBoundingClientRect instead of DOM.getBoxModel.

If this solves your concerns, I'd like to wait for the ownerFrame to land first and then follow-up with this PR.

OK, will do.

paulirish · 2017-09-05T19:46:35Z

@elisherer any update? looks like a few people eager for this.

aslushnikov · 2017-09-05T22:41:03Z

@paulirish this is stuck on my implementation of elementHandle.ownerFrame() method (#433).

# Conflicts: # lib/ElementHandle.js

…x inside the page. Also fixes the visible center used by mouse and touchscreen.

elisherer · 2017-10-02T20:58:21Z

@aslushnikov, It now supports iframes by using upwards traversal using the DOM api (describeNode) and getting the position inside the window using JavaScript.
(I used my own implementation of "ownerFrame", hope it's ok)

elisherer · 2017-10-03T14:08:01Z

why is the cla/google "waiting for status to be reported" is waiting for a couple of days? shouldn't it be instant?

aslushnikov · 2017-10-05T15:14:27Z

@aslushnikov, It now supports iframes by using upwards traversal using the DOM api (describeNode) and getting the position inside the window using JavaScript.

Thanks @elisherer. I've though about this a bit more and doing multiple hops into page to resolve bounding box seems to be fragile - page might've changed during the process. This would be terrible to debug.

I now tend to agree with you on the matter of using DOM.getBoxModel method. I'll look into it shortly to see how it could be fixed.

why is the cla/google "waiting for status to be reported" is waiting for a couple of days? shouldn't it be instant?

No worries, we can ignore the bot. I believe it's not very reliable.

aslushnikov · 2017-10-06T19:27:00Z

@elisherer: @JoelEinbinder looked into that and it turned out that:

getBoxModel is wrong with non-default page's scale
we consider page's scale to be always default in pptr

To sum up: we can use getBoxModel in pptr. Please, go ahead with this patch!
I'm sorry for making you wait for so long.

Also, @JoelEinbinder has a PR to fix clicking in iframes: #971
It would be great if you can base your work atop of that one.

…to feature/screenshot-by-selector # Conflicts: # docs/api.md # lib/ElementHandle.js

elisherer · 2017-10-07T12:20:22Z

@aslushnikov, OK, I'm done bringing back the code of getBoxModel while merging to the new changes in ElementHandle.
I did base my code on @JoelEinbinder PR #971, so it kind of already fixes the problem there with visible center (because I made visible center call boundingBox; no need for 2 separate identical calls).

aslushnikov · 2017-10-07T15:36:46Z

lib/ElementHandle.js

+        return 'Node is detached from document';
+      if (element.nodeType !== HTMLElement.ELEMENT_NODE)
+        return 'Node is not of type HTMLElement';
+      element.scrollIntoViewIfNeeded();


I wouldn't expect element.boundingBox() to cause side effects like scrolling page. Can we drop it from here?

Isn't it the same side effects as keyboard and touchscreen operations (no expecatations there either)?
Maybe it should be optional?
i.e.

element.boundingBox({ scrollIntoView: true })

(it will return a different bounding box if scroll was needed)

Isn't it the same side effects as keyboard and touchscreen operations (no expecatations there either)?

This only happens for mouse/touch operations. The reason is that we can't click to the off-screen location, it is (somewhat) understandable that element should be on screen to be clicked.

Whereas The boundingBox method is a getter, and getters are generally perceived as operations without side-effects.

(it will return a different bounding box if scroll was needed)

Thanks, I see why you need it. I'd make an internal method _innerBoundingBox(scrollIntoView), that would be used in both _visibleCenter and boundingBox():

async _visibleCenter() { const box = this._innerBoundingBox(true /* scrollIntoView */); // .. compute center } async boundingBox() { return this._innerBoundingBox(false /* scrollIntoView */); }

I agree about the above, but how do you suggest solve the problem of "off-frame" objects needing to be captured by screenshot.
I would want the behavior of scrolling before calling boundingBox().
element.scrollIntoViewIfNeeded() like in focus()?

… a new method of element handle, fix executionContext jsdoc

elisherer · 2017-10-09T05:03:55Z

@aslushnikov , as my last commit states, I added a scrollIntoViewIfNeeded method and used is as part of the visualCenter, and in the unit test of taking a screenshot with a framed scrolled, I called it just before calling boundingBox to ensure the box is visible. looks good by me.

aslushnikov · 2017-10-10T01:39:07Z

as my last commit states, I added a scrollIntoViewIfNeeded method and used is as part of the visualCenter, and in the unit test of taking a screenshot with a framed scrolled, I called it just before calling boundingBox to ensure the box is visible. looks good by me.

@elisherer element.scrollInfoViewIfNeeded would not be used for anything other then screenshots. In this case, we can just have the elementHandle.screenshot. Conveniently, the ElementHandle now has a reference to the page: https://github.com/GoogleChrome/puppeteer/blob/f1aa18af4e713abfb170e1f9f1d3a3d378b32987/lib/ElementHandle.js#L31

Even with elementHandle.screenshot, we should keep the ElementHandle.boundingBox: it comes handy in multiple places.

# Conflicts: # lib/ElementHandle.js Also add /test/test-user-data-dir* to gitignore

aslushnikov

Great work, thank you.

zhongjiewang · 2018-03-08T04:16:30Z

if the elementhandle in a frame, screenshot method , does it work ?

Add an option to take a screenshot based on a selector (on top of the…

1db2445

… clip option)

aslushnikov mentioned this pull request Aug 21, 2017

examples: add screenshot dom element example #452

Closed

revert selector screenshot capturing in favor of get bounding box of …

3ad3cc7

…an element (unit tests and documentation, with example, were updated)

elisherer changed the title ~~Add an option to take a screenshot based on a selector~~ Add get bounding box to ElementHandle for easier element screenshot Aug 22, 2017

doc: elementHandle.boundingBox missing await in example

8adff91

JoelEinbinder reviewed Aug 22, 2017

View reviewed changes

aslushnikov reviewed Aug 22, 2017

View reviewed changes

Eli Sherer added 2 commits August 22, 2017 13:37

boundingBox: use getBoundingClientRect instead of DOM.getBoxModel

4d3d62e

fix lint warning about preferring const

62c11bf

aslushnikov reviewed Aug 22, 2017

View reviewed changes

elisherer and others added 2 commits August 23, 2017 00:29

boundingBox: change box calculation algorithm to be relative to docum…

a83b807

…ent, add unit test for being used together with screenshot when not visible

boundingBox: fix getting box by using box model (relative to main fra…

8e3374c

…me), unit test added for nested frames.

aslushnikov reviewed Aug 23, 2017

View reviewed changes

aslushnikov mentioned this pull request Aug 30, 2017

Clicking on an element in an iframe fails #618

Closed

Merge branch 'master' into feature/screenshot-by-selector

c8864d0

# Conflicts: # lib/ElementHandle.js

ebidel added the P1 label Sep 21, 2017

Eli Sherer and others added 2 commits September 28, 2017 15:38

Merge branch 'master' into feature/screenshot-by-selector

07dbcf0

fix(ElementHandle): Do upwards frame traversal to get the bounding bo…

cef1d4c

…x inside the page. Also fixes the visible center used by mouse and touchscreen.

Merge branch 'master' into feature/screenshot-by-selector

65d7d71

elisherer added 2 commits October 7, 2017 14:54

feat(ElementHandle): boundingBox; bring back usage of getBoxModel

ebf2fb9

Merge branch 'master' of https://github.com/GoogleChrome/puppeteer in…

64c4c8f

…to feature/screenshot-by-selector # Conflicts: # docs/api.md # lib/ElementHandle.js

aslushnikov reviewed Oct 7, 2017

View reviewed changes

boundingBox: remove implicit scrollIntoViewIfNeeded, and expose it as…

67d0c1c

… a new method of element handle, fix executionContext jsdoc

Merge branch 'master' into feature/screenshot-by-selector

fd28715

Eli Sherer added 2 commits October 10, 2017 08:53

feat(ElementHandle): introduce ElementHandle.screenshot

a68a674

Merge branch 'master' into feature/screenshot-by-selector

7b8cdc1

# Conflicts: # lib/ElementHandle.js Also add /test/test-user-data-dir* to gitignore

aslushnikov approved these changes Oct 10, 2017

View reviewed changes

aslushnikov merged commit 7e28dba into puppeteer:master Oct 10, 2017

elisherer deleted the feature/screenshot-by-selector branch October 10, 2017 06:19

Add get bounding box to ElementHandle for easier element screenshot #445

Add get bounding box to ElementHandle for easier element screenshot #445

Conversation

elisherer commented Aug 21, 2017 • edited

ebidel commented Aug 21, 2017

aslushnikov commented Aug 21, 2017

elisherer commented Aug 21, 2017

googlebot commented Aug 21, 2017

elisherer commented Aug 21, 2017

googlebot commented Aug 21, 2017

aslushnikov commented Aug 21, 2017

elisherer commented Aug 22, 2017

aslushnikov commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elisherer Aug 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Sep 5, 2017

aslushnikov commented Sep 5, 2017

elisherer commented Oct 2, 2017

elisherer commented Oct 3, 2017

aslushnikov commented Oct 5, 2017

aslushnikov commented Oct 6, 2017

elisherer commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslushnikov Oct 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elisherer commented Oct 9, 2017

aslushnikov commented Oct 10, 2017

aslushnikov left a comment

Choose a reason for hiding this comment

zhongjiewang commented Mar 8, 2018

elisherer commented Aug 21, 2017 •

edited

elisherer Aug 23, 2017 •

edited

aslushnikov Oct 7, 2017 •

edited