text_primitives: improve OpenType settings CSS parsing #501

waywardmonkeys · 2026-01-05T13:45:16Z

Add structured parsing errors (kind + byte offset/span) for CSS OpenType settings strings.
Rename list parsers to parse_css_list.
Update Parley resolver to use the fallible CSS parser.
Add unit tests covering success cases and key error offsets/spans.

text_primitives/src/tag.rs

taj-p · 2026-01-08T04:17:19Z

text_primitives/src/tag.rs

+    at: usize,
+    span: Option<(usize, usize)>,


Usually I discourage the use of architecture-dependent types like usize to keep behaviour consistent across platforms, but I think it's fine for this type of struct. Curious what your thoughts are.

Anything else would be super annoying in terms of casts.

taj-p · 2026-01-08T04:24:33Z

text_primitives/src/tag.rs

+        while pos < self.len {
            let ch = self.source[pos];
-            ch.is_ascii_whitespace() || ch == b','
-        } {
-            pos += 1;
+            if ch.is_ascii_whitespace() || ch == b',' {
+                pos += 1;
+            } else {
+                break;
+            }
        }


I believe these lines allow an arbitrary number and order of and ,. That's technically not allowed in the CSS spec IIUC, but I understand if you'd prefer to keep the leniency.

i.e., this test passes:

#[test] fn parse_feature_settings_css_list_ok() { let parsed: Result<Vec<_>, _> = Setting::<u16>::parse_css_list(r#""liga" on,,, , ,,, 'kern', "dlig" off, "salt" 3,"#).collect(); let settings = parsed.unwrap(); assert_eq!(settings.len(), 4); assert_eq!(settings[0].tag, Tag::parse("liga").unwrap()); assert_eq!(settings[0].value, 1); assert_eq!(settings[1].tag, Tag::parse("kern").unwrap()); assert_eq!(settings[1].value, 1); assert_eq!(settings[2].tag, Tag::parse("dlig").unwrap()); assert_eq!(settings[2].value, 0); assert_eq!(settings[3].tag, Tag::parse("salt").unwrap()); assert_eq!(settings[3].value, 3); }

Better to start out lenient and make it less lenient if needed in the future. Working on a revision.

Okay, I've pushed a commit that addresses this, adds more tests, and hardens some other stuff in parsing.

waywardmonkeys · 2026-01-08T06:13:53Z

Only partially related to this, but ... would be good to have something, somewhere for handling an equivalent of the font-variant family of CSS properties and converting them into this sort of feature / setting list. (But not as part of this PR.)

dfrg

Generally LGTM with a couple questions.

dfrg · 2026-01-08T11:20:58Z

parley/src/resolve/mod.rs

                self.tmp_variations.clear();
                self.tmp_variations
-                    .extend(FontVariation::parse_list(source));
+                    .extend(FontVariation::parse_css_list(source).map_while(Result::ok));


Should this be filter_map instead? Seems useful to skip invalid entries when possible.

I'll just give the Codex response here which I think I agree with:

filter_map(Result::ok) only helps if the iterator can continue producing items after an error. Our parse_css_list iterators don’t recover: on the first Err they set done = true and terminate. In that world:

map_while(Result::ok) = “take items until first error”

filter_map(Result::ok) = “drop the error, but there are no later items anyway”

So switching to filter_map wouldn’t actually “skip invalid entries when possible” today; it would just slightly change the signal of intent (and arguably mislead future readers into thinking we do recovery).

If the intent is “stop at first parse error, keep earlier valid ones” (matching the old take_while(ok) behavior), map_while(Result::ok) is the clearer expression.
If the intent is “try to salvage later entries”, we’d need to change the parser to recover and keep iterating; then filter_map would make sense.

dfrg · 2026-01-08T11:24:46Z

text_primitives/src/tag.rs

