fix: absolute positioning element blackouts in cy.screenshot #22756
Conversation
|
Thanks for taking the time to open a PR!
|
| return before($el) | ||
| // get the current body of the AUT to accurately calculate screenshot blackouts | ||
| // as well as properly enable/disable CSS animations while screenshotting is happening | ||
| const $body = Cypress.$('body') |
There was a problem hiding this comment.
is there possibly a cleaner way to reference the body here?
There was a problem hiding this comment.
Prob isn't what you are looking for, but I think we store the AUT frame on the cypress instance
$autIframe
There was a problem hiding this comment.
I think this is fine - since the Cypress instance is injected to the AUT, and you can only have one instance of HTMLBodyElement (or the browser will raise an error) this seems safe to me.
| blackout: ['.blue'], | ||
| onBeforeScreenshot () { | ||
| const blackedOutElementCoordinates = Cypress.$( | ||
| '#__cypress-animation-disabler+div.__cypress-blackout', |
There was a problem hiding this comment.
I'm open to better selectors here. This seems to be a bit of a coincidence that our blacked out elements are adjacent to the animation disabler element. Also the test assertions below aren't great, but at least should guarantee that the blacked out element is very close to the thing it's trying to black out.
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
AtofStryker
requested review from
BlueWinds,
chrisbreiding,
emilyrohrbough,
mjhenkes and
mschile
and removed request for
a team and
BlueWinds
lmiller1990
left a comment
lmiller1990
left a comment
There was a problem hiding this comment.
Nit around types - probably a quick win, but going to approve.
| } | ||
|
|
||
| const before = ($el) => { | ||
| const before = ($body, $container) => { |
There was a problem hiding this comment.
Can we incrementally ad types as we go? HTMLBodyElement and HTMLElement, in this case.
There was a problem hiding this comment.
Going to create a follow up PR to add types to the screenshot files since it's quite a bit of changes
There was a problem hiding this comment.
opened #22768 against this branch and will rebase it once I merge this in
| return before($el) | ||
| // get the current body of the AUT to accurately calculate screenshot blackouts | ||
| // as well as properly enable/disable CSS animations while screenshotting is happening | ||
| const $body = Cypress.$('body') |
There was a problem hiding this comment.
I think this is fine - since the Cypress instance is injected to the AUT, and you can only have one instance of HTMLBodyElement (or the browser will raise an error) this seems safe to me.
User facing changelog
Fixes a regression in 9.6 where absolute elements that are blacked out with
cy.screenshotare positioned off the container element and not the body.Additional details
This PR attempts to fix a regression when screenshots were refactored for
cy.originin #20949 where element dimensions are calculated from the screenshotted element. This works in most cases, but causes some trouble with absolute elements. To fix this, since we no longer have access to theAutIframeclass, is to reference the body to calculate dimensions for elements.Steps to test
See changes to the
screenshot_viewport_capture_spec.jsHow has the user experience changed?
Before code changes with added regression test in
screenshot_viewport_capture_spec.jsAfter code changes with added regression test in
screenshot_viewport_capture_spec.jsPR Tasks
cypress-documentation?type definitions?