Add new devicePixel ref test

[greggman]

What the test does:

It sets the body to have a 2x2 checkerboard pattern fit to devicePixels (1 pixel in the pattern = 1 devicePixel)

It then puts a bunch of children above that in rows and uses devicePixelContentBoxSize to compute where each child is. It sets the same pattern for each child and offsets the pattern based on the child's calculated position so the patterns should align. If they don't align because the values returned from devicePixelContentBoxSize it should be very clear.

Here's Chrome running the test

Here's Firefox running the test

Related:
#34569

[atotic]

I will defer to dholbert on this one. I no longer work on web standards, and have used up my "free review time" for a while.

[chrishtr]

Can you just achieve the same goal with a minimal number of elements declared in HTML rather than built in script? That will be a better reduced testcase, and also make it more clear what layout you're achieving.

[dholbert]

shrug I suppose, though I think this is fine in JS, too (given the HTML shown in the code-comment).

I could imagine morphing this into e.g. declarative HTML with 5 flex containers with the most-"interesting" counts of flex items, in the hopes of triggering this bug; but it's also hard to predict which counts are most-likely "interesting" and most-likely-to-reliably-trigger-this-sort-of-bug. So, I kind of appreciate the current exhaustive "try every possible child-count value from 2 to 15" approach, which would be somewhat messy/verbose to encode in declarative HTML.

