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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Puppeteer, remove resize handling in visual regression specs #438

Merged
merged 1 commit into from
May 7, 2024

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 22, 2024

馃洜 Summary of changes

This pull request:

  • Updates Puppeteer to the latest version
  • Removes resize handling in visual regression specs

The intent is to try to resolve inconsistencies observed in visual regression failures in #437:

  • As noted at Use frame for sandboxed component preview, stricter accessibility checks聽#437 (comment), the fillImageToSize helper doesn't work as intended, since it will only fill pixels which are missing outside both the expected width and height, but not either lacking height nor lacking width.
    • Ideally we don't want to tolerate any difference in dimensions. By git history, it's not entirely clear why this was added.
  • In local testing, it seems that updating Puppeteer does have an impact in allowing the visual regression spec to pass, which could be due to differences in Chromium browser rendering. Unfortunately, due to how the visual regression test needs to change branches, main running an older version of Puppeteer can cause the failure to re-emerge. Performing the upgrade in a separate pull request allows the update to be made to main independent of the changes in Use frame for sandboxed component preview, stricter accessibility checks聽#437.

馃摐 Testing Plan

Build should pass.

@aduth
Copy link
Member Author

aduth commented Apr 22, 2024

For posterity, I did create a "fixed" version of fillImageToSize if we need it again in the future:

/**
 * Resizes an image to the given dimensions.
 *
 * @param {import('pngjs').PNGWithMetadata} image
 * @param {number} width
 * @param {number} height
 *
 * @return {import('pngjs').PNG}
 */
function fillImageToSize(image, width, height) {
  if (image.width === width && image.height === height) {
    return image;
  }

  const resizedImage = new PNG({ width, height, fill: true });
  PNG.bitblt(image, resizedImage, 0, 0, image.width, image.height, 0, 0);

  /**
   * @param {number} x
   * @param {number} y
   */
  function fillPixel(x, y) {
    // eslint-disable-next-line no-bitwise
    const index = (resizedImage.width * y + x) << 2;
    resizedImage.data[index] = 255; // Red
    resizedImage.data[index + 1] = 255; // Green
    resizedImage.data[index + 2] = 255; // Blue
    resizedImage.data[index + 3] = 255; // Alpha (Opacity)
  }

  for (let y = image.height; y < height; y++) {
    for (let x = 0; x < width; x++) {
      fillPixel(x, y);
    }
  }

  for (let x = image.width; x < width; x++) {
    for (let y = 0; y < height; y++) {
      fillPixel(x, y);
    }
  }

  return resizedImage;
}

@aduth aduth force-pushed the aduth-puppeteer-rm-fill-to-size branch from 0a41fd0 to 56d5ae5 Compare May 7, 2024 20:48
@aduth aduth merged commit c4c1580 into main May 7, 2024
4 checks passed
@aduth aduth deleted the aduth-puppeteer-rm-fill-to-size branch May 7, 2024 20:56
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.

None yet

2 participants