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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Screenshots in macOS Safari - adapt to rounded corners #8161

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

htho
Copy link
Contributor

@htho htho commented Mar 26, 2024

Purpose

#8154 addresses the issue, where screenshots are not cropped on macOS (14) Safari.
The problem is, that the screenshot mark can not be found.
This is because part of the mark is not visible because of the rounded corners in macOS.

312752882-4a8a5548-85e3-4805-855e-608366c74c8b

Approach

The solution is to increase MARK_RIGHT_MARGIN in src/screenshots/constants.js.
I found that 25 is the lowest value that works for all resolutions I was able to test.

  • In order for the tests to work I parametrized the tests to depend on MARK_RIGHT_MARGIN.
  • I enabled the screenshot-tests that were skipped in the safari browser
    • fix tests
    • retina aware
    • color-profile aware

References

#8154

Pre-Merge TODO

  • Write tests for your proposed changes
  • Make sure that existing tests do not fail

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Mar 26, 2024
@htho htho temporarily deployed to authentication March 26, 2024 09:30 — with GitHub Actions Inactive
@htho
Copy link
Contributor Author

htho commented Mar 26, 2024

Regarding Tests: functional tests seem unstable on my machine, therefore I hope to get better results on the functional test from TestCafes Test Actions on GitHub.

@htho
Copy link
Contributor Author

htho commented Mar 28, 2024

Hmm. Looking at the failed tests, the test machine seems to either also have/emulate a retina screen or to use a color profile other than sRGB.

The test looks at the corners of the screenshot, at coordinates inferred from the set size.
For retina screens the screenshot has a greater resolution than the size set. Therefore the coordinates don't match. The test looks pixels in the bottom right corner, but it actually looks somewhere in the center of the screenshot.
The top-left pixel should be correct, but somehow safari renders the first row of pixels in a darker red tone than set.

If the color profile is set to something else than sRGB then the RGB color values don't match - the red box is red, but not exactly the expected value of #FF0000.

@PavelMor25 can you check the specs of the macOS machine for me?

Also: I don't understand why the Docker tests fail.

@PavelMor25
Copy link
Collaborator

Hello @htho,

Thank you for your pull request. We will review it.

The Test Functional (Local Safari) workflow utilizes GitHub-hosted runners. In our case, tests run on macOS 13. This provides all the information about the specifications of the macOS machine:

Regarding Docker tests, you need to merge the latest changes from the master branch. This should fix the tests.

@PavelMor25 PavelMor25 removed the STATE: Need response An issue that requires a response or attention from the team. label Mar 29, 2024
@htho htho temporarily deployed to authentication April 4, 2024 08:43 — with GitHub Actions Inactive
@htho htho temporarily deployed to authentication April 5, 2024 13:09 — with GitHub Actions Inactive
@htho
Copy link
Contributor Author

htho commented Apr 5, 2024

@PavelMor25 is it possible for your, to trigger the tests?

@PavelMor25
Copy link
Collaborator

Hello @htho,

We have reviewed the pull request. Could you please revert the tests to their original state and retain only changes to the MARK_RIGHT_MARGIN constant? Once it's done, we will approve and merge the PR.

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Apr 8, 2024
@htho htho had a problem deploying to authentication April 8, 2024 09:34 — with GitHub Actions Failure
@htho htho had a problem deploying to authentication April 8, 2024 10:01 — with GitHub Actions Failure
@htho htho deployed to authentication April 8, 2024 10:02 — with GitHub Actions Active
@htho
Copy link
Contributor Author

htho commented Apr 8, 2024

@PavelMor25 I reset my branch to the commit that changes MARK_RIGHT_MARGIN.

Are you sure, you also want me to remove the commit that parameterizes the server tests?
Because without this commit, tests in crop-test.js fail.

@htho
Copy link
Contributor Author

htho commented Apr 8, 2024

For future reference, this is a branch with the additional changes I made to fix functional tests for Safari, including adjustments for Retina screens, and color profiles: https://github.com/htho/testcafe/commits/fix-functional-safari-tests

@PavelMor25 PavelMor25 merged commit 934b73e into DevExpress:master Apr 9, 2024
21 checks passed
@PavelMor25 PavelMor25 changed the title Fix Screenshots in macOS Safari - adapt to rounded corners Fix Screenshots in macOS Safari - adapt to rounded corners (closes #8154) Apr 9, 2024
@PavelMor25 PavelMor25 changed the title Fix Screenshots in macOS Safari - adapt to rounded corners (closes #8154) Fix Screenshots in macOS Safari - adapt to rounded corners Apr 9, 2024
@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Apr 9, 2024
@htho
Copy link
Contributor Author

htho commented Apr 9, 2024

great thanks!

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