Skip to content

Conversation

@macarie
Copy link
Member

@macarie macarie commented Jul 28, 2025

Description

This PR fixes the issue of masks not being applied correctly when taking a screenshot, as previously discussed in the context of #8041.

Masking elements is an important feature when handling dynamic content in Visual Regression Testing.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.
  • Please check Allow edits by maintainers to make review process faster. Note that this option is not available for repositories that are owned by Github organizations.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

@macarie macarie force-pushed the fix/screenshot-masks branch from 68a7e21 to db359cf Compare July 28, 2025 22:41
Copy link
Member Author

@macarie macarie left a comment

Choose a reason for hiding this comment

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

I’ve also corrected the internal type for screenshot options, which was not used correctly in #8041.


export interface ScreenshotOptions extends PWScreenshotOptions {}
export interface ScreenshotOptions extends PWScreenshotOptions {
mask?: ReadonlyArray<Element | Locator> | undefined
Copy link
Member Author

Choose a reason for hiding this comment

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

This property is already declared in PWScreenshotOptions but their Locator definition is not compatible with Vitest's.

Comment on lines -351 to -362
function convertToSelector(elementOrLocator: Element | Locator): string {
if (!elementOrLocator) {
throw new Error('Expected element or locator to be defined.')
}
if (elementOrLocator instanceof Element) {
return convertElementToCssSelector(elementOrLocator)
}
if ('selector' in elementOrLocator) {
return (elementOrLocator as any).selector
}
throw new Error('Expected element or locator to be an instance of Element or Locator.')
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why, but without handling Locators like this, the mask option was ignored when used in the toMatchScreenshot assertion.

I moved the function in the same file that exports convertElementToCssSelector (the function used for Element), hopefully it's not an issue.

const normalizedOptions = 'mask' in options
? {
...options,
mask: (options.mask as Array<Element | Locator>).map(convertToSelector),
Copy link
Member Author

@macarie macarie Jul 28, 2025

Choose a reason for hiding this comment

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

The same code is used in toMatchScreenshot. I guess it's fine for now as it's only one option, but if it grows we should extract the logic into a function to use in both places.

Comment on lines +347 to +352
await locator.screenshot({
save: true,
path,
maskColor,
mask,
})
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find any tests for screenshot, but this test should cover it as well.

@macarie macarie marked this pull request as ready for review July 28, 2025 22:59
@sheremet-va sheremet-va merged commit 459efba into vitest-dev:main Jul 31, 2025
12 of 14 checks passed
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