[css-values-4] Tests for cyclic dependency resolution for the lh/rlh units #8886
Conversation
wpt-pr-bot
requested review from
dbaron,
fantasai,
FremyCompany,
plinss and
tabatkins
Build PASSEDStarted: 2018-01-04 03:09:13 View more information about this build on: |
css/css-values/lh-unit-001.html
Outdated
| } | ||
| aside { | ||
| background: green; | ||
| font-size: 42px; /* number doesn't matter, as long as it's not 100 */ |
There was a problem hiding this comment.
"not either 100 nor 50"?
| assert_equals( f_s, initial_f_s, "the rlh unit on the root element's font-size property uses font metrics corresponding to the initial values of the font or line-height properties"); | ||
|
|
||
| }, "rlh in font-size on root"); | ||
| </script> |
There was a problem hiding this comment.
Shouldn't there be a test for what happens if you specify 2lh or 2rlh?
There was a problem hiding this comment.
No sure. In general, having a test checking that the unit can be use with numbers other than 1 sounds sane, but I am not sure it is needed as part of the unit cyclic dependency resolution problem, which is what I am testing here. On top of that, I am a little worried here that I'd easily run into rounding errors on this test if I started doing multiplications, especially given that on the root element, the units refer to the initial values of the properties, which depend on fonts and I have no way to force the initial value to be a round number.
As for testing that the unit in general works and makes sense with numbers other than one, I'm planning to write another series of tests about these units checking that they are sized correctly, now that we have resolved on w3c/csswg-drafts#1253. Would that be enough, or do you think there is a need for this within the cyclic dependency tests?
EDIT: fixed unclear phrasing and typo
There was a problem hiding this comment.
I think you might want a test that checks that, at least, specifying 2lh does something (as opposed to just being ignored by the browser instead of correctly cycle-resolved to 2x the default value). Checking that the height becomes taller than the initial one might be enough (or maybe larger than 1.75x more and smaller than 2.25x less?)
There was a problem hiding this comment.
Actually, that was a good idea, sorry for pushing back initially. Also, while fixing that, I found other problems with the test, and fixed them as well. Can you have a look again?
css/css-values/lh-unit-002.html
Outdated
| aside { | ||
| background: green; | ||
| height: 1em; | ||
| line-height: 42px; /* number doesn't matter, as long as it's not 50 */ |
There was a problem hiding this comment.
"not either 50 nor 100"?
Build PASSEDStarted: 2018-01-12 02:38:48 View more information about this build on: |
|
Thanks! |
Goes along with the spec changes in w3c/csswg-drafts#2115