[Reporting] set viewport to include clip area #87253
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsullivan
added
the
zDeprecated Feature:Reporting
tsullivan
force-pushed
the
fix/reporting-viewport-regression
branch
from
fb267b1 to
a45e22c
Compare
tsullivan
force-pushed
the
fix/reporting-viewport-regression
branch
from
54d8b7c to
c418bad
Compare
Member
Author
|
@elasticmachine merge upstream |
tsullivan
added
v8.0.0
release_note:skip
Member
Author
|
@elasticmachine merge upstream |
poffdeluxe
approved these changes
Jan 6, 2021
Contributor
poffdeluxe
left a comment
poffdeluxe
left a comment
There was a problem hiding this comment.
Changes look good to me!
tsullivan
commented
Jan 6, 2021
| const [viewWidth, viewHeight] = await browser.evaluate( | ||
| { | ||
| fn: () => [document.body.clientHeight, document.body.clientWidth], | ||
| fn: () => [document.body.clientWidth, document.body.clientHeight], |
Member
Author
There was a problem hiding this comment.
Sometimes it's hard to visually check that we assign height values to height values, and width values to width values. I use a plugin to highlight different strings different colors to give it a color pattern check, and that's what I did here to see that there was a bug.
Member
Author
|
@elasticmachine merge upstream |
Contributor
💚 Build SucceededMetrics [docs]Distributable file count
History
To update your PR or re-run it, just comment with: |
This was referenced Jan 6, 2021
tsullivan
added a commit
to tsullivan/kibana
that referenced
this pull request
Jan 6, 2021
* [Reporting] set viewport to include clip area * remove getViewport * fix tests * simpler * fix 1 * revert * hacks * scope the logging variables * polish * hacky fix * quieter logging * make less hacky * fix functional test * revert lowering log level of browser console messages * revise comments * setViewport only to happen once * fix snapshot of layout type tests * fix comment text * Revert "setViewport only to happen once" This reverts commit 15977f9. * fix disgusting bug * use x/y ordering for width/height * fix fn test snapshots Co-authored-by: Kibana Machine <[email protected]>
tsullivan
added a commit
that referenced
this pull request
Jan 7, 2021
* [Reporting] set viewport to include clip area * remove getViewport * fix tests * simpler * fix 1 * revert * hacks * scope the logging variables * polish * hacky fix * quieter logging * make less hacky * fix functional test * revert lowering log level of browser console messages * revise comments * setViewport only to happen once * fix snapshot of layout type tests * fix comment text * Revert "setViewport only to happen once" This reverts commit 15977f9. * fix disgusting bug * use x/y ordering for width/height * fix fn test snapshots Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
tsullivan
added a commit
that referenced
this pull request
Jan 7, 2021
* [Reporting] set viewport to include clip area * remove getViewport * fix tests * simpler * fix 1 * revert * hacks * scope the logging variables * polish * hacky fix * quieter logging * make less hacky * fix functional test * revert lowering log level of browser console messages * revise comments * setViewport only to happen once * fix snapshot of layout type tests * fix comment text * Revert "setViewport only to happen once" This reverts commit 15977f9. * fix disgusting bug * use x/y ordering for width/height * fix fn test snapshots Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #85864
Problem: In the new version of puppeteer that we have upgraded to for 7.11, the
page.screenshotfunction works differently withclipoptions.Previously, the viewport of the headless browser only needed to be resized to the dimensions of the items to capture, and it didn't matter if that item was displayed fully in the viewport or not: the item image could be captured regardless because the
clipoptions controlled the rectangle locating the visualization.Now, the image will not be fully captured unless it is entirely in the viewport.
This PR attempts to solve the issue by resizing the browser right after calculating the clip position of the element to capture.
Full changes
getScreenshotsresizes the viewport after determining the capture item(s) position and before callingbrowser.screenshot().tracemethod to the Reporting logger class. The new debug log lines added in this PR are very useful and get lost in the swarm of request logging from the browser process.getViewportmethods of the layout type instances to return the width and height without scaling. This stops the pointless flow of multiplying the numbers ingetViewportand then dividing the same numbers insetViewport.setViewportcall from thescreenshotsObservablefunction, as it is now called ingetScreenshotsdebugtotraceChecklist
Delete any items that are not applicable to this PR.
For maintainers