spec: improve default proportional font readability (#9953)#10226
spec: improve default proportional font readability (#9953)#10226lonexreb wants to merge 5 commits intowarpdotdev:masterfrom
Conversation
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This spec defines new proportional typography appearance settings, defaults, UI controls, acceptance criteria, and tests for Settings panes and rendered Markdown.
Concerns
- The Linux default relies on Inter while the spec explicitly excludes bundling Inter, so default-install behavior is nondeterministic and may not meet the GitHub-baseline acceptance goal.
- The font-family setting accepts arbitrary strings and syncs globally, but the spec does not define validation, missing-font fallback, cross-platform synced font behavior, or reset/default behavior.
- The line-height setting name is inconsistent with the
proportional_font_*naming pattern used for the other new settings. - Acceptance only checks size 18 even though the behavior contract permits size 24 and the test plan claims max-size coverage.
Verdict
Found: 0 critical, 3 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| - B1. New setting `appearance.proportional_font_family: String` | ||
| (default `"system-ui"` on macOS, `"Segoe UI Variable"` on | ||
| Windows, `"Inter"` then system fallback on Linux). |
There was a problem hiding this comment.
| - B1. New setting `appearance.proportional_font_family: String` | ||
| (default `"system-ui"` on macOS, `"Segoe UI Variable"` on | ||
| Windows, `"Inter"` then system fallback on Linux). | ||
| Same `SyncToCloud::Globally(RespectUserSyncSetting::Yes)` as |
There was a problem hiding this comment.
| `appearance.proportional_font_size: f32` | ||
| (default `14.0`, clamped to `[10.0, 24.0]`). | ||
| - B3. New setting | ||
| `appearance.proportional_line_height: f32` |
There was a problem hiding this comment.
💡 [SUGGESTION] The line-height setting drops the font_ segment used by appearance.proportional_font_family and appearance.proportional_font_size. Use one naming pattern so implementers do not create incompatible setting keys.
| - A2. User can change the family in Settings → Appearance and the | ||
| change applies to Settings panes and rendered Markdown without | ||
| restart. | ||
| - A3. Increasing the size to 18 doesn't break Settings layout |
There was a problem hiding this comment.
Co-Authored-By: Warp <agent@warp.dev>
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines new proportional font family, size, and line-height settings intended to make Settings panes and rendered Markdown more readable.
Concerns
- The target surface is ambiguous: the acceptance criteria mention Settings and Markdown, but the implementation pointer routes through app-wide Appearance state that many UI surfaces consume.
- Size and line-height settings need the same persistence/sync metadata clarity as the family setting.
- The test plan should align with the promised visual comparison and cover clamp boundaries for manually edited or synced values.
Verdict
Found: 0 critical, 1 important, 3 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| - A2. User can change the family in Settings → Appearance and the | ||
| change applies to Settings panes and rendered Markdown without | ||
| restart. |
There was a problem hiding this comment.
Appearance font; clarify the intended surface area and any explicit exclusions so implementation does not unintentionally re-font or resize unrelated UI.
| - B2. New setting | ||
| `appearance.proportional_font_size: f32` | ||
| (default `14.0`, clamped to `[10.0, 24.0]`). | ||
| - B3. New setting | ||
| `appearance.proportional_font_line_height: f32` | ||
| (default `1.55`, clamped to `[1.0, 2.5]`). |
There was a problem hiding this comment.
💡 [SUGGESTION] State the persistence and SyncToCloud behavior for size and line height just like B1 does for family; otherwise implementers may sync only the family or choose inconsistent storage behavior for the other two settings.
| `appearance.proportional_font_line_height: f32` | ||
| (default `1.55`, clamped to `[1.0, 2.5]`). | ||
| - B4. The default values are chosen to match GitHub's typography | ||
| baseline (1.5–1.6 line-height, 14–16px). Verified by side-by-side |
There was a problem hiding this comment.
💡 [SUGGESTION] B4 says this is verified by a side-by-side screenshot in the test plan, but no test-plan item requires that screenshot; add the visual comparison step or remove this claim.
| - T1–T3. Setting round-trips for family / size / | ||
| proportional-font line-height. |
There was a problem hiding this comment.
💡 [SUGGESTION] Add tests for clamp boundaries and manually edited or synced out-of-range values for size and line height; round-trip coverage alone does not verify the [10, 24] and [1.0, 2.5] contracts.
|
Round-1 fixes for oz-for-oss review: (1) Linux default now uses system-available stack (Cantarell/Ubuntu/Liberation Sans) since Inter isn't bundled; (2) added font-family validation rules, missing-font cascade behavior, per-platform sync schema with reset-to-default semantics; (3) renamed |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
Reviewed the spec-only PR for GH-9953. The spec covers the problem, V1 surfaces, new appearance settings, per-platform sync behavior, acceptance criteria, implementation pointers, and test coverage.
Concerns
- The default font stack is internally ambiguous around whether Inter participates in the default fallback order, which can lead to different platform behavior and unstable visual expectations.
- The font-family validation contract allows quoted family names but does not specify canonical serialization or escaping before CSS use.
Security
- The user-supplied and synced font-family value should be rendered from parsed, canonical family tokens rather than reused as a raw CSS string.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| - Linux: a system-available stack — | ||
| `system-ui, "Cantarell", "Ubuntu", "Liberation Sans", | ||
| "DejaVu Sans", "Noto Sans", sans-serif`. | ||
| Inter is preferred where the user has installed it, but is |
There was a problem hiding this comment.
| - B1a. **Validation.** A non-empty user-supplied value is parsed | ||
| as a CSS family-list string. Allowed characters per family: | ||
| alphanumerics, ASCII spaces, hyphens, periods, and underscores. | ||
| Family names containing other characters MUST be quoted with |
There was a problem hiding this comment.
|
Round-2 fixes: (1) per-platform defaults explicitly stated WITHOUT Inter — macOS uses |
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This spec defines new proportional font appearance settings, per-platform defaults, sync behavior, Settings UI controls, and rendering/test expectations.
Concerns
- The font resolution contract treats fallback entries as installed fonts, but the default/user stacks include CSS generic families, leaving cross-platform resolution undefined.
- Canonical serialization has an ambiguous case-normalization requirement, so implementations and tests may disagree on stored values.
Security
- The CSS-injection guard assumes quoted CSS strings prevent
</style>breakouts; that is not safe if CSS is ever written into a style text context. The spec should require CSSOM/style-property assignment or escaping HTML end-tag sequences and test the actual render insertion path.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| rule id (never the raw value). | ||
|
|
||
| - B1b. **Missing-font fallback.** Resolution proceeds left-to-right | ||
| through the parsed family list. If no listed family matches an |
There was a problem hiding this comment.
system-ui, sans-serif): they are not installed font names, so specify whether generic families bypass installed-font matching and are delegated to the platform renderer.
| always `String -> ParsedFamilyList -> CssString`, never | ||
| `String -> CssString`. This eliminates CSS injection vectors | ||
| via crafted family names (e.g., a family name containing | ||
| `</style><script>` cannot break out of the CSS context because |
There was a problem hiding this comment.
<style> text node: the HTML parser can still terminate on </style>. Require CSSOM/style-property assignment or escaping </end-tag sequences, and test that exact render path.
| with an inline error; the previous canonical value is retained. | ||
| - A7. Canonical-form round-trip: writing | ||
| `inter , "Open Sans" ,Arial` (with stray spaces) is stored as | ||
| `Inter, "Open Sans", Arial` (or the user's preferred case). |
There was a problem hiding this comment.
Inter or the user's preferred case; choose one deterministic rule (preserve input case or normalize with explicit rules) so canonical storage and tests cannot diverge.
|
Addressed all 3 important concerns from latest oz-for-oss review: Generic-family resolution defined (important #1)
Walks the macOS Default stack on Linux as a worked example. New test Canonical case-normalization disambiguated (important #2)
Full case-rule table added showing all combinations. A7 rewritten: CSS-injection guard at actual render path (important #3 / security)
Two entry points: |
|
/oz-review |
1 participant
Spec for #9953. New
appearance.proportional_font_{family,size,line_height}settings with GitHub-baseline defaults (system-ui/Segoe UI Variable/Inter, 14px, 1.55). Settings → Appearance subsection with live preview.Closes (spec-only) #9953.