Skip to content

fix: Take page scroll into account in ElementHandle.screenshot()#172

Merged
lino-levan merged 1 commit intolino-levan:mainfrom
adamgreg:fix-el-screenshot
Oct 3, 2025
Merged

fix: Take page scroll into account in ElementHandle.screenshot()#172
lino-levan merged 1 commit intolino-levan:mainfrom
adamgreg:fix-el-screenshot

Conversation

@adamgreg
Copy link
Contributor

@adamgreg adamgreg commented Oct 1, 2025

I've found that the clipping in ElementHandle.screenshot() is incorrect if the page has been scrolled, including if it was scrolled to make the element visible for the screenshot.

The co-ordinates in the element's box model are changed after scrolling, but the screenshot clip argument needs the original values.

@adamgreg
Copy link
Contributor Author

adamgreg commented Oct 1, 2025

Related: chromedp/chromedp#844

@lino-levan
Copy link
Owner

Can we merge master in to fix CI?

Copy link
Owner

@lino-levan lino-levan left a comment

Choose a reason for hiding this comment

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

I assume you've tested this and it fixes the bug, wish we could have a test here but happy to merge once you confirm that this does actually resolve the issue.

@adamgreg
Copy link
Contributor Author

adamgreg commented Oct 3, 2025

I assume you've tested this and it fixes the bug, wish we could have a test here but happy to merge once you confirm that this does actually resolve the issue.

It does fix it, at least in the case where the scrollIntoView call in ElementHandle did the scrolling. I'm no expert on viewports and positioning though, or if scrollable containers make a difference here.

@lino-levan
Copy link
Owner

Sounds good to me

@lino-levan lino-levan merged commit 5d5d65f into lino-levan:main Oct 3, 2025
3 checks passed
@adamgreg adamgreg deleted the fix-el-screenshot branch October 6, 2025 15:28
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.

2 participants