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
Added 6 background-size tests #22201
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the tiling, it cannot be determined from the test whether the background image is correctly sized or not. You are focusing too much on creating a green square, and not enough on making the test precise. It is better to have a precise test, than to have the test look like a green square. Reftests are automated, it is totally fine for each one to have a different reference. It is nice to have it be an easy-for-humans-to-interpret reference, but it is important for it to be a sufficiently restrictive test.
Also, to reduce the time spent on test runs and to reduce the duplication of code, I recommend to combine all these tests into a single file.
Lastly, please use a ratio other than 1:1, to ensure that the UA is not defaulting to 1:1.
@fantasai , It took me a while (and several re-reading) to understand your comment ... but now I think I do. I can create a single test page from (or with) all these 6 tests and then add the pitfall ("embûche") of not having a 1:1 background positioning area (for each sub-tests), therefore making the set of these tests more requiring, more demanding, with more 'banana peels' in the path of the rendering engine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, “Because of the tiling, it cannot be determined from the test whether the background image is correctly sized or not. You are focusing too much on creating a green [rectangle], and not enough on making the test precise.”. Your test does not check whether the gradient is sized correctly. You cannot actually see the size of the background image, because it is completely green and it is tiled, there is no way to see its boundaries.
The original test in the issue did not have this problem. Because it used a radial gradient, it was possible to see the size of the image even though it is tiled.
You can either tile the image or make it completely green, but you cannot do both and have a viable test.
Newest version of the test: Reference of such newest version of the test: With background-size-041-newest1.html test, I believe it checks the actual size of the (linear-gradient) background image despite and while being completely green and being tiled. @fantasai Is such version of the test correct? |
Ways this test can pass on an incorrect implementation:
|
I must close this PR and close this branch. I will create a new PR in a few min. It will have the most important comments from this PR pasted in the new PR, along with 4 new tests, 4 reference files and 1 short SVG file. A new TC config was implemented on July 7th and that forces me to create a new PR. |
Those 6 tests are a followup to
[css-backgrounds-3] background-size with " auto" and gradient image is not interoperably implemented
background-size-041.html
background-size-042.html
background-size-043.html
background-size-044.html
background-size-045.html
background-size-046.html
reference/ref-filled-green-100px-square.xht
support/green-intrinsic-none.svg