wpt icon indicating copy to clipboard operation
wpt copied to clipboard

Update align-items-baseline tests to use checkLayout

Open sammygill opened this issue 6 months ago • 2 comments

The refs for these tests currently use baseline alignment within grid as the expectation for the rendering. This can miss test passes where an implementation fails to render the expectation correctly even though the rendering of the test itself is correct. To accommodate for this scenario we can change these to use checkLayout and check the positions of the items. This ends up matching what we do for the alignment/flex-align-baseline-* flavor of tests.

sammygill avatar May 29 '25 23:05 sammygill

Hi @bfgeek! I was working on some baseline alignment bugs and ran into a scenario where a patch rendered the test correctly but we were still failing due to a difference in the expectation. I put this change together to address that and wanted to run it by you since you worked on these tests :)

sammygill avatar May 29 '25 23:05 sammygill

@bfgeek Just wanted to ping one more time in case you had any thoughts/opinions on the changes

sammygill avatar Jun 12 '25 16:06 sammygill

This PR shouldn't change expectations, just how they are checked (e.g. testharness.js instead of reftest)

Seems fine to me, assuming browsers that were passing all still pass the test (i.e. that we're not changing expectations).

Minor nits that @sammygill might want to address:

(1) Right now the test files have font-family: Ahem; in one CSS rule (implicitly with font-size and line-height left at default values), with font-size: 30px; line-height: 30px in other CSS rules. Instead of doing that, ideally we should specify all three together using the font shorthand, similar to the "what is typically recommended" at https://web-platform-tests.org/writing-tests/ahem.html i.e. font: 30px/1 Ahem

(2) In at least one test -- align-items-baseline-column-horz.html -- I see a CSS rule #target > :nth-child(2) { with font-size: 20px; and then line-height: 30px (taller line-height) -- I think that mismatch was unintentional. But if it's intentional, maybe worth calling out with a code-comment. (But really, per (1), you probably just want font-family: 30px/1 Ahem on some elements and font-family: 20px/1 Ahem on others, without separate pixel-valued line-height values.)

dholbert avatar Jun 23 '25 17:06 dholbert

Thanks @dholbert! Yes, those line-heights discrepancies were unintentional and I have fixed them along with your other suggestions!

sammygill avatar Jun 23 '25 19:06 sammygill