diff --git a/Src/FwCoreDlgs/FwCoreDlgControls/StyleInfo.cs b/Src/FwCoreDlgs/FwCoreDlgControls/StyleInfo.cs index d6a4f85833..0b7fb7dbeb 100644 --- a/Src/FwCoreDlgs/FwCoreDlgControls/StyleInfo.cs +++ b/Src/FwCoreDlgs/FwCoreDlgControls/StyleInfo.cs @@ -40,24 +40,6 @@ public class StyleInfo : BaseStyleInfo public StyleInfo(IStStyle style) : base(style) { - LoadDefaultFontFeatures(style); - } - - private void LoadDefaultFontFeatures(IStStyle style) - { - if (style == null || style.Rules == null) - return; - - for (int i = 0; i < style.Rules.StrPropCount; i++) - { - int tpt; - string value = style.Rules.GetStrProp(i, out tpt); - if (tpt == (int)FwTextPropType.ktptFontVariations) - { - m_defaultFontInfo.m_features.ExplicitValue = value; - return; - } - } } /// ------------------------------------------------------------------------------------ diff --git a/Src/xWorks/xWorksTests/CssGeneratorTests.cs b/Src/xWorks/xWorksTests/CssGeneratorTests.cs index 58870627bf..f660626a4c 100644 --- a/Src/xWorks/xWorksTests/CssGeneratorTests.cs +++ b/Src/xWorks/xWorksTests/CssGeneratorTests.cs @@ -2414,6 +2414,63 @@ public void GenerateCssForConfiguration_CustomPrintableAsciiFontFeatures_AreEsca Does.Contain("font-feature-settings:\"!abc\" 2,\"a\\\"b\\\\\" 1")); } + /// + /// LT-22351: liblcm's BaseStyleInfo.ProcessStyleRules previously dropped a style's + /// default ktptFontVariations (font features) when loading a style from its persisted + /// Rules. FieldWorks had a Styles-dialog-only adapter (StyleInfo.LoadDefaultFontFeatures) + /// that compensated for this, but the Dictionary Preview CSS generation path + /// (GenerateCssStyleFromLcmStyleSheet/AddFontInfoCss) reads plain BaseStyleInfo objects + /// straight out of LcmStyleSheet and had no such adapter, so default font features never + /// reached the preview CSS. This test builds the style the same way LcmStyleSheet does in + /// production - a real IStStyle with persisted Rules wrapped directly in a BaseStyleInfo, + /// not a TestStyle double with ExplicitValue set in memory - to prove the fix in liblcm + /// (sillsdev/liblcm#388) makes it all the way to the generated CSS. + /// + [Test] + public void GenerateCssForConfiguration_DefaultFontFeaturesFromPersistedStyleRules_ReachPreviewCss() + { + ConfiguredLcmGenerator.AssemblyFile = "xWorksTests"; + const string styleName = "DefaultFontFeaturesStyle"; + var styleFactory = Cache.ServiceLocator.GetInstance(); + var realStyle = styleFactory.Create(); + Cache.LanguageProject.StylesOC.Add(realStyle); + realStyle.Name = styleName; + realStyle.Context = ContextValues.Internal; + realStyle.Function = FunctionValues.Prose; + realStyle.Structure = StructureValues.Undefined; + realStyle.Type = StyleType.kstCharacter; + + var propsBldr = TsStringUtils.MakePropsBldr(); + propsBldr.SetStrPropValue((int)FwTextPropType.ktptFontVariations, "smcp=1"); + realStyle.Rules = propsBldr.GetTextProps(); + + // Build the style the way LcmStyleSheet.LoadStyles does in production: a BaseStyleInfo + // wrapping the real, persisted IStStyle -- no TestStyle, no in-memory ExplicitValue. + var persistedStyle = new BaseStyleInfo(realStyle); + SafelyAddStyleToSheetAndTable(styleName, persistedStyle); + try + { + var headwordNode = new ConfigurableDictionaryNode + { + FieldDescription = "SIL.FieldWorks.XWorks.TestRootClass", + Label = "Headword", + DictionaryNodeOptions = ConfiguredXHTMLGeneratorTests.GetWsOptionsForLanguages(new[] { "fr" }), + Style = styleName, + IsEnabled = true + }; + + var model = new DictionaryConfigurationModel { Parts = new List { headwordNode } }; + var cssResult = CssGenerator.GenerateCssFromConfiguration(model, m_propertyTable); + + Assert.That(cssResult, Does.Contain("font-feature-settings:\"smcp\" 1")); + } + finally + { + // The real IStStyle is undone after this test; do not leave a stale entry behind. + SafelyRemoveStyleFromSheetAndTable(styleName); + } + } + [Test] public void GenerateCssForConfiguration_ReversalSenseNumberWorks() { @@ -3150,6 +3207,69 @@ public void GenerateCssForConfiguration_WsSpanWithNormalStyle_UsesWritingSystemD Assert.That(cssResult, Contains.Substring("span[lang='" + vernWs.LanguageTag + "']{font-family:'Charis SIL',serif;font-feature-settings:\"ss11\" 1,\"ss12\" 1;")); } + /// + /// LT-22351: now that liblcm loads a style's default ktptFontVariations from its persisted + /// Rules (sillsdev/liblcm#388), a Normal style with its OWN font features must win over the + /// writing system's DefaultFontFeatures fallback in AddFontInfoCss. The sibling test above + /// covers only a Normal style WITHOUT its own features (fallback branch). + /// + [Test] + public void GenerateCssForConfiguration_NormalStyleOwnFontFeatures_BeatWritingSystemDefaultFontFeatures() + { + const string styleName = "Normal"; + var vernWs = Cache.ServiceLocator.WritingSystemManager.Get(Cache.DefaultVernWs); + vernWs.DefaultFont = new FontDefinition("Charis SIL") { Features = "ss11=1,ss12=1" }; + + // A real IStStyle whose persisted Rules carry the style's own default font features. + var realStyle = Cache.ServiceLocator.GetInstance().Create(); + Cache.LanguageProject.StylesOC.Add(realStyle); + realStyle.Name = styleName; + realStyle.Context = ContextValues.Internal; + realStyle.Function = FunctionValues.Prose; + realStyle.Structure = StructureValues.Undefined; + realStyle.Type = StyleType.kstParagraph; + var propsBldr = TsStringUtils.MakePropsBldr(); + propsBldr.SetStrPropValue((int)FwTextPropType.ktptFontVariations, "smcp=1"); + realStyle.Rules = propsBldr.GetTextProps(); + + SafelyAddStyleToSheetAndTable(styleName, new BaseStyleInfo(realStyle)); + try + { + var glossNode = new ConfigurableDictionaryNode + { + FieldDescription = "Gloss", + DictionaryNodeOptions = ConfiguredXHTMLGeneratorTests.GetWsOptionsForLanguages(new[] { vernWs.LanguageTag }) + }; + var testSensesNode = new ConfigurableDictionaryNode + { + FieldDescription = "Senses", + Children = new List { glossNode } + }; + var testEntryNode = new ConfigurableDictionaryNode + { + FieldDescription = "LexEntry", + Children = new List { testSensesNode } + }; + var model = new DictionaryConfigurationModel + { + Parts = new List { testEntryNode } + }; + PopulateFieldsForTesting(testEntryNode); + + var cssResult = Regex.Replace(CssGenerator.GenerateCssFromConfiguration(model, m_propertyTable), @"\t|\n|\r", ""); + + // The style's own features win; the font family still falls back to the WS default font. + Assert.That(cssResult, Contains.Substring("span[lang='" + vernWs.LanguageTag + "']{font-family:'Charis SIL',serif;font-feature-settings:\"smcp\" 1;")); + // The WS DefaultFontFeatures fallback must not leak through. + Assert.That(cssResult, Does.Not.Contain("ss11")); + } + finally + { + // The real IStStyle is undone after this test; do not leave a stale entry behind. + SafelyRemoveStyleFromSheetAndTable(styleName); + } + } + [Test] public void GenerateCssForConfiguration_NormalStyleForWsDoesNotOverrideNodeStyle() { @@ -4126,7 +4246,7 @@ private static TestStyle GenerateStyleFromFontInfo(LcmCache cache, string name, return new TestStyle(fontInfo, cache) { Name = name, IsParagraphStyle = isParagraphStyle }; } - private void SafelyAddStyleToSheetAndTable(string name, TestStyle style) + private void SafelyAddStyleToSheetAndTable(string name, BaseStyleInfo style) { if (m_styleSheet.Styles.Contains(name)) m_styleSheet.Styles.Remove(name); @@ -4136,6 +4256,20 @@ private void SafelyAddStyleToSheetAndTable(string name, TestStyle style) m_owningTable.Add(name, style); } + /// + /// Removes a style added by SafelyAddStyleToSheetAndTable. Tests that add a BaseStyleInfo + /// wrapping a real IStStyle MUST call this in a finally block: m_styleSheet is created once + /// per fixture, but the base class undoes all data changes after each test, which would + /// leave a stale entry whose RealStyle is invalid and crash later tests order-dependently. + /// + private void SafelyRemoveStyleFromSheetAndTable(string name) + { + if (m_styleSheet.Styles.Contains(name)) + m_styleSheet.Styles.Remove(name); + if (m_owningTable.ContainsKey(name)) + m_owningTable.Remove(name); + } + private void GenerateBulletStyle(string name) { var fontInfo = new FontInfo(); diff --git a/Src/xWorks/xWorksTests/LcmWordGeneratorTests.cs b/Src/xWorks/xWorksTests/LcmWordGeneratorTests.cs index b535cc954c..598be37f2c 100644 --- a/Src/xWorks/xWorksTests/LcmWordGeneratorTests.cs +++ b/Src/xWorks/xWorksTests/LcmWordGeneratorTests.cs @@ -328,6 +328,103 @@ public void GenerateCharacterStyleFromLcmStyleSheet_NormalStyle_UsesWritingSyste Is.EqualTo(new[] { W14.OnOffValues.True, W14.OnOffValues.True })); } + /// + /// LT-22351: default font features must survive the load from a style's persisted Rules + /// (liblcm BaseStyleInfo.ProcessStyleRules, sillsdev/liblcm#388) and reach the generated + /// Word styles. The sibling test above uses a TestStyle double whose ExplicitValue is set + /// in memory, which bypasses ProcessStyleRules; this one builds the style the way + /// LcmStyleSheet does in production - a real IStStyle wrapped in a BaseStyleInfo. + /// + [Test] + public void GenerateCharacterStyleFromLcmStyleSheet_DefaultFontFeaturesFromPersistedStyleRules_AddsWordTypographyProperties() + { + var styleName = "WordFeatureStylePersisted" + Guid.NewGuid().ToString("N"); + var realStyle = Cache.ServiceLocator.GetInstance().Create(); + Cache.LanguageProject.StylesOC.Add(realStyle); + realStyle.Name = styleName; + realStyle.Context = ContextValues.Internal; + realStyle.Function = FunctionValues.Prose; + realStyle.Structure = StructureValues.Undefined; + realStyle.Type = StyleType.kstCharacter; + var propsBldr = TsStringUtils.MakePropsBldr(); + propsBldr.SetStrPropValue((int)FwTextPropType.ktptFontVariations, "liga=0,lnum=1,pnum=1,calt=0,ss02=0,cv01=2"); + realStyle.Rules = propsBldr.GetTextProps(); + + var styles = FontHeightAdjuster.StyleSheetFromPropertyTable(m_propertyTable).Styles; + styles.Add(new BaseStyleInfo(realStyle)); + try + { + var style = WordStylesGenerator.GenerateCharacterStyleFromLcmStyleSheet(styleName, Cache.DefaultVernWs, + new ReadOnlyPropertyTable(m_propertyTable)); + + var runProps = style.GetFirstChild(); + AssertWordTypographyProperties(runProps, W14.LigaturesValues.None, W14.NumberFormValues.Lining, + W14.NumberSpacingValues.Proportional, false, 2U, false); + } + finally + { + // The real IStStyle is undone after this test; do not leave a stale entry behind. + styles.Remove(styleName); + } + } + + /// + /// LT-22351: a Normal style with its OWN default font features (loaded from its persisted + /// Rules, sillsdev/liblcm#388) must win over the writing system's DefaultFontFeatures + /// fallback in AddFontInfoWordStyles. The sibling test above covers only a Normal style + /// WITHOUT its own features (fallback branch). + /// + [Test] + public void GenerateCharacterStyleFromLcmStyleSheet_NormalStyleOwnFontFeatures_BeatWritingSystemDefaultFontFeatures() + { + var vernWs = Cache.ServiceLocator.WritingSystemManager.Get(Cache.DefaultVernWs); + vernWs.DefaultFont = new FontDefinition("Charis SIL") { Features = "ss11=1,ss12=1" }; + + var realStyle = Cache.ServiceLocator.GetInstance().Create(); + Cache.LanguageProject.StylesOC.Add(realStyle); + realStyle.Name = WordStylesGenerator.NormalParagraphStyleName; + realStyle.Context = ContextValues.Internal; + realStyle.Function = FunctionValues.Prose; + realStyle.Structure = StructureValues.Undefined; + realStyle.Type = StyleType.kstParagraph; + var propsBldr = TsStringUtils.MakePropsBldr(); + propsBldr.SetStrPropValue((int)FwTextPropType.ktptFontVariations, "ss02=1"); + realStyle.Rules = propsBldr.GetTextProps(); + + var styles = FontHeightAdjuster.StyleSheetFromPropertyTable(m_propertyTable).Styles; + if (styles.Contains(WordStylesGenerator.NormalParagraphStyleName)) + styles.Remove(WordStylesGenerator.NormalParagraphStyleName); + styles.Add(new BaseStyleInfo(realStyle)); + try + { + var style = WordStylesGenerator.GenerateCharacterStyleFromLcmStyleSheet( + WordStylesGenerator.NormalParagraphStyleName, + vernWs.Handle, + new ReadOnlyPropertyTable(m_propertyTable)); + + var runProps = style.GetFirstChild(); + Assert.That(runProps, Is.Not.Null); + + // The font family still falls back to the WS default font... + var runFonts = runProps.GetFirstChild(); + Assert.That(runFonts, Is.Not.Null); + Assert.That(runFonts.Ascii?.Value, Is.EqualTo("Charis SIL")); + + // ...but the style's own features win over the WS DefaultFontFeatures fallback. + var stylisticSets = runProps.GetFirstChild(); + Assert.That(stylisticSets, Is.Not.Null); + var styleSet = stylisticSets.Elements().Single(); + Assert.That(styleSet.Id?.Value, Is.EqualTo(2U)); + Assert.That(styleSet.Val?.Value, Is.EqualTo(W14.OnOffValues.True)); + } + finally + { + // The real IStStyle is undone after this test; restore the fixture's Normal double. + styles.Remove(WordStylesGenerator.NormalParagraphStyleName); + styles.Add(new BaseStyleInfo { Name = WordStylesGenerator.NormalParagraphStyleName, IsParagraphStyle = true }); + } + } + [Test] [Category("ManualDocx")] public void GenerateManualDocxArtifact_CharisBaseline_NoFontOptions() diff --git a/openspec/changes/add-opentype-font-features/design.md b/openspec/changes/add-opentype-font-features/design.md index 14936d0767..fdaef880fb 100644 --- a/openspec/changes/add-opentype-font-features/design.md +++ b/openspec/changes/add-opentype-font-features/design.md @@ -17,7 +17,7 @@ The longer product phases are: add OpenType features now, remove Graphite later - Keep persisted feature strings renderer-neutral and compatible with future Avalonia/HarfBuzz-style consumption. - Accept any syntactically valid OpenType tag and reject malformed tags safely with trace logging. - Add trace logging for discovery, validation, native shaping, and fallback decisions. -- Keep style/default font-feature loading on the existing inheritance path, with only the minimal compatibility adapter still required by the current build graph. +- Keep style/default font-feature loading on the existing inheritance path (the interim `StyleInfo` compatibility adapter was later removed under LT-22351; see Decision 11). - Fix truncation and malformed-input robustness gaps in legacy feature-string handling. - Add tests for UI control behavior and visual rendering differences caused by feature toggles. - Add test-only HarfBuzzSharp + SkiaSharp comparison tooling for future visual-fidelity confidence. @@ -119,11 +119,13 @@ The longer product phases are: add OpenType features now, remove Graphite later ### 11. Existing inheritance paths remain authoritative -**Decision:** `FontInfo.m_features`, `FwTextPropType.ktptFontVariations`, and style rule round-tripping remain the authoritative inheritance/data-flow path for default and explicit font features. `StyleInfo` retains a minimal compatibility adapter that reads default `ktptFontVariations` from `IStStyle.Rules` because focused validation showed that removing it loses persisted default font features in the current build graph. +**Decision:** `FontInfo.m_features`, `FwTextPropType.ktptFontVariations`, and style rule round-tripping remain the authoritative inheritance/data-flow path for default and explicit font features. `StyleInfo` originally retained a minimal compatibility adapter that read default `ktptFontVariations` from `IStStyle.Rules` because focused validation showed that removing it lost persisted default font features in the then-current build graph. -**Rationale:** The local LCM source contains `BaseStyleInfo.ProcessStyleRules` support for `ktptFontVariations`, but the active FieldWorks build/test path still requires the `StyleInfo` adapter to reload persisted defaults. The adapter is therefore a compatibility boundary, not a second policy path. +**Rationale:** At the time of this decision, the active FieldWorks build/test path still required the `StyleInfo` adapter to reload persisted defaults even though the local LCM source contained `BaseStyleInfo.ProcessStyleRules` support for `ktptFontVariations`. The adapter was a compatibility boundary, not a second policy path. -**Alternatives considered:** Remove the `StyleInfo` adapter immediately. Rejected for this change because `SaveToDB_DefaultFontFeatures_RoundTripsThroughRules` failed after removal. Broader LCM dependency alignment can retire the adapter later with the same round-trip tests as the gate. +**Alternatives considered:** Remove the `StyleInfo` adapter immediately. Rejected for this change because `SaveToDB_DefaultFontFeatures_RoundTripsThroughRules` failed after removal. Broader LCM dependency alignment could retire the adapter later with the same round-trip tests as the gate. + +**Status update (LT-22351):** The gating condition has been satisfied. liblcm's `BaseStyleInfo.ProcessStyleRules` now loads default `ktptFontVariations` (sillsdev/liblcm#388), `SaveToDB_DefaultFontFeatures_RoundTripsThroughRules` passes through the authoritative `BaseStyleInfo` path, and the `StyleInfo.LoadDefaultFontFeatures` adapter was removed under LT-22351. ### 12. Overlong and malformed feature strings fail safe diff --git a/openspec/changes/add-opentype-font-features/proposal.md b/openspec/changes/add-opentype-font-features/proposal.md index 73eb77ec12..bf087d0ff7 100644 --- a/openspec/changes/add-opentype-font-features/proposal.md +++ b/openspec/changes/add-opentype-font-features/proposal.md @@ -16,7 +16,7 @@ The phase also needs explicit OpenType-first behavior for dual-technology fonts, - Add trace logging for malformed feature strings, malformed tags, filtered feature discovery, provider selection, native shaping failures, and fallback decisions. - Accept any syntactically valid OpenType tag name, including custom/private tags; reject malformed tags safely and log them instead of narrowing accepted tags to a registry allowlist. - Fix legacy truncation logic so overlong feature strings without comma boundaries fail safe. -- Keep the existing style/font-feature inheritance path authoritative and retain only the minimal `StyleInfo` compatibility adapter needed for the current build graph to reload default `ktptFontVariations` from style rules. +- Keep the existing style/font-feature inheritance path authoritative. (A minimal `StyleInfo` compatibility adapter originally reloaded default `ktptFontVariations` from style rules; it was removed under LT-22351 once liblcm's `BaseStyleInfo.ProcessStyleRules` gained the `ktptFontVariations` case — sillsdev/liblcm#388.) - Add UI/component tests for font-feature controls and high-level visual rendering tests proving feature settings change output. - Add robustness tests for malformed input, feature filtering, OpenType-preferred behavior, truncation safety, fallback behavior, CSS/DOCX export safety, and inheritance-path round-tripping. - Add a test-only HarfBuzzSharp + SkiaSharp comparison path for shaping/rendering confidence toward future Avalonia migration; this path is not a production renderer in Phase 1. diff --git a/openspec/changes/add-opentype-font-features/research.md b/openspec/changes/add-opentype-font-features/research.md index 7198d123c1..38ffef838d 100644 --- a/openspec/changes/add-opentype-font-features/research.md +++ b/openspec/changes/add-opentype-font-features/research.md @@ -265,9 +265,12 @@ Implementation follow-up: - Removing the `StyleInfo` loader was attempted during implementation and failed `SaveToDB_DefaultFontFeatures_RoundTripsThroughRules`, while restoring it made - the style/font-tab slice pass. The loader therefore remains as a minimal - compatibility adapter until the active LCM dependency path consumes - `ktptFontVariations` defaults through `BaseStyleInfo.ProcessStyleRules`. + the style/font-tab slice pass. The loader remained as a minimal compatibility + adapter until the active LCM dependency path consumed `ktptFontVariations` + defaults through `BaseStyleInfo.ProcessStyleRules`. That happened under + LT-22351: liblcm gained the `ktptFontVariations` case in `ProcessStyleRules` + (sillsdev/liblcm#388), the round-trip test passed through the authoritative + path, and the `StyleInfo.LoadDefaultFontFeatures` adapter was removed. ### Recommended Additional Tests Beyond The Original Plan