feat: add font feature settings support #2
Conversation
Added font feature settings to TextStyle Adapted example to new structure
Implements diegomura#2155. Adds the fontFeatureSettings style property for CSS equivalent support. Takes a list of feature tags which appends to the default set, or an object to turn on/off individual features, as supported by fontkit. Allows users to apply font features like tabular numbers, fractions, alternate glyphs, control over ligatures, etc.
WalkthroughThis update introduces support for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExampleComponent
participant Renderer
participant Layout
participant TextKit
participant FontEngine
User->>ExampleComponent: Selects Font Feature Settings example
ExampleComponent->>Renderer: Renders Document with fontFeatureSettings
Renderer->>Layout: Processes styles, inherits fontFeatureSettings
Layout->>TextKit: Passes attributed text with fontFeatureSettings as features
TextKit->>FontEngine: Layouts glyphs using features
FontEngine-->>TextKit: Returns glyphs with applied font features
TextKit-->>Renderer: Renders text with advanced font features
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/stylesheet/src/types.tsOops! Something went wrong! :( ESLint: 9.20.0 Error: The 'jiti' library is required for loading TypeScript configuration files. Make sure to install it. packages/textkit/src/types.tsOops! Something went wrong! :( ESLint: 9.20.0 Error: The 'jiti' library is required for loading TypeScript configuration files. Make sure to install it. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
packages/layout/src/svg/inheritProps.ts (1)
5-34: 🛠️ Refactor suggestionDuplicate key & merge semantics need a follow-up
opacityappears twice (lines 10 and 28).
Removing the duplicate reduces noise during diff reviews and avoids accidental future edits that touch only one occurrence.Newly-added
fontFeatureSettingsis simply overridden downstream; inheriting an array or object will replace, not merge, parent values. For example a parent enablingtnumand a child enablingligaresults in onlyliga. Consider extending merge logic (similar totextDecoration) so both arrays & object values are merged.- 'opacity', + 'opacity', 'textAnchor', … - 'opacity',packages/layout/src/steps/resolveInheritance.ts (1)
38-51: 🛠️ Refactor suggestion
fontFeatureSettingsshould merge, not clobberJust like
textDecoration, multiple ancestors might want to contribute different feature tags. A minimal merge strategy could:• For array values –
Array.from(new Set([...inherited, ...value]))
• For object values –{ ...inherited, ...value }case 'fontFeatureSettings': { if (Array.isArray(value) && Array.isArray(inheritedValue)) { return Array.from(new Set([...inheritedValue, ...value])); } if ( typeof value === 'object' && typeof inheritedValue === 'object' && value && inheritedValue ) { return { ...inheritedValue, ...value }; } return value; }This avoids silent loss of ancestor settings and mirrors CSS cascade behaviour.
🧹 Nitpick comments (7)
packages/stylesheet/src/types.ts (1)
324-332: Consider surfacing a stronger type for feature tags
fontFeatureSettings?: string[] | Record<string, boolean>is flexible but very loose – nothing prevents typos such as['smcp ', 'tble']. A branded‐string/enum union for well-known tags (e.g.'smcp' | 'c2sc' | 'tnum' …) plus a fallbackstringwould give consumers intellisense without sacrificing openness.- fontFeatureSettings?: string[] | Record<string, boolean>; + /** Four-letter OpenType tag(s) – cf. https://learn.microsoft.com/typography/opentype/spec/featuretags */ + fontFeatureSettings?: (KnownFontFeature | string)[] | Record<KnownFontFeature | string, boolean>; + +export type KnownFontFeature = + | 'liga' | 'dlig' | 'smcp' | 'c2sc' | 'tnum' | 'pnum' | 'frac' /* … add as needed */;.changeset/six-paws-tie.md (1)
7-8: Tiny wording nit – “customise” → “customize” (American English)The repo docs mostly use American spelling. Aligning the changeset keeps release notes consistent.
-Add support for fontFeatureSettings to customise ligatures, tabular number display, and other font features +Add support for fontFeatureSettings to customize ligatures, tabular number display, and other font featurespackages/examples/vite/src/examples/index.ts (1)
5-7: Import & array order drift – keep examples alphabetically sorted to avoid merge noiseThe new entries break the previous (rough) alphabetical order of both the
importsection and theEXAMPLESarray. Keeping them ordered avoids future merge conflicts and makes manual scanning easier.-import fontFeatureSettings from './font-feature-settings'; -import forms from './forms'; +import fontFeatureSettings from './font-feature-settings'; +import forms from './forms'; // ← move back above `fontFeatureSettings` to restore orderLikewise in the
EXAMPLESarray, placefontFeatureSettingsnext tofontFamilyFallback.Also applies to: 30-32, 45-47
packages/layout/src/text/getAttributedString.ts (1)
50-51: DefaultfontFeatureSettingstoundefinedto avoid writing empty arrays/objects into every fragment-const { ..., fontFeatureSettings, ... } = instance.style; +const { ..., fontFeatureSettings, ... } = instance.style; ... - features: fontFeatureSettings, + features: fontFeatureSettings ?? undefined,This keeps the fragment payload minimal and avoids the “disable all features” edge-case mentioned earlier.
Also applies to: 104-105
packages/examples/vite/src/index.tsx (1)
3-4: Nit: follow import grouping guidelinesProject files are usually imported after external libs.
Swap the two imports to keep the existing pattern:-import { PDFViewer } from '@rpdf/renderer'; -import React, { useEffect, useState } from 'react'; +import React, { useEffect, useState } from 'react'; +import { PDFViewer } from '@rpdf/renderer';packages/examples/vite/src/examples/font-feature-settings/index.jsx (2)
24-37: Combine style helpers to avoid duplicate objectsYou can DRY the Rubik/Roboto mixins:
- rubik: { fontFamily: 'Rubik' }, - roboto: { fontFamily: 'Roboto' }, + fontRubik: { fontFamily: 'Rubik' }, + fontRoboto: { fontFamily: 'Roboto' },Not mandatory, but reduces bundle size when many examples accumulate.
40-48: Register fonts once per bundlePlacing
Font.registerin module scope means every example import re-registers the font.
Move the registration to a shared “fonts.ts” loaded once, or guard withFont.getRegisteredFonts().has('Rubik').
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/six-paws-tie.md(1 hunks)packages/examples/vite/src/examples/font-feature-settings/index.jsx(1 hunks)packages/examples/vite/src/examples/index.ts(2 hunks)packages/examples/vite/src/index.tsx(3 hunks)packages/layout/src/steps/resolveInheritance.ts(1 hunks)packages/layout/src/svg/inheritProps.ts(1 hunks)packages/layout/src/text/getAttributedString.ts(2 hunks)packages/layout/tests/steps/resolveInhritance.test.ts(1 hunks)packages/stylesheet/src/types.ts(1 hunks)packages/textkit/src/layout/generateGlyphs.ts(2 hunks)
🔇 Additional comments (2)
packages/layout/tests/steps/resolveInhritance.test.ts (1)
178-181: 👍 Nice test – covers the new inheritance path
Nothing to add; this ensures the property behaves like the other inheritable text styles.packages/textkit/src/layout/generateGlyphs.ts (1)
54-59: Safe-guard whenfeaturesis falsyFontkit allows
undefined, but passing an empty array/object enables no features, overriding the font defaults.
Consider normalising:-const glyphRun = font[0].layout(runString, features, undefined, undefined, 'ltr'); +const normalisedFeatures = + Array.isArray(features) && features.length === 0 + ? undefined + : (features as any) ?? undefined; + +const glyphRun = font[0].layout(runString, normalisedFeatures, undefined, undefined, 'ltr');
| const { font, features } = attributes; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
features lacks a type on Run.attributes
features is now destructured but Run (and AttributedStringRun upstream) has no field for it, so attributes.features is typed as any.
Add the field in the declaration (string[] | Record<string, number> | undefined) to keep strict TS projects compiling.
🤖 Prompt for AI Agents
In packages/textkit/src/layout/generateGlyphs.ts around lines 45 to 46, the
destructured `features` from `attributes` is typed as `any` because the `Run`
and `AttributedStringRun` types do not declare a `features` field. To fix this,
add a `features` field to the `Run` and `AttributedStringRun` type declarations
with the type `string[] | Record<string, number> | undefined` to ensure proper
typing and compatibility with strict TypeScript settings.
…x.jsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/main.yml (1)
114-114: Consistency in job name formatting
The E2E job name currently uses a comma to separate parameters, while other jobs (e.g., unit tests) use a semicolon. For clarity and consistency, consider updating it to:-name: 'E2E tests (Node.js ${{ matrix.node_version }}, ${{ matrix.cjs_or_esm }})' +name: 'E2E tests (Node.js ${{ matrix.node_version }}; module system: ${{ matrix.cjs_or_esm }})'packages/textkit/src/types.ts (2)
3-3: Check cross-package dependency layering
import type { FontFeatureSettings } from '@rpdf/stylesheet';introduces a direct dependency from textkit → stylesheet.
Iftextkitis intended to remain a low-level, rendering-agnostic package, verify that depending on stylesheet types does not break your internal layering rules or create cyclic deps in the published packages graph. If that coupling is acceptable, theimport typequalifier is perfect to keep the dependency compile-time only.
55-55: Type refinement looks good – add a short docstringReplacing
unknown[]with the strongly-typedFontFeatureSettingsgreatly improves safety and DX – nice!
Consider adding a brief comment (e.g. “OpenType feature settings, mirrors CSSfont-feature-settings”) above this line so future contributors immediately understand the expected shape without jumping to stylesheet types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/six-paws-tie.md(1 hunks).github/workflows/main.yml(1 hunks)packages/stylesheet/src/types.ts(2 hunks)packages/textkit/src/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/six-paws-tie.md
- packages/stylesheet/src/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/textkit/src/types.ts (1)
packages/stylesheet/src/types.ts (1)
FontFeatureSettings(351-353)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Run unit tests (Node.js 18; React 18)
- GitHub Check: Lint
- GitHub Check: Run unit tests (Node.js 18; React 17)
- GitHub Check: Check size
- GitHub Check: Run unit tests (Node.js 21; React 18)
- GitHub Check: Run unit tests (Node.js 20; React 18)
- GitHub Check: Run unit tests (Node.js 20; React 17)
- GitHub Check: Run unit tests (Node.js 18; React 16)
- GitHub Check: Run unit tests (Node.js 21; React 16)
- GitHub Check: Run unit tests (Node.js 21; React 17)
- GitHub Check: Run unit tests (Node.js 18; React 17)
- GitHub Check: Run unit tests (Node.js 20; React 16)
- GitHub Check: Run unit tests (Node.js 18; React 18)
- GitHub Check: E2E tests (Node.js 21, cjs)
- GitHub Check: E2E tests (Node.js 20, cjs)
- GitHub Check: E2E tests (Node.js 18, esm)
- GitHub Check: Lint
- GitHub Check: Check size
MendyLanda
force-pushed
the
font-feature-settings
branch
from
15b637a to
0bfb69c
Compare
Implements diegomura#2155. Adds the fontFeatureSettings style property for CSS equivalent support. Takes a list of feature tags which appends to the default set, or an object to turn on/off individual features, as supported by fontkit.
Allows users to apply font features like tabular numbers, fractions, alternate glyphs, control over ligatures, etc.
Thanks to @stefanwittwer #2740
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style