It would be nice, however, if we adjusted this to either rename i or document what it represents (the total number of flex items that we're going to generate for the current flex container).

[atotic]

devicePixelSize gets rounded depending on element location/size. When I used to test this, I'd create a canvas with fractional width (100.5px?), and then vary the location by fraction (0.1px increments).
The flexbox tests this indirectly (because varying number of children varies their locations/size).

I can't come up with anything that looks as clean as this test, but I'd prefer to explicitly set location/size. Relying on flexbox to do it for me also exercises flexbox algorithm, and I always try to minimize my dependencies.

[dholbert]

The flexbox tests this indirectly (because varying number of children varies their locations/size).
[...]
I'd prefer to explicitly set location/size.

So, I don't think it's possible to both use this test's same relatively-clean "checkerboard overlaying" strategy (which I like) and to also directly/explicitly set the position of any element. If you directly set an element's position to anything other than the origin, then you can't know for sure whether that element will be painted at an even vs. odd device-pixel-position. And you have to have that information in order to properly set background-position on that element (or its children, etc) to align their checkerboard-image-background with the document's checkerboard-image-background.

(That's kind of the point of this test -- if the cumulative devicePixelBoxSize measurements of assumed-to-be-adjacent elements are correct, then we'll be able to correctly predict how to set background-position on the various checkerboard elements so that they align properly with the checkerboard behind them and won't stand out).

IMO this setup is pretty reasonable. I could imagine (but probably wouldn't require) simplifying things by making all the flex containers have only 2 flex-items, both inflexible (flex: none with specified width and height), with a variety of widths on the first items in the flex containers (e.g. ranging from 10.0px, 10.1px, etc, up to 11px), so that the second flex item will end up having different pixel-snapping behavior in different flex containers. Then we would set the background-position on the second flex item based on its assumed device-pixel x/y coordinates, which we would determine based on the previous flex item's observed size and based on the cumulative sizes of the previous flex containers (as the test currently does).

This would let us use explicit hardcoded sizes, and it would sort-of let us explicitly specify positions as well (via positions being trivially inferrable based on the sizes of prior-siblings), if those things are important to you.

(We could also achieve a similar setup using floats instead of flexbox, but IMO float layout is hairier than flexbox in simple setups like this, particularly when we're using flexbox pretty trivially as this test does with no complex content-based sizing or alignment.)

[dholbert]

sorry for the delay here!

Overall, this makes sense to me, as long as we have the guarantee that two adjacent flex items should be drawn with directly-abutting device-pixel edges on their content boxes (which seems like a reasonable assumption).

I've got some readability/clarity notes at this point, but the overall approach seems reasonable to me.

[greggman]

Thank you for the review. I think I addressed all the issues you raised.

I changed the pattern to be 4x4 instead of 2x2. My thinking was 2x2 will fail if off by 1 but succeed if off by 2 or if both x and y are off by 1. With 4x4 the pattern should fail in more cases (though I expect the normal "bad" case to be off by 1)

[dholbert]

Thanks for addressing the review feedback! I'll take a further look later and run this against Firefox with the patch (the fix for our bug landed today!)

Dropping some quick nits for now...

[dholbert]

Viewing today's updated test-version in Chrome on my regular no-pixel-scaling display, I see "fringes" around various flex items at a handful of zoom levels -- e.g. 110% and 125% at least (the first two Ctrl+ ticks).

Needless to say, those location-specific "fringes" aren't present in the reference case, because the reference case just draws a single pattern across the whole page. So, the test seems to fail in Chrome under full-page zoom (and presumably at various HiDPI display levels).

Assuming a valid test and implementation, I think we wouldn't expect these fringes to be present... Full-page-zoom does get reflected in devicePixelRatio, so the test should theoretically be accounting for it properly in its device pixel arithmetic. I wonder if this is a test bug (/ if we're assuming too much) or a Chrome bug.

[greggman]

wow, that was hard to see. It was a bug in the test. The pattern needed to be offset -(x % patternSize). That bug didn't show when the pattern was 2x2 since positive or negative would be the same but with a 4x4 pattern the offset needs to be negative. Uploaded a fix and I changed the pattern so the issue would have stuck out more. Also note: Firefox fixed their bug yesterday so Firefox Nightly display this correctly now.

[greggman]

Thanks again for the review. I think I addressed your latest comments.

[dholbert]

Thanks for the fixes!

One more thing to fix: there's a "lint" task that's failing, due to some whitespace-at-the-end-of-various-lines (see the lint task in the Some checks were not successful section here -- direct link: https://github.com/web-platform-tests/wpt/pull/34682/checks?check_run_id=7383242439

That needs fixing. With that, this might be good, I think? I'll take a final look tomorrow and let you know if I see any final issues. :)

FWIW, I also ran a (slightly tweaked) version of your test (as-of-earlier-today) through Firefox Nightly CI, and I confirmed that it passes on trunk -- e.g. here's the pass result for a MacOS run:
https://treeherder.mozilla.org/logviewer?job_id=384603288&repo=try&lineNumber=19899-19902
...and I'm doing another Try run to confirm that it fails on builds from before the patch landed (which I expect it does based on observing it rendering badly in Firefox release):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e178b200151fdfe17056da1e7c5cd99da8c53ed3

[EDIT added the next day: yes, this Try run does show that it fails as-expected in older Firefox Nightly builds. Hooray!]

[dholbert]

(There's also a failing task in "Azure Pipelines" but that seems to be some sort of infrastructure issue with Safari Technical Preview failing to be downloaded for a particular platform -- not your problem / not related to this test, I think.

Direct link to the failure there:
https://dev.azure.com/web-platform-tests/b14026b4-9423-4454-858f-bf76cf6d1faa/_apis/build/builds/84592/logs/27

2022-07-18T04:53:00.0501510Z + ./wpt install --channel preview --download-only -d . --rename STP safari browser
2022-07-18T04:53:07.8874680Z INFO:install:Now installing safari browser...
[...]
2022-07-18T04:53:08.1433370Z ValueError: no download for 12.4
2022-07-18T04:53:08.1621610Z ##[error]Bash exited with code '1'.
2022-07-18T04:53:08.1715030Z ##[section]Finishing: Install Safari Technology Preview

@jgraham do you know who we should notify about that? (Perhaps someone who works on WPT infra stuff and/or Safari/WPT integration?)

[dholbert]

(maybe @gsnedders knows what's going on with the Safari Technology Preview azure-pipelines task, and if that's something we need to worry about? [seems like an infrastructure issue, as noted above]

[gsnedders]

(maybe @gsnedders knows what's going on with the Safari Technology Preview azure-pipelines task, and if that's something we need to worry about? [seems like an infrastructure issue, as noted above]

#34875

[dholbert]

Final clarity nits; looks good to me with these changes!

[dholbert]

maybe move the elemToDevicePixelSize declaration inside of the setPatternsUsingSizeInfo() function, since that's the only place it's used?

[dholbert]

Looks good, aside from two small typos - thanks for all the fixups & getting this over the finish line! I think we can merge once these last nits are addressed.

[dholbert]

I clicked the button to merge; hopefully everything works properly. :)

Thanks again for the Firefox bug report and for the thorough test!

[dholbert]

wpt.fyi now has this test deployed, and shows it passing on all browsers; hooray!
https://wpt.fyi/results/resize-observer/devicepixel2.html