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

Added 6 background-size tests #22201

Closed

Conversation

TalbotG
Copy link
Contributor

@TalbotG TalbotG commented Mar 12, 2020

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

Copy link
Contributor

@fantasai fantasai left a 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.

@TalbotG
Copy link
Contributor Author

TalbotG commented Apr 2, 2020

Because of the tiling, it cannot be determined from the test whether the background image is correctly sized or not. (...) it is important for it to be a sufficiently restrictive test.
(...) 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.
I can also create another single test page with 4 (or 6 sub-tests) and then test with background-size: auto 10px (and background-size: auto 10%) and background-repeat: repeat-y .

@TalbotG TalbotG removed the request for review from dbaron June 15, 2020 22:59
@TalbotG TalbotG assigned fantasai and frivoal and unassigned dbaron Jun 15, 2020
Copy link
Contributor

@fantasai fantasai left a 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.

@TalbotG
Copy link
Contributor Author

TalbotG commented Sep 24, 2020

Newest version of the test:
http://www.gtalbot.org/BrowserBugsSection/CSS3Backgrounds/background-size-041-newest1.html

Reference of such newest version of the test:
http://www.gtalbot.org/BrowserBugsSection/CSS3Backgrounds/reference/background-size-041-ref-newest1.html

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?

@fantasai
Copy link
Contributor

Ways this test can pass on an incorrect implementation:

  • UA doesn't support background-size at all, or ignores the declaration.
  • UA handles sizing in the y axis correctly, but not the x axis.
  • UA resolves to a size taller than the background positioning area. (We can't tell if the image was sized correctly, or oversized and clipped to the background painting area.)
  • UA assumes a 1:1 ratio -- such an incorrect assumption will still pass 4/6 subtests.

@TalbotG
Copy link
Contributor Author

TalbotG commented Nov 21, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants