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

Add tests for mathematical functions in <img sizes=""> #10167

Merged

Conversation

Projects
None yet
5 participants
@domfarolino
Copy link
Member

domfarolino commented Mar 25, 2018

This should be enough to test all of the math functions here, for whatwg/html#3084

Edit

Also, I'm curious about the tests here. Currently we seem to be trying to test that various, sometimes media-condition adorned sizes attribute values resolve to the same computed value. We test this by checking that currentSrc for each image (in a particular group) is the same (since the computed value of sizes can affect the choice of image), however this is a little incomplete right?

I tested in Chrome and Firefox, and sometimes vastly different sizes attribute values have no effect on the chosen image. Doesn't this mean that even if the parsing of a particular sizes attribute failed (and we either chose some very wrong value, or fell back on the default 100vw) we could still be choosing the correct image just based on how a particular user agent decides to choose an image? So we might fail parsing sizes properly, yet still pass a bunch of tests.

If this is the case, I'm wondering if it might be more beneficial to instead not have multiple sources in the srcset, and choose a particular non-100vw sizes value so we can reliably see if the actual computed width (maybe clientWidth?) of an image is what it should be after the width descriptor is factored in with the expected sizes value?

@wpt-pr-bot wpt-pr-bot added the html label Mar 25, 2018

@wpt-pr-bot wpt-pr-bot requested review from ayg, jdm, jgraham and zqzhang Mar 25, 2018

@domfarolino

This comment has been minimized.

Copy link
Member Author

domfarolino commented Mar 25, 2018

/cc @annevk @zcorpan Might want to review this as they've seen/reviewed the relevant PR

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 25, 2018

Build PASSED

Started: 2018-03-25 05:16:23
Finished: 2018-03-25 05:26:23

View more information about this build on:

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 26, 2018

@Wilto and @yoavweiss might have more suitable feedback. What you're suggesting sounds correct to me, but I haven't really studied this space much.

@domfarolino

This comment has been minimized.

Copy link
Member Author

domfarolino commented Mar 26, 2018

Thanks. And to simplify what I was saying earlier (since it is a tad lengthy), consider the following test case:

<p>
<img srcset="test.png 600w, make.png 601w" sizes="40px">
<img srcset="test.png 600w, make.png 601w" sizes="min(40px, 80px)">

In this case, Chrome passes the test (Chrome just seems to always choose the src with the highest w descriptor) as it chooses the same source for each <img>, even though the actual rendered sizes are completely different (because min, not yet implemented in Chrome, fails to parse and defaults to 100vw).

@domfarolino domfarolino changed the title Add tests for https://github.com/whatwg/html/pull/3084 Add tests for mathematical functions in <img sizes=""> Apr 2, 2018

@domfarolino

This comment has been minimized.

Copy link
Member Author

domfarolino commented Apr 2, 2018

As requested @annevk , this seems to be in limbo and I'm not familiar enough with this area to intelligently request reviewers. If you wanted to take a look at this perhaps that would be great, and if you need any clarification/information from me please let me know!

zaphodtx pushed a commit to zaphodtx/web-platform-tests that referenced this pull request Apr 3, 2018

@zcorpan

This comment has been minimized.

Copy link
Contributor

zcorpan commented Apr 3, 2018

I think your analysis is correct and your suggested change seems interesting. I think I wanted to test selection, since that is the primary feature for sizes. I'm open to changing how we test sizes though.

@zcorpan

zcorpan approved these changes Apr 3, 2018

@@ -52,8 +52,14 @@
<img srcset='/images/green-1x1.png?e34 50w, /images/green-16x16.png?e34 51w' sizes='\[,1px'>
<img srcset='/images/green-1x1.png?e35 50w, /images/green-16x16.png?e35 51w' sizes='1\p\x'>
<img srcset='/images/green-1x1.png?e36 50w, /images/green-16x16.png?e36 51w' sizes='calc(1px)'>
<img srcset='/images/green-1x1.png?e36 50w, /images/green-16x16.png?e36 51w' sizes='min(1px, 100px)'>

This comment has been minimized.

Copy link
@zcorpan

zcorpan Apr 3, 2018

Contributor

The number in the query should be unique so that we avoid having the browser prefer to load a candidate based on it already being in cache. You can use 36a or so.

@domfarolino

This comment has been minimized.

Copy link
Member Author

domfarolino commented Apr 6, 2018

Good catch, I've addressed that. Should be all for now; should I maybe open an issue as a reminder to go back and address some of the things I previously brought up regarding testing that each group has the same image selected?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Apr 9, 2018

Yeah, please.

@annevk annevk merged commit cc9648d into web-platform-tests:master Apr 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

annevk added a commit to whatwg/html that referenced this pull request Apr 9, 2018

@domfarolino domfarolino deleted the domfarolino:img-sizes-parsing-tests branch Apr 9, 2018

yutakahirano added a commit to yutakahirano/html that referenced this pull request Apr 26, 2018

alice added a commit to alice/html that referenced this pull request Jan 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.