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.

Sign up for GitHub

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

Jump to bottom

fix: absolute positioning element blackouts in cy.screenshot #22756

Merged
merged 5 commits into from Jul 13, 2022
Merged

fix: absolute positioning element blackouts in cy.screenshot #22756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4be4c3
add regression tests
AtofStryker Jul 12, 2022
77844cb
attempt to fix screenshot blacks basing blackouted element around con…
AtofStryker Jul 12, 2022
317a523
move blackout test to screenshot viewport suite and update snapshot
AtofStryker Jul 12, 2022
12724e2
add github issue to test
AtofStryker Jul 13, 2022
1002b5a
Merge branch 'develop' into bugfix/screenshot-blackouts
AtofStryker Jul 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 12 additions & 8 deletions packages/driver/src/cy/commands/screenshot.ts
View file
Open in desktop
Expand Up @@ -337,7 +337,7 @@ const takeScreenshot = (Cypress, state, screenshotConfig, options: TakeScreensho
}
}

const before = ($el) => {
const before = ($body, $container) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we incrementally ad types as we go? HTMLBodyElement and HTMLElement, in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to create a follow up PR to add types to the screenshot files since it's quite a bit of changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opened #22768 against this branch and will rebase it once I merge this in

return Promise.try(() => {
if (disableTimersAndAnimations) {
return cy.pauseTimers(true)
Expand All @@ -349,11 +349,11 @@ const takeScreenshot = (Cypress, state, screenshotConfig, options: TakeScreensho
// could fail if iframe is cross-origin, so fail gracefully
try {
if (disableTimersAndAnimations) {
$dom.addCssAnimationDisabler($el)
$dom.addCssAnimationDisabler($body)
}

_.each(getBlackout(screenshotConfig), (selector) => {
$dom.addBlackouts($el, selector)
$dom.addBlackouts($body, $container, selector)
})
} catch (err) {
/* eslint-disable no-console */
Expand All @@ -366,14 +366,14 @@ const takeScreenshot = (Cypress, state, screenshotConfig, options: TakeScreensho
})
}

const after = ($el) => {
const after = ($body) => {
// could fail if iframe is cross-origin, so fail gracefully
try {
if (disableTimersAndAnimations) {
$dom.removeCssAnimationDisabler($el)
$dom.removeCssAnimationDisabler($body)
}

$dom.removeBlackouts($el)
$dom.removeBlackouts($body)
} catch (err) {
/* eslint-disable no-console */
console.error('Failed to modify app dom:')
Expand Down Expand Up @@ -417,7 +417,11 @@ const takeScreenshot = (Cypress, state, screenshotConfig, options: TakeScreensho
? subject
: $dom.wrap(state('document').documentElement)

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')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there possibly a cleaner way to reference the body here?

Copy link
Member

Choose a reason for hiding this comment

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

Prob isn't what you are looking for, but I think we store the AUT frame on the cypress instance
$autIframe

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


return before($body, $el)
.then(() => {
if (onBeforeScreenshot) {
onBeforeScreenshot.call(state('ctx'), $el)
Expand All @@ -444,7 +448,7 @@ const takeScreenshot = (Cypress, state, screenshotConfig, options: TakeScreensho

return props
})
.finally(() => after($el))
.finally(() => after($body))
}

interface InternalScreenshotOptions extends Partial<Cypress.Loggable & Cypress.Timeoutable & Cypress.ScreenshotOptions> {
Expand Down
5 changes: 3 additions & 2 deletions packages/driver/src/dom/blackout.ts
View file
Open in desktop
Expand Up @@ -32,11 +32,12 @@ function addBlackoutForElement ($body, $el) {
$(`<div class="__cypress-blackout" style="${style}">`).appendTo($body)
}

function addBlackouts ($body, selector) {
function addBlackouts ($body, $container, selector) {
let $el

try {
$el = $body.find(selector)
// only scope blacked out elements to to screenshotted element, not necessarily the whole body
$el = $container.find(selector)
if (!$el.length) return
} catch (err) {
// if it's an invalid selector, just ignore it
Expand Down
15 changes: 9 additions & 6 deletions system-tests/__snapshots__/screenshot_viewport_capture_spec.js
View file
Open in desktop
Expand Up @@ -18,19 +18,20 @@ exports['e2e screenshot viewport capture / passes'] = `


✓ takes consistent viewport captures
✓ properly blacks out absolute elements within a relative container

1 passing
2 passing


(Results)

┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 1 │
│ Passing: 1 │
│ Tests: 2 │
│ Passing: 2 │
│ Failing: 0 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 26 │
│ Screenshots: 27 │
│ Video: true │
│ Duration: X seconds │
│ Spec Ran: screenshot_viewport_capture.cy.js │
Expand Down Expand Up @@ -91,6 +92,8 @@ exports['e2e screenshot viewport capture / passes'] = `
are (23).png
- /XXX/XXX/XXX/cypress/screenshots/screenshot_viewport_capture.cy.js/viewport-comp (1000x660)
are (24).png
- /XXX/XXX/XXX/cypress/screenshots/screenshot_viewport_capture.cy.js/properly blac (400x400)
ks out absolute elements within a relative container.png


(Video)
Expand All @@ -107,9 +110,9 @@ exports['e2e screenshot viewport capture / passes'] = `

Spec Tests Passing Failing Pending Skipped
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ✔ screenshot_viewport_capture.cy.js XX:XX 1 1 - - - │
│ ✔ screenshot_viewport_capture.cy.js XX:XX 2 2 - - - │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
✔ All specs passed! XX:XX 1 1 - - -
✔ All specs passed! XX:XX 2 2 - - -


`
59 changes: 59 additions & 0 deletions system-tests/projects/e2e/cypress/e2e/screenshot_viewport_capture.cy.js
View file
Open in desktop
Expand Up @@ -26,3 +26,62 @@ it('takes consistent viewport captures', () => {
Cypress._.times(25, fn)
})
})

// @see https://github.com/cypress-io/cypress/issues/22173
it('properly blacks out absolute elements within a relative container', () => {
cy.visit('cypress/fixtures/screenshot-blackout.html')
.get('.centered-container')
.screenshot({
blackout: ['.blue'],
onBeforeScreenshot () {
const blackedOutElementCoordinates = Cypress.$(
'#__cypress-animation-disabler+div.__cypress-blackout',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

)[0].getBoundingClientRect()

const actualElementCoordinates = Cypress.$(
'.centered-container .blue',
)[0].getBoundingClientRect()

// make sure blackout element is within 1 pixel of it's element it is supposed to black out
expect(blackedOutElementCoordinates.bottom).to.be.closeTo(
actualElementCoordinates.bottom,
1,
)

expect(blackedOutElementCoordinates.height).to.be.closeTo(
actualElementCoordinates.height,
1,
)

expect(blackedOutElementCoordinates.left).to.be.closeTo(
actualElementCoordinates.left,
1,
)

expect(blackedOutElementCoordinates.right).to.be.closeTo(
actualElementCoordinates.right,
1,
)

expect(blackedOutElementCoordinates.top).to.be.closeTo(
actualElementCoordinates.top,
1,
)

expect(blackedOutElementCoordinates.width).to.be.closeTo(
actualElementCoordinates.width,
1,
)

expect(blackedOutElementCoordinates.x).to.be.closeTo(
actualElementCoordinates.x,
1,
)

expect(blackedOutElementCoordinates.y).to.be.closeTo(
actualElementCoordinates.y,
1,
)
},
})
})
47 changes: 47 additions & 0 deletions system-tests/projects/e2e/cypress/fixtures/screenshot-blackout.html
View file
Open in desktop
@@ -0,0 +1,47 @@
<html>
<head>
<style>
.centered-container {
position: relative;
height: 400px;
width: 400px;
max-width: 100%;
margin: 0 auto;
text-align: center;
vertical-align: middle;
}
.third-container {
color: white;
}
.grey {
background-color: grey;
}
.blue {
background-color: blue;
position: absolute;
top: 0;
right: 0%;
}
.red {
background-color: red;
position: absolute;
top: 0;
right: 33.33%;
}
.purple {
background-color: purple;
position: absolute;
top: 0;
right: 66.66%;
}
</style>
</head>
<body>
<h1>Screenshot Blackout Test Absolute Positioning</h1>
<div class="centered-container grey">
<div class="third-container blue"> Blue Container</div>
<div class="third-container red"> Red Container</div>
<div class="third-container purple"> Purple Container</div>
</div>
</body>
</html>