diff --git a/FormCraft.ForFluentUI.UnitTests/Fields/FieldConfigurationRefreshTests.cs b/FormCraft.ForFluentUI.UnitTests/Fields/FieldConfigurationRefreshTests.cs new file mode 100644 index 00000000..d7c49c35 --- /dev/null +++ b/FormCraft.ForFluentUI.UnitTests/Fields/FieldConfigurationRefreshTests.cs @@ -0,0 +1,259 @@ +using FormCraft.ForFluentUI.Extensions; +using FormCraft.ForFluentUI.UnitTests.Components; +using Microsoft.FluentUI.AspNetCore.Components; + +namespace FormCraft.ForFluentUI.UnitTests.Fields; + +/// +/// A Fluent field component must render the configuration of the field it is currently showing +/// (#335). +/// +/// +/// +/// The same defect #298 fixed for MudBlazor, unfixed in this adapter: components read their +/// configuration once in OnInitialized and never look again, so an instance re-parameterised +/// with a different Context keeps rendering the previous field's settings. Blazor reuses a +/// component instance whenever the render-tree shape matches, which a swapped +/// FormCraftComponent.Configuration — a wizard step, a mode toggle — does routinely. +/// +/// +/// Mirrors FormCraft.ForMudBlazor.UnitTests.Fields.FieldConfigurationRefreshTests deliberately. +/// One behaviour implemented twice and drifting is this library's recurring defect (#146, #177, #184, +/// #190, #203, #279), and the fix for it — the hook on FieldComponentBase — is now shared, so +/// the coverage should be recognisably the same on both sides. +/// +/// +public class FieldConfigurationRefreshTests : FluentUITestBase +{ + /// + /// The assumption the refresh rests on: Context.Field is the same object across renders. + /// + /// + /// Re-pinned here rather than assumed from the MudBlazor side. Both adapters go through + /// FieldRendererService.RenderField, which allocates a fresh FieldRenderContext per + /// render — so the context is not stable — but fills its Field from the built + /// configuration, which FormBuilder.Build() makes immutable and hands out by reference. + /// The guard compares that reference, so it has to hold or the refresh either never fires or + /// fires on every keystroke. + /// + [Fact] + public void Context_Field_Should_Be_The_Same_Instance_Across_Renders() + { + // Arrange + var component = Render>(parameters => parameters + .Add(p => p.Model, new TestModel()) + .Add(p => p.Configuration, TextConfiguration("text"))); + + var first = component.FindComponent>().Instance.Context.Field; + + // Act + component.Render(); + component.Render(); + + // Assert + var second = component.FindComponent>().Instance.Context.Field; + ReferenceEquals(first, second).ShouldBeTrue(); + } + + /// + /// A different field arriving on the same instance re-reads that field's configuration. + /// + /// + /// Both configurations declare a field called Name at the same position, so Blazor reuses + /// the component — and the input type stayed on whatever the first configuration declared. + /// + [Fact] + public void TextField_Should_Rebind_Its_InputType_When_The_Configuration_Is_Swapped() + { + // Arrange + var component = Render>(parameters => parameters + .Add(p => p.Model, new TestModel()) + .Add(p => p.Configuration, TextConfiguration("text"))); + + component.FindComponent().Instance.TextInputType + .ShouldBe(TextInputType.Text); + + // Act + component.Render(parameters => parameters + .Add(p => p.Configuration, TextConfiguration("password"))); + + // Assert + component.FindComponent().Instance.TextInputType + .ShouldBe(TextInputType.Password); + } + + /// + /// An attribute the new field does not declare reverts to its default (#335). + /// + /// + /// The complement, and the one that catches a fix that only ever overwrites. A reload + /// assigning each attribute it finds leaves the previous field's value in place for every + /// attribute the new field omits — so a field that dropped Lines would keep rendering a + /// text area. + /// + /// Asserted on which component renders, not on a property: the razor picks + /// FluentTextArea over FluentTextInput on Lines > 1, so the rendered shape + /// is the honest question. + /// + /// + [Fact] + public void TextField_Should_Revert_To_A_Single_Line_When_The_New_Configuration_Drops_Lines() + { + // Arrange + var multiLine = FormBuilder + .Create() + .AddField(x => x.Name, field => field + .WithLabel("Name") + .WithAttribute("Lines", 4)) + .Build(); + + var component = Render>(parameters => parameters + .Add(p => p.Model, new TestModel()) + .Add(p => p.Configuration, multiLine)); + + component.FindComponents().Count.ShouldBe(1); + + // Act - the replacement field declares no Lines at all. + component.Render(parameters => parameters + .Add(p => p.Configuration, TextConfiguration("text"))); + + // Assert + component.FindComponents().ShouldBeEmpty(); + component.FindComponents().Count.ShouldBe(1); + } + + /// + /// A numeric field rebinds its Min when a different field declares another one (#335). + /// + /// + /// + /// The numeric component collects Min/Max/Step into a + /// Dictionary<string, object> through a helper that only ever adds when the + /// attribute is configured, then splats it with @attributes. Nothing removed a key, so + /// before this fix the dictionary accumulated across fields; it is now cleared on every reload. + /// + /// + /// ⚠️ Scope of this test. It swaps one bound for another rather than dropping it, because + /// omission cannot be expressed through a splat: Blazor retains a component parameter that + /// a later render stops supplying, so a field that declares no Min leaves + /// FluentNumberInput.Min holding the previous field's value even though FormCraft's + /// dictionary is correct. Expressing "unset" would mean FormCraft supplying Fluent's own defaults + /// (int.MinValue) explicitly, i.e. binding the bounds as real parameters instead of + /// splatting a dictionary. That is a change to how the Fluent numeric components are written and + /// is recorded as a follow-up rather than smuggled in here. + /// + /// + [Fact] + public void NumericField_Should_Rebind_Its_Min_When_The_Configuration_Is_Swapped() + { + // Arrange + // Typed as int? deliberately: AddIfConfigured reads it back with GetAttribute, so a + // plainly-boxed int would not match and the bound would never be configured at all. The + // existing numeric suite spells its Min/Max/Step the same way. + var component = Render>(parameters => parameters + .Add(p => p.Model, new NumericModel()) + .Add(p => p.Configuration, BoundedConfiguration(5))); + + // Asserted on what the Fluent input was actually bound, the way the existing numeric suite + // does: ExtraAttributes is splatted onto the component, so the dictionary's contents become + // its parameters. + component.FindComponent>().Instance.Min.ShouldBe(5); + + // Act + component.Render(parameters => parameters + .Add(p => p.Configuration, BoundedConfiguration(9))); + + // Assert + component.FindComponent>().Instance.Min.ShouldBe(9); + } + + /// + /// A lookup keeps showing its stored value after a configuration swap (#335). + /// + /// + /// + /// The regression test for a fix that was almost worse than the bug. The first attempt reset + /// _displayText to empty in the hook — correct for staleness, and catastrophic on its own, + /// because nothing else in this component repopulates it from the model. The MudBlazor lookup + /// gets away with clearing because its OnParametersSet calls UpdateDisplayText() on + /// every render and repairs the blank on the same pass; the Fluent one has no such call, so a + /// field with a perfectly good stored value rendered empty for ever. + /// + /// + /// The hook now re-derives the text rather than clearing it, which is what "reload, not patch" + /// means when the property is derived rather than read. + /// + /// + [Fact] + public void LookupField_Should_Keep_Displaying_Its_Value_After_A_Configuration_Swap() + { + // Arrange + var model = new TripModel { CityId = 7 }; + + var component = Render>(parameters => parameters + .Add(p => p.Model, model) + .Add(p => p.Configuration, LookupConfiguration())); + + component.FindComponent().Instance.Value.ShouldBe("7"); + + // Act - a different configuration object declaring the same lookup field. + component.Render(parameters => parameters + .Add(p => p.Configuration, LookupConfiguration())); + + // Assert - the display still reflects the model, rather than having been blanked. + component.FindComponent().Instance.Value.ShouldBe("7"); + } + + private static IFormConfiguration LookupConfiguration() => + FormBuilder + .Create() + .AddField(x => x.CityId, field => + { + field.WithLabel("City"); + + // Called as a static method rather than as an extension on purpose: this project + // references BOTH adapters, and the MudBlazor package publishes an .AsLookup(...) of + // the same name into namespace FormCraft, so the extension form would be + // CS0121-ambiguous here. Same reasoning as FluentUILookupFieldComponentTests. + FluentUIFieldBuilderExtensions.AsLookup( + field, + dataProvider: _ => Task.FromResult(new LookupResult + { + Items = [new City(7, "Lisbon")], + TotalCount = 1, + }), + valueSelector: c => c.Id, + displaySelector: c => c.Name, + configureColumns: cols => + cols.Add(new LookupColumn { Title = "Name", ValueSelector = c => c.Name })); + }) + .Build(); + + private class TripModel + { + public int CityId { get; set; } + } + + private record City(int Id, string Name); + + private static IFormConfiguration BoundedConfiguration(int min) => + FormBuilder + .Create() + .AddField(x => x.Amount, field => field + .WithLabel("Amount") + .WithAttribute("Min", (int?)min)) + .Build(); + + private static IFormConfiguration TextConfiguration(string inputType) => + FormBuilder + .Create() + .AddField(x => x.Name, field => field + .WithLabel("Name") + .WithAttribute("InputType", inputType)) + .Build(); + + private class NumericModel + { + public int Amount { get; set; } + } +} diff --git a/FormCraft.ForFluentUI/Fields/AutocompleteField/FluentUIAutocompleteFieldComponent.razor.cs b/FormCraft.ForFluentUI/Fields/AutocompleteField/FluentUIAutocompleteFieldComponent.razor.cs index 34d98d3b..d750e4de 100644 --- a/FormCraft.ForFluentUI/Fields/AutocompleteField/FluentUIAutocompleteFieldComponent.razor.cs +++ b/FormCraft.ForFluentUI/Fields/AutocompleteField/FluentUIAutocompleteFieldComponent.razor.cs @@ -30,10 +30,31 @@ protected override void OnInitialized() { base.OnInitialized(); + SyncSelectedOption(); + } + + /// + /// + /// Moved off OnInitialized so a component instance handed a different field re-reads it + /// rather than rendering the previous field's settings (#335). + /// + protected override void OnFieldConfigurationChanged() + { + base.OnFieldConfigurationChanged(); + _searchFunc = GetAttribute>>>>( "AutocompleteSearchFunc"); _optionProvider = GetAttribute("AutocompleteOptionProvider"); + // Both are results of the configuration above rather than of the value, so they belong to the + // field that produced them. _options is the previous field's last result set, which the + // dropdown would keep offering until a fresh search replaced it; _selectedOption is what the + // box displays, and SyncSelectedOption only rebuilds it when the VALUE differs — so two + // fields whose values compare equal but whose labels differ would leave the previous field's + // label on screen. + _options = []; + _selectedOption = null; + SyncSelectedOption(); } diff --git a/FormCraft.ForFluentUI/Fields/BooleanField/FluentUIBooleanFieldComponent.razor.cs b/FormCraft.ForFluentUI/Fields/BooleanField/FluentUIBooleanFieldComponent.razor.cs index 46ebd890..3bc8a960 100644 --- a/FormCraft.ForFluentUI/Fields/BooleanField/FluentUIBooleanFieldComponent.razor.cs +++ b/FormCraft.ForFluentUI/Fields/BooleanField/FluentUIBooleanFieldComponent.razor.cs @@ -20,6 +20,16 @@ protected override void OnInitialized() base.OnInitialized(); _localValue = CurrentValue; + } + + /// + /// + /// Moved off OnInitialized so a component instance handed a different field re-reads it + /// rather than rendering the previous field's settings (#335). + /// + protected override void OnFieldConfigurationChanged() + { + base.OnFieldConfigurationChanged(); // Checkbox is the default; a switch is opt-in, matching the MudBlazor adapter. DisplayStyle = GetAttribute("DisplayStyle", BooleanDisplayStyle.Checkbox); diff --git a/FormCraft.ForFluentUI/Fields/LookupField/FluentUILookupFieldComponent.razor.cs b/FormCraft.ForFluentUI/Fields/LookupField/FluentUILookupFieldComponent.razor.cs index 584e611c..1ee48228 100644 --- a/FormCraft.ForFluentUI/Fields/LookupField/FluentUILookupFieldComponent.razor.cs +++ b/FormCraft.ForFluentUI/Fields/LookupField/FluentUILookupFieldComponent.razor.cs @@ -45,16 +45,33 @@ public partial class FluentUILookupFieldComponent private IReadOnlyList Columns => _columns; /// - protected override void OnInitialized() + /// + /// Moved off OnInitialized so a component instance handed a different field re-reads it + /// rather than rendering the previous field's settings (#335). The hook runs on first render too, + /// so this component needs no OnInitialized of its own. + /// + protected override void OnFieldConfigurationChanged() { - base.OnInitialized(); + base.OnFieldConfigurationChanged(); + // Columns are read from the field's attributes, so they belong to the field rather than to + // this instance — a different field gets its own. _columns = BuildColumns(); - if (CurrentValue is not null) - { - DisplayText = CurrentValue.ToString() ?? string.Empty; - } + // ⛔ Re-derived, not merely cleared. Nothing else in this component repopulates the display + // from the model: unlike the MudBlazor lookup, which calls UpdateDisplayText() from + // OnParametersSet on every render and would repair a blank on the same pass, here the only + // other writer is a row selection. Clearing alone therefore left a field with a perfectly + // good stored value rendering empty for ever — a worse bug than the staleness it replaced. + DisplayText = CurrentValue?.ToString() ?? string.Empty; + + // The picker belongs to the field that opened it. Its rows came from the PREVIOUS field's + // LookupDataProvider, and _rows is a List so nothing type-guards it: clicking one + // after a swap DynamicInvokes the new field's value/display selectors against the old + // field's row object, which throws out of a click handler when the item types differ. + _isOpen = false; + _rows.Clear(); + _searchText = string.Empty; } private async Task TogglePickerAsync() diff --git a/FormCraft.ForFluentUI/Fields/LovField/FluentUILovFieldComponent.razor.cs b/FormCraft.ForFluentUI/Fields/LovField/FluentUILovFieldComponent.razor.cs index 78449fd9..b6914c0b 100644 --- a/FormCraft.ForFluentUI/Fields/LovField/FluentUILovFieldComponent.razor.cs +++ b/FormCraft.ForFluentUI/Fields/LovField/FluentUILovFieldComponent.razor.cs @@ -48,9 +48,27 @@ public partial class FluentUILovFieldComponent private IReadOnlyList> Columns => LovConfig?.Columns ?? []; /// - protected override void OnInitialized() + /// + /// Moved off OnInitialized so a component instance handed a different field re-reads it + /// rather than rendering the previous field's settings (#335). The hook runs on first render too, + /// so this component needs no OnInitialized of its own. + /// + protected override void OnFieldConfigurationChanged() { - base.OnInitialized(); + base.OnFieldConfigurationChanged(); + + // ⛔ Cleared before anything is rebuilt, and the SELECTION is the part that matters. It holds + // rows drawn from the previous field's data source, and a subsequent pick appends to it — so + // the display would read "old, old, new" and, worse, PublishSelectionAsync would write the + // previous field's values into the NEW field's model property. The MudBlazor LOV clears the + // same list for the same reason (#298); the two adapters drifting on this is exactly what + // moving the hook into core is meant to stop. + _selectedItems.Clear(); + _rows.Clear(); + DisplayText = string.Empty; + _searchText = string.Empty; + _isOpen = false; + _isLoading = false; LovConfig = GetAttribute>("LovConfiguration") ?? throw new InvalidOperationException( diff --git a/FormCraft.ForFluentUI/Fields/NumericField/FluentUINullableNumericFieldComponent.razor.cs b/FormCraft.ForFluentUI/Fields/NumericField/FluentUINullableNumericFieldComponent.razor.cs index de7601c7..cd503c62 100644 --- a/FormCraft.ForFluentUI/Fields/NumericField/FluentUINullableNumericFieldComponent.razor.cs +++ b/FormCraft.ForFluentUI/Fields/NumericField/FluentUINullableNumericFieldComponent.razor.cs @@ -24,6 +24,21 @@ protected override void OnInitialized() base.OnInitialized(); _localValue = CurrentValue; + } + + /// + /// + /// Moved off OnInitialized so a component instance handed a different field re-reads it + /// rather than rendering the previous field's settings (#335). + /// + protected override void OnFieldConfigurationChanged() + { + base.OnFieldConfigurationChanged(); + + // CLEARED first. ExtraAttributes is a dictionary and AddIfConfigured only ever adds, so + // without this the new field inherits every bound the previous one declared — the + // patch-not-reload trap in its purest form (#335). + ExtraAttributes.Clear(); AddIfConfigured("Min"); AddIfConfigured("Max"); diff --git a/FormCraft.ForFluentUI/Fields/NumericField/FluentUINumericFieldComponent.razor.cs b/FormCraft.ForFluentUI/Fields/NumericField/FluentUINumericFieldComponent.razor.cs index 1122cb04..c8ace156 100644 --- a/FormCraft.ForFluentUI/Fields/NumericField/FluentUINumericFieldComponent.razor.cs +++ b/FormCraft.ForFluentUI/Fields/NumericField/FluentUINumericFieldComponent.razor.cs @@ -32,6 +32,21 @@ protected override void OnInitialized() base.OnInitialized(); _localValue = CurrentValue; + } + + /// + /// + /// Moved off OnInitialized so a component instance handed a different field re-reads it + /// rather than rendering the previous field's settings (#335). + /// + protected override void OnFieldConfigurationChanged() + { + base.OnFieldConfigurationChanged(); + + // CLEARED first. ExtraAttributes is a dictionary and AddIfConfigured only ever adds, so + // without this the new field inherits every bound the previous one declared — the + // patch-not-reload trap in its purest form (#335). + ExtraAttributes.Clear(); AddIfConfigured("Min"); AddIfConfigured("Max"); diff --git a/FormCraft.ForFluentUI/Fields/SelectField/FluentUIMultiSelectFieldComponent.razor.cs b/FormCraft.ForFluentUI/Fields/SelectField/FluentUIMultiSelectFieldComponent.razor.cs index 0fef4ab2..65134056 100644 --- a/FormCraft.ForFluentUI/Fields/SelectField/FluentUIMultiSelectFieldComponent.razor.cs +++ b/FormCraft.ForFluentUI/Fields/SelectField/FluentUIMultiSelectFieldComponent.razor.cs @@ -13,11 +13,23 @@ public partial class FluentUIMultiSelectFieldComponent private IEnumerable> Options { get; set; } = []; /// - protected override void OnInitialized() + /// + /// Moved off OnInitialized so a component instance handed a different field re-reads it + /// rather than rendering the previous field's settings (#335). + /// + protected override void OnFieldConfigurationChanged() { - base.OnInitialized(); + base.OnFieldConfigurationChanged(); + + // Falls back to an EMPTY list rather than to the current value of Options. `?? Options` reads + // as "keep the default" and is that on first load, but on a reload it means "keep the previous + // field's options" — offering the user choices from a field no longer on screen (#335). + Options = GetAttribute>>("MultiSelectOptions")?.ToList() + ?? []; - Options = GetAttribute>>("MultiSelectOptions")?.ToList() ?? Options; + // Recomputed whenever Options are: the selection is projected THROUGH them, so it is stale the + // moment they change. This component has no OnInitialized of its own — the hook runs on first + // render too, so this is the only place that needs to do it. _selectedOptions = OptionsFor(CurrentValue); } diff --git a/FormCraft.ForFluentUI/Fields/SelectField/FluentUISelectFieldComponent.razor.cs b/FormCraft.ForFluentUI/Fields/SelectField/FluentUISelectFieldComponent.razor.cs index 0dab82a7..2c2202bc 100644 --- a/FormCraft.ForFluentUI/Fields/SelectField/FluentUISelectFieldComponent.razor.cs +++ b/FormCraft.ForFluentUI/Fields/SelectField/FluentUISelectFieldComponent.razor.cs @@ -16,6 +16,21 @@ protected override void OnInitialized() base.OnInitialized(); _localValue = CurrentValue; + } + + /// + /// + /// Moved off OnInitialized so a component instance handed a different field re-reads it + /// rather than rendering the previous field's settings (#335). + /// + protected override void OnFieldConfigurationChanged() + { + base.OnFieldConfigurationChanged(); + + // Cleared BEFORE resolving, because ResolveOptions returns the current Options for a field + // that configures none. That reads as "keep the default" and is exactly that on first load — + // but on a reload it means "keep the previous field's options" (#335). + Options = []; Options = ResolveOptions(); } diff --git a/FormCraft.ForFluentUI/Fields/TextField/FluentUITextFieldComponent.razor.cs b/FormCraft.ForFluentUI/Fields/TextField/FluentUITextFieldComponent.razor.cs index 2e1eb2ba..6fcb9e66 100644 --- a/FormCraft.ForFluentUI/Fields/TextField/FluentUITextFieldComponent.razor.cs +++ b/FormCraft.ForFluentUI/Fields/TextField/FluentUITextFieldComponent.razor.cs @@ -22,6 +22,16 @@ protected override void OnInitialized() base.OnInitialized(); _localValue = CurrentValue; + } + + /// + /// + /// Moved off OnInitialized so a component instance handed a different field re-reads it + /// rather than rendering the previous field's settings (#335). + /// + protected override void OnFieldConfigurationChanged() + { + base.OnFieldConfigurationChanged(); var configuredInputType = Context.Field.InputType ?? GetAttribute("InputType", "text") ?? "text"; InputType = FluentTextInputTypeMap.Resolve(configuredInputType); diff --git a/FormCraft.ForMudBlazor/Fields/BooleanField/MudBlazorBooleanFieldComponent.razor.cs b/FormCraft.ForMudBlazor/Fields/BooleanField/MudBlazorBooleanFieldComponent.razor.cs index 5c2c2767..6f4211a1 100644 --- a/FormCraft.ForMudBlazor/Fields/BooleanField/MudBlazorBooleanFieldComponent.razor.cs +++ b/FormCraft.ForMudBlazor/Fields/BooleanField/MudBlazorBooleanFieldComponent.razor.cs @@ -43,27 +43,17 @@ protected override void OnInitialized() _localValue = CurrentValue is bool val ? val : false; } - /// - /// Tracks which field this instance's cached properties were loaded from (#298). - /// + /// /// - /// Wired locally, for the same reason NativeRequiredValue above is: this component derives - /// from FieldComponentBase directly, so it inherits neither the hook on - /// MudBlazorFieldComponentBase nor the one on MudBlazorFileUploadComponentBase. Only - /// the wiring repeats — holds the rule and its reasoning. + /// Moved off OnInitialized so an instance handed a different field re-reads it rather than + /// rendering the previous field's settings (#298). Inherited from + /// FieldComponentBase since #335 — this component derives from it directly, so before the + /// hook moved into core it had to wire its own. /// - private readonly FieldConfigurationTracker _fieldTracker = new(); - - private void RefreshFieldConfigurationIfChanged() + protected override void OnFieldConfigurationChanged() { - if (!_fieldTracker.HasChanged(Context?.Field)) - { - return; - } + base.OnFieldConfigurationChanged(); - // Moved off OnInitialized so an instance handed a different field re-reads it rather than - // rendering the previous field's settings (#298). - // // Checkbox is the default (parity with the legacy render path); a // switch can be requested explicitly via the DisplayStyle attribute. DisplayStyle = GetAttribute("DisplayStyle", BooleanDisplayStyle.Checkbox); @@ -75,8 +65,6 @@ protected override void OnParametersSet() { base.OnParametersSet(); - RefreshFieldConfigurationIfChanged(); - // Sync local value when model changes externally var currentVal = CurrentValue is bool val ? val : false; if (currentVal != _localValue) diff --git a/FormCraft.ForMudBlazor/Fields/FileUploadField/MudBlazorFileUploadComponentBase.cs b/FormCraft.ForMudBlazor/Fields/FileUploadField/MudBlazorFileUploadComponentBase.cs index 002e3ac6..e7202618 100644 --- a/FormCraft.ForMudBlazor/Fields/FileUploadField/MudBlazorFileUploadComponentBase.cs +++ b/FormCraft.ForMudBlazor/Fields/FileUploadField/MudBlazorFileUploadComponentBase.cs @@ -100,53 +100,6 @@ protected string RequiredDescriptionId protected string RequiredDescription => HasLabel ? $"{Label} is required." : "This file upload is required."; - /// - /// Tracks which field this instance's cached properties were loaded from (#298). - /// - /// - /// The upload components sit on their own base rather than - /// , but they cache configuration in - /// exactly the same way — Accept, MaxFileSize, UploadMode and the rest, read - /// once in OnInitialized — so they have the same staleness bug and need the same hook. The - /// shared piece is the tracker; only this wiring is repeated. - /// - private readonly FieldConfigurationTracker _fieldTracker = new(); - - /// - protected override void OnInitialized() - { - base.OnInitialized(); - RefreshFieldConfigurationIfChanged(); - } - - /// - protected override void OnParametersSet() - { - base.OnParametersSet(); - RefreshFieldConfigurationIfChanged(); - } - - private void RefreshFieldConfigurationIfChanged() - { - if (_fieldTracker.HasChanged(Context?.Field)) - { - OnFieldConfigurationChanged(); - } - } - - /// - /// Reads everything this component caches from Context.Field. Called once per field (#298). - /// - /// - /// Override this instead of loading configuration in OnInitialized, and assign every cached - /// property on every call — including back to its default. The override is a reload, not a patch: - /// a property left untouched because the new field does not declare that attribute keeps the - /// previous field's value, which is the same bug in a smaller box. - /// - protected virtual void OnFieldConfigurationChanged() - { - } - /// /// The field's Browse button, captured by @ref in both upload components. /// diff --git a/FormCraft.ForMudBlazor/Fields/MudBlazorFieldComponentBase.cs b/FormCraft.ForMudBlazor/Fields/MudBlazorFieldComponentBase.cs index c54816e0..2f848fb0 100644 --- a/FormCraft.ForMudBlazor/Fields/MudBlazorFieldComponentBase.cs +++ b/FormCraft.ForMudBlazor/Fields/MudBlazorFieldComponentBase.cs @@ -482,82 +482,25 @@ protected bool ShouldReport(string category) /// protected virtual bool SuppressShrinkLabelDiagnostic => false; - /// - /// Tracks which field this instance's cached properties were loaded from (#298). - /// - private readonly FieldConfigurationTracker _fieldTracker = new(); - - /// - protected override void OnInitialized() - { - base.OnInitialized(); - - // Before the derived component's own OnInitialized body runs — it calls base.OnInitialized() - // first, so its configuration is loaded by the time its diagnostics look at it. - RefreshFieldConfigurationIfChanged(); - } - /// protected override void OnParametersSet() { base.OnParametersSet(); - - // #298. Blazor reuses a component instance whenever the render-tree shape matches, so this is - // the only place a component learns it has been handed a different field. Without it the - // instance renders the previous field's mask, adornment and input type indefinitely — silently, - // and with output that looks entirely plausible. - RefreshFieldConfigurationIfChanged(); - EmitShrinkLabelDiagnosticIfNeeded(); } - /// - /// Calls when, and only when, the field changed. - /// + /// /// - /// The guard — see — is what makes this affordable: - /// runs on every keystroke, so the alternative is re-reading every - /// attribute per character typed. + /// Resets this base's own latch on the same terms the hook's docs ask derived components to reset + /// theirs. A ShrinkLabel conflict is a fact about a FIELD, so an instance that reported one for + /// its first field must still be able to report one for the next — otherwise a reused component + /// goes permanently silent about every field after the first (#298). /// - private void RefreshFieldConfigurationIfChanged() + protected override void OnFieldConfigurationChanged() { - if (!_fieldTracker.HasChanged(Context?.Field)) - { - return; - } + base.OnFieldConfigurationChanged(); - // The base's own latch, reset on the same terms the hook's docs ask derived components to - // reset theirs. A ShrinkLabel conflict is a fact about a FIELD, so an instance that reported - // one for its first field must still be able to report one for the next — otherwise a reused - // component goes permanently silent about every field after the first (#298). _shrinkLabelDiagnosticEmitted = false; - - OnFieldConfigurationChanged(); - } - - /// - /// Reads everything this component caches from Context.Field. Called once per field (#298). - /// - /// - /// - /// Override this instead of loading configuration in OnInitialized. It runs on first render - /// and again whenever a different field arrives, so a component that puts all of its - /// GetAttribute calls here can never render a stale setting. - /// - /// - /// ⛔ Assign every cached property on every call, including back to its default. The override - /// is a reload, not a patch: a property left untouched because the new field does not declare that - /// attribute keeps the previous field's value, which is the same bug in a smaller box. A - /// field that dropped its mask would go on masking. - /// - /// - /// Configuration-shaped diagnostics belong here too — they describe the field, so a new field - /// deserves its own verdict, and any per-instance latch they use should be reset alongside the - /// properties it guards. - /// - /// - protected virtual void OnFieldConfigurationChanged() - { } /// diff --git a/FormCraft/Components/FieldComponentBase.cs b/FormCraft/Components/FieldComponentBase.cs index 2785266e..c50cbaa1 100644 --- a/FormCraft/Components/FieldComponentBase.cs +++ b/FormCraft/Components/FieldComponentBase.cs @@ -58,12 +58,21 @@ protected virtual async Task NotifyValueChangedAsync(TValue? value) StateHasChanged(); // Force re-render after value change } + /// + /// Tracks which field this instance's cached configuration was loaded from (#298, #335). + /// + private readonly FieldConfigurationTracker _fieldTracker = new(); + /// protected override void OnInitialized() { base.OnInitialized(); LoadValueFromModel(); _isInitialized = true; + + // Before the derived component's own OnInitialized body runs — it calls base.OnInitialized() + // first, so its configuration is loaded by the time the rest of that body looks at it. + RefreshFieldConfigurationIfChanged(); } /// @@ -76,6 +85,55 @@ protected override void OnParametersSet() { LoadValueFromModel(); } + + // Blazor reuses a component instance whenever the render-tree shape matches, so this is the + // only place a component learns it has been handed a different field. Without it the instance + // renders the previous field's settings indefinitely — silently, with plausible-looking + // output (#298 for MudBlazor, #335 for Fluent UI). + RefreshFieldConfigurationIfChanged(); + } + + /// + /// Calls when, and only when, the field changed. + /// + /// + /// The guard — see — is what makes this affordable: + /// runs on every keystroke for an immediately-bound input, so the + /// alternative is re-reading every attribute per character typed. + /// + private void RefreshFieldConfigurationIfChanged() + { + if (_fieldTracker.HasChanged(Context?.Field)) + { + OnFieldConfigurationChanged(); + } + } + + /// + /// Reads everything this component caches from Context.Field. Called once per field. + /// + /// + /// + /// Override this instead of loading configuration in OnInitialized. It runs on first render + /// and again whenever a different field arrives, so a component that puts all of its + /// GetAttribute calls here can never render a stale setting. + /// + /// + /// ⛔ Assign every cached property on every call, including back to its default. The + /// override is a reload, not a patch: a property left untouched because the new field does not + /// declare that attribute keeps the previous field's value, which is the same bug in a + /// smaller box. Watch for two shapes in particular, both of which shipped and had to be fixed — + /// X = GetAttribute(…) ?? X, which reads as "keep the default" and means "keep the previous + /// field's value" on a reload; and an assignment guarded by if (value != null). + /// + /// + /// State derived from the configuration counts too — display text, a selected-items list, a + /// revealed-password flag — along with any per-instance diagnostic latch, since a new field + /// deserves its own verdict. + /// + /// + protected virtual void OnFieldConfigurationChanged() + { } /// diff --git a/FormCraft.ForMudBlazor/Fields/FieldConfigurationTracker.cs b/FormCraft/Components/FieldConfigurationTracker.cs similarity index 76% rename from FormCraft.ForMudBlazor/Fields/FieldConfigurationTracker.cs rename to FormCraft/Components/FieldConfigurationTracker.cs index 5dee7ce7..76f577b9 100644 --- a/FormCraft.ForMudBlazor/Fields/FieldConfigurationTracker.cs +++ b/FormCraft/Components/FieldConfigurationTracker.cs @@ -1,4 +1,4 @@ -namespace FormCraft.ForMudBlazor; +namespace FormCraft; /// /// Tracks which field a component has loaded its configuration from, so it can tell when it has been @@ -27,10 +27,13 @@ namespace FormCraft.ForMudBlazor; /// had to qualify around. /// /// -/// It lives in its own type because two unrelated base classes need it — -/// MudBlazorFieldComponentBase and MudBlazorFileUploadComponentBase, which share only -/// FieldComponentBase in the UI-agnostic core. Copying three lines and their reasoning into -/// both is how this package acquired the duplication #284 exists to undo. +/// It lives in core, beside , because every adapter +/// needs it and it references no UI type — one field and a +/// call. #298 shipped it inside the MudBlazor package and had to +/// wire it in three places there (the field base, the file-upload base, and the boolean +/// component, which derives from directly); #335 +/// found the Fluent adapter needed the same again. Three wirings in one package was the signal that +/// this belongs a layer down — copying it per adapter is the duplication #279 spent a PR undoing. /// /// internal sealed class FieldConfigurationTracker diff --git a/README.md b/README.md index 2acbfdd8..8e6ca073 100644 --- a/README.md +++ b/README.md @@ -68,11 +68,13 @@ Experience FormCraft in action! Visit our [interactive demo](https://phmatray.gi **Editing one row no longer validates the whole collection.** A keystroke in a row raises a field-change notification, and handling it used to validate every item × every field and then discard all but the edited cell — 250 validator invocations per character on a 50-row × 5-field form, 249 of them thrown away. It now validates just that cell. Together with #269 and #312, which removed the per-render and per-validation `Expression.Compile()`, the **validation** cost of a keystroke no longer scales with the number of rows. Rendering still does: a keystroke re-renders every row of the collection, so a large grid can still feel heavy while typing — that part is not addressed here. -- **A field component now re-reads its configuration when it is handed a different field (#298).** Every MudBlazor field component read its settings once, in `OnInitialized`, and never looked again. Blazor reuses a component instance whenever the render-tree shape matches, so an instance could be given a different field while those cached attributes still described the previous one — and it would go on rendering that field's mask, adornment, input type, numeric format and select options indefinitely. Nothing threw and nothing logged; the field simply showed the wrong thing, plausibly enough that it read as correct. +- **A field component now re-reads its configuration when it is handed a different field — in both adapters (#298, #335).** Every field component read its settings once, in `OnInitialized`, and never looked again. Blazor reuses a component instance whenever the render-tree shape matches, so an instance could be given a different field while those cached attributes still described the previous one — and it would go on rendering that field's mask, adornment, input type, numeric bounds and select options indefinitely. Nothing threw and nothing logged; the field simply showed the wrong thing, plausibly enough that it read as correct. **How you hit it:** by **swapping `FormCraftComponent.Configuration`** on a live form — a wizard step, a mode toggle, anything that renders a different form over the same component tree. - Components now load their configuration in a new `OnFieldConfigurationChanged()` hook, called on first render and again whenever the field changes — guarded by a reference comparison on the field itself, so an ordinary re-render (which happens on every keystroke) costs one comparison rather than a re-read of every attribute. State derived from the configuration is reset with it, including the password-visibility toggle: a revealed password on the old field must not leave the new field's secret rendered in clear text. + Components now load their configuration in an `OnFieldConfigurationChanged()` hook, called on first render and again whenever the field changes — guarded by a reference comparison on the field itself, so an ordinary re-render (which happens on every keystroke) costs one comparison rather than a re-read of every attribute. State derived from the configuration is reset with it, including the password-visibility toggle: a revealed password on the old field must not leave the new field's secret rendered in clear text. + + **The hook lives on `FieldComponentBase` in `FormCraft` core**, so both the MudBlazor and Fluent UI adapters inherit it and a custom component gets it wherever it sits. It started inside the MudBlazor package and had to be wired three times there — that package has three component hierarchies — which was the signal it belonged a layer down; #335 moved it before the Fluent adapter could become a fourth copy, the drift this library has already paid for in #146, #177, #184, #190, #203 and #279. **Collection rows are deliberately *not* keyed.** Blazor matches rows by position, so removing one re-points each surviving component at its neighbour's data. Displayed values are unaffected — those reload from the model — but component *identity* is, and with it any state the value does not restore. Keying the loop on the item was tried and reverted: Blazor compares keys with `Equals`, and item types are constrained only to `new()`, so two `record` or `struct` rows with equal content are a duplicate key and the render throws. On a record-typed item form, adding two empty rows was enough. The remaining identity issue is tracked separately.