Skip to content

LT-22324: add OpenType font feature options - #870

Merged
johnml1135 merged 7 commits into
mainfrom
LT-22324
Jun 8, 2026
Merged

LT-22324: add OpenType font feature options#870
johnml1135 merged 7 commits into
mainfrom
LT-22324

Conversation

@johnml1135

@johnml1135johnml1135 commented May 6, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR contains only the direct OpenType font feature work from LT-22324 after splitting the broader branch into separate reviewable pieces.

Included

  • managed font feature settings model and dialog/UI integration
  • native Uniscribe/OpenType rendering updates
  • direct managed and unmanaged test coverage for font feature behavior
  • OpenSpec docs, research, tasks, manual testing, and evidence for the font-options change

Excluded

  • agentic support, MCP, and screenshot-skill infrastructure changes
  • general render benchmark and support-tooling work now tracked in PR Add WinForms MCP and skills #869

This change is Reviewable

CopilotAI review requested due to automatic review settings May 6, 2026 20:02

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds OpenType font-feature support to FieldWorks’ existing WinForms/Views stack (LT-22324), including a renderer-neutral feature string model, UI integration, native Uniscribe OpenType shaping, and layered test coverage plus OpenSpec/docs artifacts.

Changes:

  • Introduces renderer-neutral tag=value feature parsing/normalization and integrates it into WinForms font-feature UI.
  • Adds native Uniscribe OpenType shaping/placing path driven by run feature strings, plus deterministic native tests using a committed SIL font fixture.
  • Adds managed cache-identity tests and test-only HarfBuzzSharp/SkiaSharp comparison coverage; includes OpenSpec requirements/research/manual evidence docs.

Reviewed changes

Copilot reviewed 33 out of 38 changed files in this pull request and generated 7 comments.

