Fix LT-22351: OpenType/Graphite font features from style defaults now reach Dictionary Preview

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;
-				}
-			}
 		}
 
 		/// ------------------------------------------------------------------------------------

Src/xWorks/xWorksTests/CssGeneratorTests.cs

@@ -2414,6 +2414,63 @@ public void GenerateCssForConfiguration_CustomPrintableAsciiFontFeatures_AreEsca
 				Does.Contain("font-feature-settings:\"!abc\" 2,\"a\\\"b\\\\\" 1"));
 		}
 
+		/// <summary>
+		/// 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.
+		/// </summary>
+		[Test]
+		public void GenerateCssForConfiguration_DefaultFontFeaturesFromPersistedStyleRules_ReachPreviewCss()
+		{
+			ConfiguredLcmGenerator.AssemblyFile = "xWorksTests";
+			const string styleName = "DefaultFontFeaturesStyle";
+			var styleFactory = Cache.ServiceLocator.GetInstance<IStStyleFactory>();
+			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<ConfigurableDictionaryNode> { 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;"));
 		}
 
+		/// <summary>
+		/// 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).
+		/// </summary>
+		[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<IStStyleFactory>().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<ConfigurableDictionaryNode> { glossNode }
+				};
+				var testEntryNode = new ConfigurableDictionaryNode
+				{
+					FieldDescription = "LexEntry",
+					Children = new List<ConfigurableDictionaryNode> { testSensesNode }
+				};
+				var model = new DictionaryConfigurationModel
+				{
+					Parts = new List<ConfigurableDictionaryNode> { 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);
 		}
 
+		/// <summary>
+		/// 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.
+		/// </summary>
+		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();

Src/xWorks/xWorksTests/LcmWordGeneratorTests.cs

@@ -328,6 +328,103 @@ public void GenerateCharacterStyleFromLcmStyleSheet_NormalStyle_UsesWritingSyste
 				Is.EqualTo(new[] { W14.OnOffValues.True, W14.OnOffValues.True }));
 		}
 
+		/// <summary>
+		/// 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.
+		/// </summary>
+		[Test]
+		public void GenerateCharacterStyleFromLcmStyleSheet_DefaultFontFeaturesFromPersistedStyleRules_AddsWordTypographyProperties()
+		{
+			var styleName = "WordFeatureStylePersisted" + Guid.NewGuid().ToString("N");
+			var realStyle = Cache.ServiceLocator.GetInstance<IStStyleFactory>().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<StyleRunProperties>();
+				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);
+			}
+		}
+
+		/// <summary>
+		/// 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).
+		/// </summary>
+		[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<IStStyleFactory>().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<StyleRunProperties>();
+				Assert.That(runProps, Is.Not.Null);
+
+				// The font family still falls back to the WS default font...
+				var runFonts = runProps.GetFirstChild<RunFonts>();
+				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<W14.StylisticSets>();
+				Assert.That(stylisticSets, Is.Not.Null);
+				var styleSet = stylisticSets.Elements<W14.StyleSet>().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()

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
 

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.

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