Ticking request changes until the question in the previous patch is addressed. Also some questions, I don't understand why we need this type defined three types.

servo/components/style/values/computed/font.rs
766	I don't understand, why can't this be `type FontSizeAdjust = GenericFontSizeAdjust<NonNegativeNumber>;`? Then you don't need manual `ToAnimatedValue` etc.
servo/components/style/values/specified/font.rs
720	nit: Turbofish is not needed here, `NonNegative<f32>`
733	Can you use `clone_font_size().used_size` (since presumably that's what you want)? But that makes me think this is not right. `"font-size-adjust"` is one of the early properties and thus gets cascaded early, _before_ doing the min-font-size etc adjustments. Is that what you really want? Should we look at the parent's font size? Is it sound to look at our own given `font-size-adjust` can affect font metrics?
754	Same, why can't this be `GenericFontSizeAdjust<FontSizeAdjustFactor>`?

This revision now requires changes to proceed.Jul 6 2023, 9:09 AM

jfkthame added inline comments.Jul 14 2023, 3:06 PM

servo/components/style/values/computed/font.rs
766	I don't remember the exact details, but I had trouble getting it to build... something was getting confused about types.... but having gone back over it, this time things seem to be working OK, so I think we could indeed revert to that.
servo/components/style/values/specified/font.rs
733	Yeah, I think this is a problem. While this implementation passes the (limited) current tests, it's not actually right; it ends up resolving `from-font` based on the parent element's font, which is wrong. What we want is the ex-height (or whatever metric is chosen) ratio from what's going to be the current element's font. What I'm currently thinking is that we could handle this in cascade.rs, as part of fixup_font_stuff. I'll see if I can make that work.
754	As above, at some point along the way I was running into compilation failures that I couldn't figure out at the time, but now it seems OK.

jfkthame requested review of this revision.Jul 19 2023, 10:38 AM

jfkthame updated this revision to Diff 744120.

jfkthame edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B560714: Diff 744120.Jul 19 2023, 10:38 AM

jfkthame added inline comments.Jul 19 2023, 11:18 AM

servo/components/style/values/specified/font.rs
733	An example of this is https://codepen.io/jfkthame/pen/NWEMaaG, which shows incorrect behavior with the old version of the patch (the result of font-size-adjust from-font depends on the font of the parent, instead of resolving based on the metrics of the element's own font). With the revised version, this works as expected. (FWIW, it fails in current Safari Tech Preview; I'll add a WPT version of the test to show this.)

jfkthame updated this revision to Diff 744159.Jul 19 2023, 12:32 PM

Harbormaster completed remote builds in B560741: Diff 744159.Jul 19 2023, 12:32 PM

jfkthame added a child revision: D183984: Bug 1708240 - Add a WPT reftest to check that from-font is resolved using metrics of the current element's font, not the parent's. r=#style.Jul 19 2023, 12:32 PM

This is fine generally (but worth waiting for the spec issue to be resolved probably?)

servo/components/style/properties/cascade.rs
1292	maybe `resolve_font_size_adjust_from_font_if_needed`?
servo/components/style/values/computed/font.rs
762	Oh, ok, so you're preserving the `FromFont` in the computed value, but fixing up during the cascade. That's... a bit weird, but seems ok I think.
servo/components/style/values/specified/font.rs
706	Why is this needed? Could you leave a comment?
723	I think you don't need the `_cloned` now.

This revision now requires changes to proceed.Jul 26 2023, 10:54 AM

Herald added a subscriber: zrhoffman. · View Herald TranscriptJul 26 2023, 10:54 AM

jfkthame marked 2 inline comments as done.Jul 27 2023, 4:55 PM

jfkthame added inline comments.

servo/components/style/values/specified/font.rs
706	Ah, right - not needed, that was residual from earlier versions. Removed.
723	Without it, the compiler complains at `SelectorParseErrorKind::UnexpectedIdent(ident)` below. (But maybe there's a better way to do it?)

jfkthame requested review of this revision.Jul 27 2023, 4:56 PM

jfkthame updated this revision to Diff 747682.

jfkthame marked an inline comment as done.

Harbormaster completed remote builds in B563777: Diff 747682.Jul 27 2023, 4:56 PM

emilio added inline comments.Jul 27 2023, 4:56 PM

servo/components/style/values/specified/font.rs
723	Ah yeah, you just need to use `ident.clone()` only in the error path.

In D182451#6101020, @emilio wrote:

This is fine generally (but worth waiting for the spec issue to be resolved probably?)

Agreed -- as it happens Chris has just chimed in there, and confirmed the expected behavior, so we're good. But this can wait till post-soft-freeze anyhow.

jfkthame updated this revision to Diff 747683.Jul 27 2023, 5:02 PM

Harbormaster completed remote builds in B563778: Diff 747683.Jul 27 2023, 5:02 PM

emilio accepted this revision as: emilio.Jul 27 2023, 6:13 PM

emilio added a project: testing-approved.

emilio removed a reviewer: Restricted Project.

emilio added inline comments.

servo/components/style/properties/cascade.rs
1234	You could move this afterwards right before using it, correct? No big deal either way.
1270	I guess you could've used `None` to fallback to ascent, and `Some` for other stuff, but seems fine.
servo/components/style/values/specified/font.rs
709	Maybe point to the draft?