Show a summary per file
FileDescription
openspec/changes/add-opentype-font-features/views-migration-matrix.mdAdds a Views subsystem inventory/migration matrix to guide phased migration and feature parity work.
openspec/changes/add-opentype-font-features/tasks.mdDocuments implementation/task checklist for Phase 1 OpenType features and associated testing.
openspec/changes/add-opentype-font-features/specs/font-feature-settings/spec.mdDefines requirements for feature independence from Graphite and renderer-neutral storage/application.
openspec/changes/add-opentype-font-features/specs/architecture/ui-framework/winforms-patterns/spec.mdSpecifies WinForms composition/localization requirements for feature controls.
openspec/changes/add-opentype-font-features/specs/architecture/ui-framework/views-rendering/spec.mdSpecifies Views rendering + cache identity requirements around feature strings.
openspec/changes/add-opentype-font-features/specs/architecture/testing/test-strategy/spec.mdDefines layered test strategy including visual baselines and cross-renderer comparisons.
openspec/changes/add-opentype-font-features/research.mdCaptures research notes and external references (Uniscribe OT APIs, HarfBuzz, Skia/Avalonia).
openspec/changes/add-opentype-font-features/proposal.mdSummarizes the change scope, goals, and impacted areas for LT-22324.
openspec/changes/add-opentype-font-features/manual-testing.mdRecords manual WinApp/WinForms MCP evidence steps and screenshots for the UI changes.
openspec/changes/add-opentype-font-features/design.mdDocuments design decisions (renderer-neutral model, Uniscribe OT, provider UI seam, test tooling).
openspec/changes/add-opentype-font-features/.openspec.yamlAdds OpenSpec metadata for the change set.
Src/views/lib/UniscribeSegment.cppAdds OT feature parsing and optional ScriptShape/Place OpenType path during shaping/placing.
Src/views/Test/TestViews.vcxproj.filtersAdds Charis SIL test font fixture files to VS filters.
Src/views/Test/TestViews.vcxprojCopies Charis SIL font fixture beside TestViews.exe for deterministic native tests.
Src/views/Test/TestUniscribeEngine.hAdds deterministic native tests for OT feature metrics/pixel deltas and state switching.
Src/views/Test/TestData/Fonts/CharisSIL-5.000/README.txtAdds redistributable font README to support deterministic fixture usage.
Src/views/Test/TestData/Fonts/CharisSIL-5.000/OFL.txtAdds SIL OFL license text for the committed test font fixture.
Src/views/Test/RenderEngineTestBase.hExtends TxtSrc to carry szFontVar feature strings into render props for tests.
Src/FwCoreDlgs/FwCoreDlgsTests/FwFontDialogTests.csAdds test ensuring OpenType features round-trip and normalize through font dialog save.
Src/FwCoreDlgs/FwCoreDlgControls/FwCoreDlgControlsTests/TestFontFeaturesButton.csAdds tests for renderer-neutral tag emission and normalization behavior in the control.
Src/FwCoreDlgs/FwCoreDlgControls/FwCoreDlgControlsTests/FwFontTabTests.csAdds style/font-tab tests ensuring OpenType feature strings round-trip and normalize.
Src/FwCoreDlgs/FwCoreDlgControls/FwCoreDlgControlsTests/FwAttributesTests.csAdds test verifying attributes control returns normalized feature strings and inheritance status.
Src/FwCoreDlgs/FwCoreDlgControls/FontFeaturesButton.csRefactors button around provider seam; adds OpenType discovery via GDI table parsing; normalizes input.
Src/FwCoreDlgs/FwCoreDlgControls/DefaultFontsControl.resxUpdates help text and group label to generic “Font Options” wording.
Src/FwCoreDlgs/FwCoreDlgControls/DefaultFontsControl.csDecouples UI enablement from Graphite checkbox; wires provider preference and setup flow.
Src/Common/SimpleRootSite/SimpleRootSiteTests/RenderEngineFactoryTests.csAdds tests for feature normalization propagation and cache identity behavior.
Src/Common/SimpleRootSite/RenderEngineFactory.csAdds feature strings into renderer cache key and normalizes/copies features into graphics props.
Src/Common/RenderVerification/RenderComparisonTests/RenderComparisonTests.csprojAdds a test-only comparison project with HarfBuzzSharp/SkiaSharp dependencies.
Src/Common/RenderVerification/RenderComparisonTests/HarfBuzzSkiaComparisonTests.csAdds HarfBuzz shaping-data toggle test and a basic Skia “non-blank render” comparison test.
Src/Common/FwUtils/FwUtilsTests/FontFeatureSettingsTests.csAdds unit tests for parsing/normalizing renderer-neutral feature strings.
Src/Common/FwUtils/FontFeatureSettings.csIntroduces renderer-neutral parser/normalizer and validity checks for OpenType tags.
Docs/opentype-font-features.mdDocuments feature-string model, UI usage, renderer boundaries, and export notes.
Directory.Packages.propsAdds centralized package versions for HarfBuzzSharp and SkiaSharp (test infrastructure).

Comment threadSrc/Common/SimpleRootSite/RenderEngineFactory.cs Outdated
Comment threadSrc/views/lib/UniscribeSegment.cpp Outdated
Comment threadSrc/FwCoreDlgs/FwCoreDlgControls/FontFeaturesButton.cs
Comment threadSrc/FwCoreDlgs/FwCoreDlgControls/FontFeaturesButton.cs
Comment threadSrc/FwCoreDlgs/FwCoreDlgControls/FontFeaturesButton.cs
Comment threadSrc/views/Test/TestUniscribeEngine.h Outdated
Comment threadSrc/FwCoreDlgs/FwCoreDlgControls/DefaultFontsControl.cs Outdated
@johnml1135
johnml1135 changed the base branch from main to 001-render-speedupMay 7, 2026 19:29
Base automatically changed from 001-render-speedup to mainMay 8, 2026 20:01
@johnml1135

Copy link
Copy Markdown
ContributorAuthor

Validated the Word export changes on this branch.