+
 /// A single OpenType setting (tag + value).
 #[derive(Clone, Copy, PartialEq, Debug)]
 pub struct Setting<T> {


What do you think about hiding this and just exposing concrete FontVariation and FontFeature types instead? I’m trying to think of a case where we want to handle these generically and none comes to mind.

I've done this as a separate commit in the series ... it ended up being sizable once I took it to the logical endpoint. Or what I think is the logical endpoint.

dfrg · 2026-01-08T11:27:29Z

Only partially related to this, but ... would be good to have something, somewhere for handling an equivalent of the font-variant family of CSS properties and converting them into this sort of feature / setting list. (But not as part of this PR.)

Seems like a reasonable thing to include.

nicoburns · 2026-01-08T12:21:48Z

I feel like all parsing in the text_primitives crate ought to be gated behind a parsing (or whatever) feature. But these specific changes LGTM.

- Add structured parsing errors (kind + byte offset/span) for CSS OpenType settings strings. - Rename list parsers to `parse_css_list`. - Update Parley resolver to use the fallible CSS parser. - Add unit tests covering success cases and key error offsets/spans.

Co-authored-by: Taj Pereira <[email protected]>

- Make `ParseCssList::next` strict about separators (no ,, / leading comma) - Improve `u16` feature value error classification (`InvalidSyntax` vs `OutOfRange`) - Add regression tests for separator “soup” and value errors

…e/variation types * Replace `Setting<T>` with `FontFeature`/`FontVariation` and export them from `text_primitives` * Add `FontVariations`/`FontFeatures` (`Source`/`List`) wrappers and make them `Into<StyleProperty>` * Add ergonomic constructors: `empty()` and `From<&[T]>`/`From<&[T; N]>` for list usage * Update resolve logic and tests to the new API surface

waywardmonkeys · 2026-01-09T06:52:39Z

Only partially related to this, but ... would be good to have something, somewhere for handling an equivalent of the font-variant family of CSS properties and converting them into this sort of feature / setting list. (But not as part of this PR.)

Seems like a reasonable thing to include.

I've written something for this ... but it is also a fair bit of stuff. I think I'd rather submit as a followup.

waywardmonkeys · 2026-01-09T06:58:11Z

I've added the FeatureSpec stuff as a commit in this series ... but if there's a lot of work to be done to it, then I'm happy to pull it out and make it a separate PR (or potentially drop it).

dfrg · 2026-01-09T21:44:36Z

I've added the FeatureSpec stuff as a commit in this series ... but if there's a lot of work to be done to it, then I'm happy to pull it out and make it a separate PR (or potentially drop it).

My gut feeling is that these don't quite fit this crate, at least as designed. If they were a direct mapping of the various CSS font-variant- properties then I think they would make more sense.

Browsers are also capable of simulating at least sub, super and small-caps so a layout engine should probably be able to intercept these and decide how to proceed rather than just turning them into OpenType features.

If you don't mind, my preference would be to pull these out into a separate PR for further discussion.

waywardmonkeys · 2026-01-10T00:57:29Z

I agree. I do want to land this though with the other commits here, including removing the public Setting.

dfrg · 2026-01-10T01:24:08Z

Good to go!

waywardmonkeys requested review from dfrg and taj-p January 5, 2026 13:45

waywardmonkeys mentioned this pull request Jan 5, 2026

Add text_style + styled_text crates (CSS-like resolution + document blocks) #495

Open

waywardmonkeys requested a review from DJMcNab January 5, 2026 13:49

taj-p approved these changes Jan 8, 2026

View reviewed changes

waywardmonkeys force-pushed the opentype-settings-parse branch from df22fc3 to 60373c0 Compare January 8, 2026 06:12

dfrg approved these changes Jan 8, 2026

View reviewed changes

waywardmonkeys and others added 4 commits January 9, 2026 10:41

text_primitives: Mark some functions as const fn

48dba59

Co-authored-by: Taj Pereira <[email protected]>

text-primitives: Improve feature settings parsing

e0e0fae

- Make `ParseCssList::next` strict about separators (no ,, / leading comma) - Improve `u16` feature value error classification (`InvalidSyntax` vs `OutOfRange`) - Add regression tests for separator “soup” and value errors

waywardmonkeys force-pushed the opentype-settings-parse branch from 60373c0 to 7508192 Compare January 9, 2026 04:00

waywardmonkeys force-pushed the opentype-settings-parse branch 3 times, most recently from 0c4fdae to 49cd65d Compare January 9, 2026 07:16

waywardmonkeys force-pushed the opentype-settings-parse branch from 49cd65d to 7508192 Compare January 10, 2026 01:58

waywardmonkeys enabled auto-merge January 10, 2026 02:00

waywardmonkeys added this pull request to the merge queue Jan 10, 2026

Merged via the queue into linebender:main with commit b4dc8bd Jan 10, 2026
48 checks passed

waywardmonkeys deleted the opentype-settings-parse branch January 10, 2026 02:07

text_primitives: improve OpenType settings CSS parsing #501

text_primitives: improve OpenType settings CSS parsing #501

Conversation

waywardmonkeys commented Jan 5, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taj-p Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waywardmonkeys commented Jan 8, 2026

Uh oh!

dfrg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfrg commented Jan 8, 2026

Uh oh!

nicoburns commented Jan 8, 2026

Uh oh!

waywardmonkeys commented Jan 9, 2026

Uh oh!

waywardmonkeys commented Jan 9, 2026

Uh oh!

dfrg commented Jan 9, 2026

Uh oh!

waywardmonkeys commented Jan 10, 2026

Uh oh!

dfrg commented Jan 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

taj-p Jan 8, 2026 •

edited

Loading

dfrg commented Jan 10, 2026 •

edited

Loading