How it was validated:

  • Added an automated regression in LcmWordGeneratorTests that verifies the Normal Word style picks up the writing-system default font (Charis SIL) and emits the expected OpenType stylistic sets (ss11 and ss12).
  • Added two gated manual artifact tests in LcmWordGeneratorTests:
    • baseline export with no font options
    • export with ss11=1,ss12=1
  • Ran the manual tests locally with FW_RUN_MANUAL_DOCX_EXPORT_TESTS=1, which regenerated:
    • Output/Debug/ManualDocxArtifacts/charis-baseline-no-font-options.docx
    • Output/Debug/ManualDocxArtifacts/charis-ss11-ss12.docx
  • Verified the generated DOCX packages directly by inspecting word/styles.xml:
    • the baseline export has no w14:styleSet entries
    • the feature export contains w14:styleSet ids 11 and 12
  • Also updated the styles part so the saved package carries the required mc / w14 namespace declarations and Ignorable="w14", ensuring the Word typography markup survives serialization.

I do not have Microsoft Word installed on this machine, so the validation here is at the DOCX/package level rather than a final visual check in Word itself. But the exported document now contains the expected WordprocessingML that Word uses for these OpenType stylistic sets.

@johnml1135
johnml1135force-pushed the LT-22324 branch 2 times, most recently from 11e413a to a9d4f58CompareMay 11, 2026 17:07
@johnml1135

Copy link
Copy Markdown
ContributorAuthor

Follow-up review pass is pushed in db8a0f108.

Addressed in this pass:

  • RenderEngineFactory: default writing-system features now only flow in when the run is actually using <default font>, and numeric legacy Graphite feature strings are preserved instead of being normalized away.
  • FontFeaturesButton: the OpenType menu path no longer routes renderer-neutral tags through GraphiteFontFeatures.ConvertFontFeatureCodesToIds(...); it now converts valid OpenType tags directly in a renderer-neutral helper.
  • DefaultFontsControl: feature discovery refreshes through a single RefreshFeatureContext(...) path instead of double-running SetupFontFeatures().
  • TestUniscribeEngine: fixed the test helper GDI leak by releasing the desktop DC and restoring the original bitmap selection before cleanup.
  • Native UniscribeSegment: also fixed two additional local issues from review/debugging that were not in the Copilot threads: the OpenType path now copies SCRIPT_VISATTR data back into uri.prgsva, and the feature parser now guards against numeric overflow.
  • Export/test follow-ups: CSS now reuses FontFeatureSettings.Parse(...) so it only emits valid OpenType tags, DOCX numbering parts now carry the same mc/w14 compatibility declarations as styles, and RenderComparisonTests is now included in FieldWorks.sln so solution restore/build stays aligned with the traversal build.

Left open intentionally for a later pass:

  • deriving Uniscribe script/langsys tags instead of the current latn / DFLT fallback
  • localizing the OpenType provider Off / On labels
  • caching OpenType feature discovery results across repeated font selections

Validation run on this branch after the new commit:

  • ./test.ps1 -SkipManaged -TestProject TestViews -StartedBy agent
  • ./test.ps1 -SkipNative -TestProject FwCoreDlgControlsTests -StartedBy agent
  • ./test.ps1 -SkipNative -TestProject xWorksTests -StartedBy agent
  • CI: Full local check

@github-actions

github-actionsBot commented May 11, 2026

Copy link
Copy Markdown

NUnit Tests

1 files ± 0 1 suites ±0 9m 15s ⏱️ - 2m 33s
4 249 tests +39 4 176 ✅ +36 73 💤 +3 0 ❌ ±0 
4 258 runs +39 4 185 ✅ +36 73 💤 +3 0 ❌ ±0 

Results for commit 31b0ee6. ± Comparison against base commit 53d5dbb.

This pull request removes 2 and adds 41 tests. Note that renamed tests count towards both.
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RenderEngineFactoryTests ‑ get_Renderer_Graphite
SIL.FieldWorks.Common.RootSites.SimpleRootSiteTests.RenderEngineFactoryTests ‑ get_Renderer_Uniscribe
SIL.FieldWorks.Common.FwUtils.FontFeatureSettingsTests ‑ NormalizePreservingLegacy_NormalizesOpenTypeFeatures
SIL.FieldWorks.Common.FwUtils.FontFeatureSettingsTests ‑ NormalizePreservingLegacy_NormalizesOpenTypeFeaturesThatStartWithPunctuation
SIL.FieldWorks.Common.FwUtils.FontFeatureSettingsTests ‑ NormalizePreservingLegacy_PreservesNumericGraphiteFeatureIds
SIL.FieldWorks.Common.FwUtils.FontFeatureSettingsTests ‑ Normalize_ReturnsDeterministicRendererNeutralString
SIL.FieldWorks.Common.FwUtils.FontFeatureSettingsTests ‑ Parse_AcceptsCustomPrintableAsciiTags
SIL.FieldWorks.Common.FwUtils.FontFeatureSettingsTests ‑ Parse_IgnoresInvalidEntries
SIL.FieldWorks.Common.FwUtils.FontFeatureSettingsTests ‑ Parse_LastValueWinsForDuplicateTags
SIL.FieldWorks.Common.FwUtils.FontFeatureSettingsTests ‑ Parse_LogsIgnoredInvalidEntries
SIL.FieldWorks.Common.FwUtils.FontFeatureSettingsTests ‑ Parse_ReturnsNormalizedTagValueSettings
SIL.FieldWorks.Common.RenderVerification.RenderComparisonTests.HarfBuzzSkiaComparisonTests ‑ DrawShapedText_ProducesNonBlankComparisonBitmap
…

♻️ This comment has been updated with latest results.

@johnml1135

Copy link
Copy Markdown
ContributorAuthor

Final OpenType review cleanup is pushed in 314e7f6bd.

Addressed in this pass:

  • Native OpenType shaping now derives script tags through Uniscribe OpenType itemization/font script APIs and derives language-system tags from the writing-system ICU/SLDR locale via Windows LOCALE_SOPENTYPELANGUAGETAG, with font-language validation/fallbacks.
  • OpenType feature runs now bypass the shared analysis/shape cache paths that are not keyed by locale/tag state, keeping ordinary runs on the existing cache path.
  • OpenType provider labels are resource-backed, including Off/On and the common OpenType feature names.
  • OpenType feature discovery now has a small bounded cache keyed from the selected GDI LOGFONT values.
  • Added native coverage for Serbian Cyrillic locl language-system behavior and managed coverage for the feature discovery cache.

Validation:

  • ./build.ps1 passed.
  • Output/Debug/TestViews.exe -v UniscribeEngine passed 6/6.
  • ./test.ps1 -SkipNative -TestProject FwCoreDlgControlsTests -NoBuild passed 45/45.
  • ./test.ps1 -SkipNative -TestProject RootSiteTests -TestFilter "Name=RenderHarness_CapturesSimpleView_ReturnsValidBitmap" -NoBuild passed 1/1.
  • Full ./test.ps1 ran to completion with 4204 passed, 61 skipped, and 1 failed test: ToneParsInvokerTest.
  • I verified ToneParsInvokerTest also fails in a clean baseline worktree at db8a0f108, so it is not introduced by this pass.
  • CI: Full local check passed: commit-message lint and whitespace check reported no problems.

The three remaining Copilot review threads were replied to and resolved after this push.

@johnml1135
johnml1135 requested a review from CopilotMay 12, 2026 13:12

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@jasonleenaylorjasonleenaylor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonleenaylor reviewed 27 files and all commit messages, and made 2 comments.
Reviewable status: 27 of 60 files reviewed, all discussions resolved.


Src/Common/SimpleRootSite/RenderEngineFactory.cs line 39 at r3 (raw file):

/// font.
///</summary>
public IRenderEngine get_Renderer(ILgWritingSystem ws, IVwGraphics vg)

This name slipped through in an earlier PR I guess, this is not the correct C# style for a method, it looks more like the c++ expectation or COM naming patterns.


Src/xWorks/xWorksTests/XhtmlDocViewTests.cs line 46 at r3 (raw file):

 {
using (var docView =newTestXhtmlDocView())
{

A comment explaining why this test failure is important would be good for maintainence.

@johnml1135johnml1135 left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonleenaylor Fixed in f606e05.

  • RenderEngineFactory now exposes GetRenderer(...) for C# callers/tests, while keeping IRenderEngineFactory.get_Renderer(...) as an explicit COM-interface adapter for native/generated-interface callers.
  • Added a comment to XhtmlDocView_ImplementsRefreshableRoot explaining that it protects the FwXWindow refresh traversal path for XHTML dictionary views.

Validation:

  • ./test.ps1 -SkipNative -TestProject SimpleRootSiteTests -TestFilter "FullyQualifiedName~RenderEngineFactoryTests" -NoBuild -StartedBy agent passed 7/7.
  • ./test.ps1 -SkipNative -TestProject xWorksTests -TestFilter "FullyQualifiedName~XhtmlDocView_ImplementsRefreshableRoot" -StartedBy agent passed 1/1.
  • CI: Commit messages passed.

Note: an unfiltered SimpleRootSiteTests run still hits existing Views layout failures in selection tests, so I used the focused fixture run for this review change.

@github-actions

github-actionsBot commented May 22, 2026

Copy link
Copy Markdown

Render comparison artifacts

Render snapshot failures were reported in 75427247010e run 26317352517.1, but the latest run 9a33fd5e8fbe run 27030042749.1 is clean.

This comment will be replaced if a future run produces render snapshot failures again.

@johnml1135

Copy link
Copy Markdown
ContributorAuthor

@jasonleenaylor - what is still needed for this pull request?

@jasonleenaylorjasonleenaylor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonleenaylor reviewed 36 files and all commit messages, and made 2 comments.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on johnml1135).


Src/FwCoreDlgs/FwCoreDlgControls/FontFeaturesButton.cs line 44 at r5 (raw file):

 private IFontFeatureProvider m_featureProvider;
private static readonly TraceSwitch s_openTypeTraceSwitch =newTraceSwitch("FontFeatures.OpenType", "OpenType font feature discovery and provider selection", "Off");

I worry about the discoverability, there are many vestigial debugging tools littered throughout FieldWorks that noone needs or remembers. When we need it we typically end up half way through making a new one before we stumble on the old one. There should be a very specific comment here to explain what tracing this controls and why we need it. Or just rip it out if it has served its purpose.


Src/views/lib/UniscribeSegment.cpp line 366 at r5 (raw file):

}
static FwOpenTypeTag LanguageTagFromLocale(const StrUni & stuIcuLocale)

This name sent me down a rabbit trail. Add a method comment explaining that this is for retrieving an OpenType language-system tag and rename the method to 'OpenTypeLanguageTagFromIcuLocale'

@johnml1135

Copy link
Copy Markdown
ContributorAuthor

@jasonleenaylor Fixed in 31b0ee6.

  • Added a specific comment above FontFeatures.OpenType explaining that the switch is only there for the two OpenType diagnostics it gates today: provider selection and filtering non-user-configurable GSUB/GPOS tags.
  • Renamed LanguageTagFromLocale(...) to OpenTypeLanguageTagFromIcuLocale(...) and added a method comment clarifying that it retrieves the OpenType language-system tag from the ICU locale.

Validation:

  • ./test.ps1 -SkipNative -TestProject FwCoreDlgControlsTests -NoBuild -StartedBy agent passed: 47/47.
  • ./test.ps1 -SkipManaged -TestProject TestViews -StartedBy agent rebuilt Views and TestViews successfully, confirming the native rename compiles cleanly. The suite still hits an existing native assert in Src/Generic/Vector.h:311, which is outside this comment-only/rename-only change.
  • A build-backed managed test run with rebuild was also blocked by an existing ILRepack stack overflow in Src/FwParatextLexiconPlugin/ILRepack.targets, so I used the successful focused -NoBuild rerun after FwCoreDlgControls and FwCoreDlgControlsTests had already built in that attempt.

@jasonleenaylorjasonleenaylor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@jasonleenaylor reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on johnml1135).

@johnml1135
johnml1135 merged commit ea21aab into mainJun 8, 2026
8 checks passed
@johnml1135
johnml1135 deleted the LT-22324 branch June 8, 2026 19:07
johnml1135 added a commit that referenced this pull request Jul 14, 2026
Both are fully implemented and merged (PR #870, PR #906), unrelated
to the Avalonia migration spine; archiving keeps openspec/changes/
scoped to work still in flight.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@johnml1135@jasonleenaylor