From b328fd8332a118866d509bb21a6d6964138c8985 Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Wed, 4 Mar 2026 14:35:27 +0100 Subject: [PATCH 01/10] Resolve duplicate HTML IDs in submission form. --- ...amic-form-control-container.component.html | 2 +- ...ynamic-form-control-container.component.ts | 34 +++++++- .../dynamic-scrollable-dropdown.component.ts | 37 ++++++++- .../ds-dynamic-form-ui/unique-id-registry.ts | 77 +++++++++++++++++++ .../section-license.component.html | 2 +- 5 files changed, 148 insertions(+), 4 deletions(-) create mode 100644 src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html index 606db29db37..9e73b55f751 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html @@ -3,7 +3,7 @@ [formGroup]="group" [ngClass]="[getClass('element', 'container'), getClass('grid', 'container')]"> diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts index f2ea9c3baf2..160df6ee3c9 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts @@ -125,6 +125,7 @@ import { DYNAMIC_FORM_CONTROL_TYPE_AUTOCOMPLETE } from './models/autocomplete/ds import { DsDynamicSponsorAutocompleteComponent } from './models/sponsor-autocomplete/ds-dynamic-sponsor-autocomplete.component'; import { SPONSOR_METADATA_NAME } from './models/ds-dynamic-complex.model'; import { DsDynamicSponsorScrollableDropdownComponent } from './models/sponsor-scrollable-dropdown/dynamic-sponsor-scrollable-dropdown.component'; +import { UniqueIdRegistry } from './unique-id-registry'; export function dsDynamicFormControlMapFn(model: DynamicFormControlModel): Type | null { switch (model.type) { @@ -210,6 +211,19 @@ export function dsDynamicFormControlMapFn(model: DynamicFormControlModel): Type< changeDetection: ChangeDetectionStrategy.Default }) export class DsDynamicFormControlContainerComponent extends DynamicFormControlContainerComponent implements OnInit, OnChanges, OnDestroy { + + /** + * The unique element ID assigned to this component instance. + * For the first occurrence of a base ID, this equals the base ID (preserving backward compatibility). + * For subsequent occurrences, a numeric suffix is appended (e.g., `baseId_1`, `baseId_2`). + */ + private _uniqueId: string; + + /** + * The base element ID (from getElementId) before deduplication. + */ + private _baseId: string; + @ContentChildren(DynamicTemplateDirective) contentTemplateList: QueryList; // eslint-disable-next-line @angular-eslint/no-input-rename @Input('templates') inputTemplateList: QueryList; @@ -254,6 +268,20 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo */ fetchThumbnail: boolean; + /** + * Returns a unique element ID for this component instance. + * The first occurrence of a base ID keeps the original value (e.g., `dc_title`). + * Subsequent occurrences receive a numeric suffix (e.g., `metashare_..._mediaType_1`). + */ + get id(): string { + if (!this._uniqueId) { + this._baseId = this.layoutService.getElementId(this.model); + const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; + this._uniqueId = UniqueIdRegistry.register(this._baseId, instanceKey); + } + return this._uniqueId; + } + get componentType(): Type | null { return dsDynamicFormControlMapFn(this.model); } @@ -499,9 +527,13 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo } /** - * Unsubscribe from all subscriptions + * Unsubscribe from all subscriptions and release the unique ID from the registry. */ ngOnDestroy(): void { + if (this._uniqueId && this._baseId) { + const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; + UniqueIdRegistry.release(this._baseId, instanceKey); + } this.subs .filter((sub) => hasValue(sub)) .forEach((sub) => sub.unsubscribe()); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts index b284fa4fc5e..13453dc2903 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts @@ -3,6 +3,7 @@ import { Component, EventEmitter, Input, + OnDestroy, OnInit, Output, ViewChild, @@ -29,6 +30,7 @@ import { FormFieldMetadataValueObject } from '../../../models/form-field-metadat import { FindAllData } from '../../../../../../core/data/base/find-all-data'; import { CacheableObject } from '../../../../../../core/cache/cacheable-object.model'; import { RemoteData } from '../../../../../../core/data/remote-data'; +import { UniqueIdRegistry } from '../../unique-id-registry'; /** * Component representing a dropdown input field @@ -38,7 +40,7 @@ import { RemoteData } from '../../../../../../core/data/remote-data'; styleUrls: ['./dynamic-scrollable-dropdown.component.scss'], templateUrl: './dynamic-scrollable-dropdown.component.html' }) -export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyComponent implements OnInit { +export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyComponent implements OnInit, OnDestroy { @ViewChild('dropdownMenu', { read: ElementRef }) dropdownMenu: ElementRef; @Input() bindId = true; @@ -57,6 +59,29 @@ export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyCom public selectedIndex = 0; public acceptableKeys = ['Space', 'NumpadMultiply', 'NumpadAdd', 'NumpadSubtract', 'NumpadDecimal', 'Semicolon', 'Equal', 'Comma', 'Minus', 'Period', 'Quote', 'Backquote']; + /** + * The unique element ID assigned to this component instance. + */ + private _uniqueId: string; + + /** + * The base element ID before deduplication. + */ + private _baseId: string; + + /** + * Returns a unique element ID for this scrollable dropdown instance. + * Prevents duplicate HTML IDs when the same model appears in multiple form groups. + */ + get id(): string { + if (!this._uniqueId) { + this._baseId = this.layoutService.getElementId(this.model); + const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; + this._uniqueId = UniqueIdRegistry.register(this._baseId, instanceKey); + } + return this._uniqueId; + } + /** * If true the component can rely on the findAll method for data loading. * This is a behaviour activated by dependency injection through the dropdown config. @@ -293,4 +318,14 @@ export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyCom this.currentValue = result; } + /** + * Release the unique ID from the registry when the component is destroyed. + */ + ngOnDestroy(): void { + if (this._uniqueId && this._baseId) { + const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; + UniqueIdRegistry.release(this._baseId, instanceKey); + } + } + } diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts new file mode 100644 index 00000000000..5c59d458258 --- /dev/null +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts @@ -0,0 +1,77 @@ +/** + * Static registry for generating unique HTML element IDs across dynamic form components. + * + * When the same form model ID appears multiple times in the DOM (e.g., scrollable dropdowns + * for mediaType/detailedType in different type-bound form groups), this registry ensures + * each rendered instance receives a unique HTML ID. + * + * - First occurrence: keeps the original base ID (backward compatible). + * - Subsequent occurrences: appends a numeric suffix (`_1`, `_2`, etc.). + * + * Components must call `register()` during initialization and `release()` during destruction. + */ +export class UniqueIdRegistry { + + /** + * Tracks how many active instances exist for each base element ID. + * Key = base element ID, Value = number of active instances. + */ + private static idCounts: Map = new Map(); + + /** + * Tracks the assigned suffix for each component instance. + * Key = a unique instance token (component + model-based), Value = the suffix index assigned. + */ + private static instanceSuffixes: Map = new Map(); + + /** + * Register a base ID and return a unique ID for this instance. + * The first occurrence returns the base ID unchanged. + * Subsequent occurrences return `baseId_N` where N is the occurrence index (1, 2, ...). + * + * @param baseId The base element ID (from getElementId). + * @param instanceKey A unique key identifying this specific component instance. + * @returns The unique element ID to use in the DOM. + */ + static register(baseId: string, instanceKey: string): string { + // If this instance was already registered, return its existing ID + if (this.instanceSuffixes.has(instanceKey)) { + const suffix = this.instanceSuffixes.get(instanceKey); + return suffix === 0 ? baseId : `${baseId}_${suffix}`; + } + + const count = this.idCounts.get(baseId) || 0; + this.idCounts.set(baseId, count + 1); + this.instanceSuffixes.set(instanceKey, count); + return count === 0 ? baseId : `${baseId}_${count}`; + } + + /** + * Release the unique ID when a component is destroyed. + * Only decrements the count when the instanceKey is still in the registry + * (prevents double-release by container and child component sharing the same key). + * + * @param baseId The base element ID. + * @param instanceKey The unique key used during registration. + */ + static release(baseId: string, instanceKey: string): void { + if (!this.instanceSuffixes.has(instanceKey)) { + return; + } + this.instanceSuffixes.delete(instanceKey); + const count = this.idCounts.get(baseId) || 0; + if (count <= 1) { + this.idCounts.delete(baseId); + } else { + this.idCounts.set(baseId, count - 1); + } + } + + /** + * Clear the entire registry. Used primarily in tests. + */ + static clear(): void { + this.idCounts.clear(); + this.instanceSuffixes.clear(); + } +} diff --git a/src/app/submission/sections/clarin-license-resource/section-license.component.html b/src/app/submission/sections/clarin-license-resource/section-license.component.html index ea14ada884a..31e671a0187 100644 --- a/src/app/submission/sections/clarin-license-resource/section-license.component.html +++ b/src/app/submission/sections/clarin-license-resource/section-license.component.html @@ -68,7 +68,7 @@ *ngFor="let license of filteredLicenses4Selector" (click)="selectLicense(license.id)" [value]="license.id" - id="license_option_{{ license.id }}"> + [id]="'license_option_' + license.id"> {{license.licenseLabel}} {{license.name}} From 6142afed5e87f118a144feff110ebaa01935cdb5 Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Wed, 4 Mar 2026 16:25:35 +0100 Subject: [PATCH 02/10] refactor for unique ID registry --- ...amic-form-control-container.component.html | 2 +- .../ds-dynamic-form-ui/unique-id-registry.ts | 33 +++++++------------ src/test.ts | 3 ++ 3 files changed, 15 insertions(+), 23 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html index 9e73b55f751..606db29db37 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html @@ -3,7 +3,7 @@ [formGroup]="group" [ngClass]="[getClass('element', 'container'), getClass('grid', 'container')]"> diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts index 5c59d458258..ab72fff6231 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts @@ -13,10 +13,11 @@ export class UniqueIdRegistry { /** - * Tracks how many active instances exist for each base element ID. - * Key = base element ID, Value = number of active instances. + * Monotonic counter per base ID. Always increments, never decrements, + * so released suffixes are never reissued to a different instance. + * Key = base element ID, Value = next suffix to assign. */ - private static idCounts: Map = new Map(); + private static nextSuffix: Map = new Map(); /** * Tracks the assigned suffix for each component instance. @@ -40,38 +41,26 @@ export class UniqueIdRegistry { return suffix === 0 ? baseId : `${baseId}_${suffix}`; } - const count = this.idCounts.get(baseId) || 0; - this.idCounts.set(baseId, count + 1); - this.instanceSuffixes.set(instanceKey, count); - return count === 0 ? baseId : `${baseId}_${count}`; + const suffix = this.nextSuffix.get(baseId) || 0; + this.nextSuffix.set(baseId, suffix + 1); + this.instanceSuffixes.set(instanceKey, suffix); + return suffix === 0 ? baseId : `${baseId}_${suffix}`; } /** * Release the unique ID when a component is destroyed. - * Only decrements the count when the instanceKey is still in the registry - * (prevents double-release by container and child component sharing the same key). * - * @param baseId The base element ID. * @param instanceKey The unique key used during registration. */ - static release(baseId: string, instanceKey: string): void { - if (!this.instanceSuffixes.has(instanceKey)) { - return; - } + static release(instanceKey: string): void { this.instanceSuffixes.delete(instanceKey); - const count = this.idCounts.get(baseId) || 0; - if (count <= 1) { - this.idCounts.delete(baseId); - } else { - this.idCounts.set(baseId, count - 1); - } } /** - * Clear the entire registry. Used primarily in tests. + * Clear the entire registry. Used in tests to reset state between specs. */ static clear(): void { - this.idCounts.clear(); + this.nextSuffix.clear(); this.instanceSuffixes.clear(); } } diff --git a/src/test.ts b/src/test.ts index 2f07cf0d1da..4928069fc21 100644 --- a/src/test.ts +++ b/src/test.ts @@ -8,6 +8,7 @@ import { platformBrowserDynamicTesting } from '@angular/platform-browser-dynamic/testing'; import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; +import { UniqueIdRegistry } from './app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry'; // First, initialize the Angular testing environment. getTestBed().initTestEnvironment( @@ -21,4 +22,6 @@ jasmine.getEnv().afterEach(() => { getTestBed().inject(MockStore, null)?.resetSelectors(); // Close any leftover modals getTestBed().inject(NgbModal, null)?.dismissAll?.(); + // Reset unique ID registry to prevent static state leaking between tests + UniqueIdRegistry.clear(); }); From 18b0bbd53d1261ce27dc8a102dd53718c2e67bdd Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Wed, 4 Mar 2026 16:44:02 +0100 Subject: [PATCH 03/10] fix: remove unused baseId param from release in UniqueIdRegistry --- .../ds-dynamic-form-control-container.component.ts | 8 ++++---- .../dynamic-scrollable-dropdown.component.ts | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts index 160df6ee3c9..5da9b36a0dc 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts @@ -215,7 +215,7 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo /** * The unique element ID assigned to this component instance. * For the first occurrence of a base ID, this equals the base ID (preserving backward compatibility). - * For subsequent occurrences, a numeric suffix is appended (e.g., `baseId_1`, `baseId_2`). + * For subsequent occurrences, a numeric suffix is appended. */ private _uniqueId: string; @@ -270,8 +270,8 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo /** * Returns a unique element ID for this component instance. - * The first occurrence of a base ID keeps the original value (e.g., `dc_title`). - * Subsequent occurrences receive a numeric suffix (e.g., `metashare_..._mediaType_1`). + * The first occurrence of a base ID keeps the original value. + * Subsequent occurrences receive a numeric suffix. */ get id(): string { if (!this._uniqueId) { @@ -532,7 +532,7 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo ngOnDestroy(): void { if (this._uniqueId && this._baseId) { const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; - UniqueIdRegistry.release(this._baseId, instanceKey); + UniqueIdRegistry.release(instanceKey); } this.subs .filter((sub) => hasValue(sub)) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts index 13453dc2903..55918033f55 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts @@ -324,7 +324,7 @@ export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyCom ngOnDestroy(): void { if (this._uniqueId && this._baseId) { const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; - UniqueIdRegistry.release(this._baseId, instanceKey); + UniqueIdRegistry.release(instanceKey); } } From a5e07f0309ee1681c8695fb056da5825972c3c8a Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 5 Mar 2026 09:43:54 +0100 Subject: [PATCH 04/10] fix: propagate unique ID from container to child form controls for accessibility --- .../ds-dynamic-form-control-container.component.html | 2 +- .../ds-dynamic-form-control-container.component.ts | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html index 606db29db37..9e73b55f751 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html @@ -3,7 +3,7 @@ [formGroup]="group" [ngClass]="[getClass('element', 'container'), getClass('grid', 'container')]"> diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts index 5da9b36a0dc..537c69cee3c 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts @@ -431,6 +431,18 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo (instance as any).formModel = this.formModel; (instance as any).formGroup = this.formGroup; } + + // Propagate the container's unique ID to the child form control component + // so that the child's rendered element id matches the container's label[for]. + // Without this, child components use layoutService.getElementId(model) which + // returns the base ID, while the container may return a suffixed ID from UniqueIdRegistry. + if (this.componentRef?.instance) { + const uniqueId = this.id; + Object.defineProperty(this.componentRef.instance, 'id', { + get: () => uniqueId, + configurable: true, + }); + } } } From a7378102334972725cc1dd1d80e96ea3acedf0c0 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 5 Mar 2026 11:28:47 +0100 Subject: [PATCH 05/10] fix: recycle released suffixes in UniqueIdRegistry to preserve stable IDs across re-renders --- .../ds-dynamic-form-ui/unique-id-registry.ts | 53 +++++++++++++------ 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts index ab72fff6231..5a266d8da18 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts @@ -8,27 +8,28 @@ * - First occurrence: keeps the original base ID (backward compatible). * - Subsequent occurrences: appends a numeric suffix (`_1`, `_2`, etc.). * + * Released suffixes are recycled so that a single live instance always receives the base ID. * Components must call `register()` during initialization and `release()` during destruction. */ export class UniqueIdRegistry { /** - * Monotonic counter per base ID. Always increments, never decrements, - * so released suffixes are never reissued to a different instance. - * Key = base element ID, Value = next suffix to assign. + * Tracks which suffixes are currently in use for each base ID. + * Key = base element ID, Value = set of active suffix numbers. */ - private static nextSuffix: Map = new Map(); + private static activeSuffixes: Map> = new Map>(); /** - * Tracks the assigned suffix for each component instance. - * Key = a unique instance token (component + model-based), Value = the suffix index assigned. + * Tracks the assigned base ID and suffix for each component instance. + * Key = a unique instance token (component + model-based), + * Value = { baseId, suffix } assigned to that instance. */ - private static instanceSuffixes: Map = new Map(); + private static instances: Map = new Map(); /** * Register a base ID and return a unique ID for this instance. * The first occurrence returns the base ID unchanged. - * Subsequent occurrences return `baseId_N` where N is the occurrence index (1, 2, ...). + * Subsequent occurrences return `baseId_N` where N is the lowest available suffix (1, 2, ...). * * @param baseId The base element ID (from getElementId). * @param instanceKey A unique key identifying this specific component instance. @@ -36,31 +37,49 @@ export class UniqueIdRegistry { */ static register(baseId: string, instanceKey: string): string { // If this instance was already registered, return its existing ID - if (this.instanceSuffixes.has(instanceKey)) { - const suffix = this.instanceSuffixes.get(instanceKey); - return suffix === 0 ? baseId : `${baseId}_${suffix}`; + const existing = this.instances.get(instanceKey); + if (existing) { + return existing.suffix === 0 ? baseId : `${baseId}_${existing.suffix}`; } - const suffix = this.nextSuffix.get(baseId) || 0; - this.nextSuffix.set(baseId, suffix + 1); - this.instanceSuffixes.set(instanceKey, suffix); + // Find the lowest available suffix for this base ID + const active = this.activeSuffixes.get(baseId) || new Set(); + let suffix = 0; + while (active.has(suffix)) { + suffix++; + } + + active.add(suffix); + this.activeSuffixes.set(baseId, active); + this.instances.set(instanceKey, { baseId, suffix }); return suffix === 0 ? baseId : `${baseId}_${suffix}`; } /** * Release the unique ID when a component is destroyed. + * The released suffix becomes available for reuse by future registrations. * * @param instanceKey The unique key used during registration. */ static release(instanceKey: string): void { - this.instanceSuffixes.delete(instanceKey); + const entry = this.instances.get(instanceKey); + if (entry) { + const active = this.activeSuffixes.get(entry.baseId); + if (active) { + active.delete(entry.suffix); + if (active.size === 0) { + this.activeSuffixes.delete(entry.baseId); + } + } + } + this.instances.delete(instanceKey); } /** * Clear the entire registry. Used in tests to reset state between specs. */ static clear(): void { - this.nextSuffix.clear(); - this.instanceSuffixes.clear(); + this.activeSuffixes.clear(); + this.instances.clear(); } } From 52f17a6697e7a30ed03a17a53ba2d581d4999e72 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Thu, 5 Mar 2026 15:30:01 +0100 Subject: [PATCH 06/10] refactor: deduplicate UniqueIdRegistry usage, store instanceKey, and add unit tests --- ...ynamic-form-control-container.component.ts | 21 +++- .../dynamic-scrollable-dropdown.component.ts | 37 +------ .../unique-id-registry.spec.ts | 104 ++++++++++++++++++ .../ds-dynamic-form-ui/unique-id-registry.ts | 2 +- 4 files changed, 122 insertions(+), 42 deletions(-) create mode 100644 src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.spec.ts diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts index 537c69cee3c..61a9e6176f9 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts @@ -224,6 +224,13 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo */ private _baseId: string; + /** + * The instance key used when registering with UniqueIdRegistry. + * Stored so that ngOnDestroy can release with the exact same key, + * even if the model's parent has been reassigned. + */ + private _instanceKey: string; + @ContentChildren(DynamicTemplateDirective) contentTemplateList: QueryList; // eslint-disable-next-line @angular-eslint/no-input-rename @Input('templates') inputTemplateList: QueryList; @@ -276,8 +283,8 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo get id(): string { if (!this._uniqueId) { this._baseId = this.layoutService.getElementId(this.model); - const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; - this._uniqueId = UniqueIdRegistry.register(this._baseId, instanceKey); + this._instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; + this._uniqueId = UniqueIdRegistry.register(this._baseId, this._instanceKey); } return this._uniqueId; } @@ -436,6 +443,11 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo // so that the child's rendered element id matches the container's label[for]. // Without this, child components use layoutService.getElementId(model) which // returns the base ID, while the container may return a suffixed ID from UniqueIdRegistry. + // + // Object.defineProperty is used because many child components come from the + // third-party @ng-dynamic-forms/ui-ng-bootstrap library and cannot be modified + // to accept an @Input() override. The instance-level property takes precedence + // over the prototype getter in all child components (both third-party and custom). if (this.componentRef?.instance) { const uniqueId = this.id; Object.defineProperty(this.componentRef.instance, 'id', { @@ -542,9 +554,8 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo * Unsubscribe from all subscriptions and release the unique ID from the registry. */ ngOnDestroy(): void { - if (this._uniqueId && this._baseId) { - const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; - UniqueIdRegistry.release(instanceKey); + if (this._instanceKey) { + UniqueIdRegistry.release(this._instanceKey); } this.subs .filter((sub) => hasValue(sub)) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts index 55918033f55..b284fa4fc5e 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.ts @@ -3,7 +3,6 @@ import { Component, EventEmitter, Input, - OnDestroy, OnInit, Output, ViewChild, @@ -30,7 +29,6 @@ import { FormFieldMetadataValueObject } from '../../../models/form-field-metadat import { FindAllData } from '../../../../../../core/data/base/find-all-data'; import { CacheableObject } from '../../../../../../core/cache/cacheable-object.model'; import { RemoteData } from '../../../../../../core/data/remote-data'; -import { UniqueIdRegistry } from '../../unique-id-registry'; /** * Component representing a dropdown input field @@ -40,7 +38,7 @@ import { UniqueIdRegistry } from '../../unique-id-registry'; styleUrls: ['./dynamic-scrollable-dropdown.component.scss'], templateUrl: './dynamic-scrollable-dropdown.component.html' }) -export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyComponent implements OnInit, OnDestroy { +export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyComponent implements OnInit { @ViewChild('dropdownMenu', { read: ElementRef }) dropdownMenu: ElementRef; @Input() bindId = true; @@ -59,29 +57,6 @@ export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyCom public selectedIndex = 0; public acceptableKeys = ['Space', 'NumpadMultiply', 'NumpadAdd', 'NumpadSubtract', 'NumpadDecimal', 'Semicolon', 'Equal', 'Comma', 'Minus', 'Period', 'Quote', 'Backquote']; - /** - * The unique element ID assigned to this component instance. - */ - private _uniqueId: string; - - /** - * The base element ID before deduplication. - */ - private _baseId: string; - - /** - * Returns a unique element ID for this scrollable dropdown instance. - * Prevents duplicate HTML IDs when the same model appears in multiple form groups. - */ - get id(): string { - if (!this._uniqueId) { - this._baseId = this.layoutService.getElementId(this.model); - const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; - this._uniqueId = UniqueIdRegistry.register(this._baseId, instanceKey); - } - return this._uniqueId; - } - /** * If true the component can rely on the findAll method for data loading. * This is a behaviour activated by dependency injection through the dropdown config. @@ -318,14 +293,4 @@ export class DsDynamicScrollableDropdownComponent extends DsDynamicVocabularyCom this.currentValue = result; } - /** - * Release the unique ID from the registry when the component is destroyed. - */ - ngOnDestroy(): void { - if (this._uniqueId && this._baseId) { - const instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; - UniqueIdRegistry.release(instanceKey); - } - } - } diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.spec.ts new file mode 100644 index 00000000000..51d444c6732 --- /dev/null +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.spec.ts @@ -0,0 +1,104 @@ +import { UniqueIdRegistry } from './unique-id-registry'; + +describe('UniqueIdRegistry', () => { + + afterEach(() => { + UniqueIdRegistry.clear(); + }); + + describe('register', () => { + it('should return the base ID for the first registration', () => { + const id = UniqueIdRegistry.register('email', 'email_root'); + expect(id).toBe('email'); + }); + + it('should return a suffixed ID for the second registration of the same base ID', () => { + UniqueIdRegistry.register('email', 'email_root'); + const id2 = UniqueIdRegistry.register('email', 'email_group1'); + expect(id2).toBe('email_1'); + }); + + it('should return incrementing suffixes for multiple registrations', () => { + UniqueIdRegistry.register('email', 'email_root'); + const id2 = UniqueIdRegistry.register('email', 'email_group1'); + const id3 = UniqueIdRegistry.register('email', 'email_group2'); + expect(id2).toBe('email_1'); + expect(id3).toBe('email_2'); + }); + + it('should return the cached value for idempotent re-registration', () => { + const id1 = UniqueIdRegistry.register('email', 'email_root'); + const id2 = UniqueIdRegistry.register('email', 'email_root'); + expect(id1).toBe(id2); + expect(id1).toBe('email'); + }); + + it('should handle independent base IDs separately', () => { + const id1 = UniqueIdRegistry.register('email', 'email_root'); + const id2 = UniqueIdRegistry.register('name', 'name_root'); + expect(id1).toBe('email'); + expect(id2).toBe('name'); + }); + }); + + describe('release', () => { + it('should free suffix 0 so a new registration reuses the base ID', () => { + UniqueIdRegistry.register('email', 'email_root'); + UniqueIdRegistry.release('email_root'); + + const id = UniqueIdRegistry.register('email', 'email_new'); + expect(id).toBe('email'); + }); + + it('should free a suffixed entry so a new registration reuses that suffix', () => { + UniqueIdRegistry.register('email', 'email_root'); + UniqueIdRegistry.register('email', 'email_group1'); + UniqueIdRegistry.release('email_group1'); + + const id = UniqueIdRegistry.register('email', 'email_group2'); + expect(id).toBe('email_1'); + }); + + it('should be a no-op for an unknown instance key', () => { + UniqueIdRegistry.register('email', 'email_root'); + UniqueIdRegistry.release('unknown_key'); + + // Original registration still works + const id = UniqueIdRegistry.register('email', 'email_root'); + expect(id).toBe('email'); + }); + + it('should assign lowest available suffix after releasing middle suffix', () => { + UniqueIdRegistry.register('x', 'x_a'); // suffix 0 + UniqueIdRegistry.register('x', 'x_b'); // suffix 1 + UniqueIdRegistry.register('x', 'x_c'); // suffix 2 + + UniqueIdRegistry.release('x_b'); // free suffix 1 + + const id = UniqueIdRegistry.register('x', 'x_d'); + expect(id).toBe('x_1'); + }); + }); + + describe('clear', () => { + it('should reset all state so registrations start fresh', () => { + UniqueIdRegistry.register('email', 'email_root'); + UniqueIdRegistry.register('email', 'email_group1'); + UniqueIdRegistry.clear(); + + const id = UniqueIdRegistry.register('email', 'email_root'); + expect(id).toBe('email'); + }); + }); + + describe('destroy and re-create scenario', () => { + it('should return the base ID when the sole instance is destroyed and recreated', () => { + // Simulates a form field being destroyed (e.g., type change) and recreated + UniqueIdRegistry.register('local_hasCMDI', 'local_hasCMDI_root'); + UniqueIdRegistry.release('local_hasCMDI_root'); + + const id = UniqueIdRegistry.register('local_hasCMDI', 'local_hasCMDI_root'); + expect(id).toBe('local_hasCMDI'); + }); + }); +}); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts index 5a266d8da18..38c7be4c37f 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts @@ -8,7 +8,7 @@ * - First occurrence: keeps the original base ID (backward compatible). * - Subsequent occurrences: appends a numeric suffix (`_1`, `_2`, etc.). * - * Released suffixes are recycled so that a single live instance always receives the base ID. + * Released suffixes are recycled and may be assigned to future registrations. * Components must call `register()` during initialization and `release()` during destruction. */ export class UniqueIdRegistry { From 709804b1136d742adc86e07a80fc7d20b06a4a22 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Fri, 6 Mar 2026 12:13:12 +0100 Subject: [PATCH 07/10] Refactored the code to make this fix more simple --- ...c-form-control-container.component.spec.ts | 63 +++++++++++ ...ynamic-form-control-container.component.ts | 43 ++------ .../unique-id-registry.spec.ts | 104 ------------------ .../ds-dynamic-form-ui/unique-id-registry.ts | 85 -------------- src/test.ts | 3 - 5 files changed, 72 insertions(+), 226 deletions(-) delete mode 100644 src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.spec.ts delete mode 100644 src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts index 355e10b9a09..52354cc75c3 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts @@ -377,4 +377,67 @@ describe('DsDynamicFormControlContainerComponent test suite', () => { expect(testFn(formModel[25])).toEqual(DsDynamicFormGroupComponent); }); + describe('unique id generation', () => { + it('should return the base element ID when the model has no parent', () => { + // testModel (formModel[8]) is a root-level DynamicInputModel with id 'input' + // and has no parent, so id should equal the base element ID. + expect(component.model.parent).toBeFalsy(); + expect(component.id).toBe(component.model.id); + }); + + it('should append the parent ID when the model has a parent', () => { + const parentGroup = new DynamicFormGroupModel({ + id: 'parentSection', + group: [new DynamicInputModel({ id: 'email' })] + }); + const childModel = parentGroup.group[0]; + // parent is set by DynamicFormService.createFormGroup, not by the constructor + childModel.parent = parentGroup; + + component.model = childModel; + expect(component.id).toBe('email_parentSection'); + }); + + it('should produce different IDs for the same field in different parent groups', () => { + const group1 = new DynamicFormGroupModel({ + id: 'sectionA', + group: [new DynamicInputModel({ id: 'title' })] + }); + const group2 = new DynamicFormGroupModel({ + id: 'sectionB', + group: [new DynamicInputModel({ id: 'title' })] + }); + // parent is set by DynamicFormService.createFormGroup, not by the constructor + group1.group[0].parent = group1; + group2.group[0].parent = group2; + + component.model = group1.group[0]; + const id1 = component.id; + + component.model = group2.group[0]; + const id2 = component.id; + + expect(id1).toBe('title_sectionA'); + expect(id2).toBe('title_sectionB'); + expect(id1).not.toBe(id2); + }); + + it('should return the same id on repeated access (idempotent)', () => { + const parentGroup = new DynamicFormGroupModel({ + id: 'myGroup', + group: [new DynamicInputModel({ id: 'field' })] + }); + const childModel = parentGroup.group[0]; + // parent is set by DynamicFormService.createFormGroup, not by the constructor + childModel.parent = parentGroup; + + component.model = childModel; + + const first = component.id; + const second = component.id; + expect(first).toBe(second); + expect(first).toBe('field_myGroup'); + }); + }); + }); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts index 61a9e6176f9..4466dea93e9 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts @@ -125,7 +125,6 @@ import { DYNAMIC_FORM_CONTROL_TYPE_AUTOCOMPLETE } from './models/autocomplete/ds import { DsDynamicSponsorAutocompleteComponent } from './models/sponsor-autocomplete/ds-dynamic-sponsor-autocomplete.component'; import { SPONSOR_METADATA_NAME } from './models/ds-dynamic-complex.model'; import { DsDynamicSponsorScrollableDropdownComponent } from './models/sponsor-scrollable-dropdown/dynamic-sponsor-scrollable-dropdown.component'; -import { UniqueIdRegistry } from './unique-id-registry'; export function dsDynamicFormControlMapFn(model: DynamicFormControlModel): Type | null { switch (model.type) { @@ -212,25 +211,6 @@ export function dsDynamicFormControlMapFn(model: DynamicFormControlModel): Type< }) export class DsDynamicFormControlContainerComponent extends DynamicFormControlContainerComponent implements OnInit, OnChanges, OnDestroy { - /** - * The unique element ID assigned to this component instance. - * For the first occurrence of a base ID, this equals the base ID (preserving backward compatibility). - * For subsequent occurrences, a numeric suffix is appended. - */ - private _uniqueId: string; - - /** - * The base element ID (from getElementId) before deduplication. - */ - private _baseId: string; - - /** - * The instance key used when registering with UniqueIdRegistry. - * Stored so that ngOnDestroy can release with the exact same key, - * even if the model's parent has been reassigned. - */ - private _instanceKey: string; - @ContentChildren(DynamicTemplateDirective) contentTemplateList: QueryList; // eslint-disable-next-line @angular-eslint/no-input-rename @Input('templates') inputTemplateList: QueryList; @@ -277,16 +257,14 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo /** * Returns a unique element ID for this component instance. - * The first occurrence of a base ID keeps the original value. - * Subsequent occurrences receive a numeric suffix. + * When the model has a parent (e.g., it lives inside a form group), + * the parent's ID is appended to distinguish duplicate field IDs + * that appear in different form sections. */ get id(): string { - if (!this._uniqueId) { - this._baseId = this.layoutService.getElementId(this.model); - this._instanceKey = `${this._baseId}_${this.model?.parent?.id || 'root'}`; - this._uniqueId = UniqueIdRegistry.register(this._baseId, this._instanceKey); - } - return this._uniqueId; + const baseId = this.layoutService.getElementId(this.model); + const parentId = this.model?.parent?.id; + return parentId ? `${baseId}_${parentId}` : baseId; } get componentType(): Type | null { @@ -442,13 +420,13 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo // Propagate the container's unique ID to the child form control component // so that the child's rendered element id matches the container's label[for]. // Without this, child components use layoutService.getElementId(model) which - // returns the base ID, while the container may return a suffixed ID from UniqueIdRegistry. + // returns the base ID, while the container appends the parent's ID. // // Object.defineProperty is used because many child components come from the // third-party @ng-dynamic-forms/ui-ng-bootstrap library and cannot be modified // to accept an @Input() override. The instance-level property takes precedence // over the prototype getter in all child components (both third-party and custom). - if (this.componentRef?.instance) { + if (this.componentRef?.instance && this.model?.parent?.id) { const uniqueId = this.id; Object.defineProperty(this.componentRef.instance, 'id', { get: () => uniqueId, @@ -551,12 +529,9 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo } /** - * Unsubscribe from all subscriptions and release the unique ID from the registry. + * Unsubscribe from all subscriptions. */ ngOnDestroy(): void { - if (this._instanceKey) { - UniqueIdRegistry.release(this._instanceKey); - } this.subs .filter((sub) => hasValue(sub)) .forEach((sub) => sub.unsubscribe()); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.spec.ts deleted file mode 100644 index 51d444c6732..00000000000 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.spec.ts +++ /dev/null @@ -1,104 +0,0 @@ -import { UniqueIdRegistry } from './unique-id-registry'; - -describe('UniqueIdRegistry', () => { - - afterEach(() => { - UniqueIdRegistry.clear(); - }); - - describe('register', () => { - it('should return the base ID for the first registration', () => { - const id = UniqueIdRegistry.register('email', 'email_root'); - expect(id).toBe('email'); - }); - - it('should return a suffixed ID for the second registration of the same base ID', () => { - UniqueIdRegistry.register('email', 'email_root'); - const id2 = UniqueIdRegistry.register('email', 'email_group1'); - expect(id2).toBe('email_1'); - }); - - it('should return incrementing suffixes for multiple registrations', () => { - UniqueIdRegistry.register('email', 'email_root'); - const id2 = UniqueIdRegistry.register('email', 'email_group1'); - const id3 = UniqueIdRegistry.register('email', 'email_group2'); - expect(id2).toBe('email_1'); - expect(id3).toBe('email_2'); - }); - - it('should return the cached value for idempotent re-registration', () => { - const id1 = UniqueIdRegistry.register('email', 'email_root'); - const id2 = UniqueIdRegistry.register('email', 'email_root'); - expect(id1).toBe(id2); - expect(id1).toBe('email'); - }); - - it('should handle independent base IDs separately', () => { - const id1 = UniqueIdRegistry.register('email', 'email_root'); - const id2 = UniqueIdRegistry.register('name', 'name_root'); - expect(id1).toBe('email'); - expect(id2).toBe('name'); - }); - }); - - describe('release', () => { - it('should free suffix 0 so a new registration reuses the base ID', () => { - UniqueIdRegistry.register('email', 'email_root'); - UniqueIdRegistry.release('email_root'); - - const id = UniqueIdRegistry.register('email', 'email_new'); - expect(id).toBe('email'); - }); - - it('should free a suffixed entry so a new registration reuses that suffix', () => { - UniqueIdRegistry.register('email', 'email_root'); - UniqueIdRegistry.register('email', 'email_group1'); - UniqueIdRegistry.release('email_group1'); - - const id = UniqueIdRegistry.register('email', 'email_group2'); - expect(id).toBe('email_1'); - }); - - it('should be a no-op for an unknown instance key', () => { - UniqueIdRegistry.register('email', 'email_root'); - UniqueIdRegistry.release('unknown_key'); - - // Original registration still works - const id = UniqueIdRegistry.register('email', 'email_root'); - expect(id).toBe('email'); - }); - - it('should assign lowest available suffix after releasing middle suffix', () => { - UniqueIdRegistry.register('x', 'x_a'); // suffix 0 - UniqueIdRegistry.register('x', 'x_b'); // suffix 1 - UniqueIdRegistry.register('x', 'x_c'); // suffix 2 - - UniqueIdRegistry.release('x_b'); // free suffix 1 - - const id = UniqueIdRegistry.register('x', 'x_d'); - expect(id).toBe('x_1'); - }); - }); - - describe('clear', () => { - it('should reset all state so registrations start fresh', () => { - UniqueIdRegistry.register('email', 'email_root'); - UniqueIdRegistry.register('email', 'email_group1'); - UniqueIdRegistry.clear(); - - const id = UniqueIdRegistry.register('email', 'email_root'); - expect(id).toBe('email'); - }); - }); - - describe('destroy and re-create scenario', () => { - it('should return the base ID when the sole instance is destroyed and recreated', () => { - // Simulates a form field being destroyed (e.g., type change) and recreated - UniqueIdRegistry.register('local_hasCMDI', 'local_hasCMDI_root'); - UniqueIdRegistry.release('local_hasCMDI_root'); - - const id = UniqueIdRegistry.register('local_hasCMDI', 'local_hasCMDI_root'); - expect(id).toBe('local_hasCMDI'); - }); - }); -}); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts deleted file mode 100644 index 38c7be4c37f..00000000000 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry.ts +++ /dev/null @@ -1,85 +0,0 @@ -/** - * Static registry for generating unique HTML element IDs across dynamic form components. - * - * When the same form model ID appears multiple times in the DOM (e.g., scrollable dropdowns - * for mediaType/detailedType in different type-bound form groups), this registry ensures - * each rendered instance receives a unique HTML ID. - * - * - First occurrence: keeps the original base ID (backward compatible). - * - Subsequent occurrences: appends a numeric suffix (`_1`, `_2`, etc.). - * - * Released suffixes are recycled and may be assigned to future registrations. - * Components must call `register()` during initialization and `release()` during destruction. - */ -export class UniqueIdRegistry { - - /** - * Tracks which suffixes are currently in use for each base ID. - * Key = base element ID, Value = set of active suffix numbers. - */ - private static activeSuffixes: Map> = new Map>(); - - /** - * Tracks the assigned base ID and suffix for each component instance. - * Key = a unique instance token (component + model-based), - * Value = { baseId, suffix } assigned to that instance. - */ - private static instances: Map = new Map(); - - /** - * Register a base ID and return a unique ID for this instance. - * The first occurrence returns the base ID unchanged. - * Subsequent occurrences return `baseId_N` where N is the lowest available suffix (1, 2, ...). - * - * @param baseId The base element ID (from getElementId). - * @param instanceKey A unique key identifying this specific component instance. - * @returns The unique element ID to use in the DOM. - */ - static register(baseId: string, instanceKey: string): string { - // If this instance was already registered, return its existing ID - const existing = this.instances.get(instanceKey); - if (existing) { - return existing.suffix === 0 ? baseId : `${baseId}_${existing.suffix}`; - } - - // Find the lowest available suffix for this base ID - const active = this.activeSuffixes.get(baseId) || new Set(); - let suffix = 0; - while (active.has(suffix)) { - suffix++; - } - - active.add(suffix); - this.activeSuffixes.set(baseId, active); - this.instances.set(instanceKey, { baseId, suffix }); - return suffix === 0 ? baseId : `${baseId}_${suffix}`; - } - - /** - * Release the unique ID when a component is destroyed. - * The released suffix becomes available for reuse by future registrations. - * - * @param instanceKey The unique key used during registration. - */ - static release(instanceKey: string): void { - const entry = this.instances.get(instanceKey); - if (entry) { - const active = this.activeSuffixes.get(entry.baseId); - if (active) { - active.delete(entry.suffix); - if (active.size === 0) { - this.activeSuffixes.delete(entry.baseId); - } - } - } - this.instances.delete(instanceKey); - } - - /** - * Clear the entire registry. Used in tests to reset state between specs. - */ - static clear(): void { - this.activeSuffixes.clear(); - this.instances.clear(); - } -} diff --git a/src/test.ts b/src/test.ts index 4928069fc21..2f07cf0d1da 100644 --- a/src/test.ts +++ b/src/test.ts @@ -8,7 +8,6 @@ import { platformBrowserDynamicTesting } from '@angular/platform-browser-dynamic/testing'; import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; -import { UniqueIdRegistry } from './app/shared/form/builder/ds-dynamic-form-ui/unique-id-registry'; // First, initialize the Angular testing environment. getTestBed().initTestEnvironment( @@ -22,6 +21,4 @@ jasmine.getEnv().afterEach(() => { getTestBed().inject(MockStore, null)?.resetSelectors(); // Close any leftover modals getTestBed().inject(NgbModal, null)?.dismissAll?.(); - // Reset unique ID registry to prevent static state leaking between tests - UniqueIdRegistry.clear(); }); From 200e6224ace9584917f61e0b36dc20412aae1867 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Mon, 9 Mar 2026 10:18:18 +0100 Subject: [PATCH 08/10] Updated components to make IT pass --- ...c-form-control-container.component.spec.ts | 75 +++++++----------- ...ynamic-form-control-container.component.ts | 79 ++++++++++++++----- src/test.ts | 5 ++ 3 files changed, 90 insertions(+), 69 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts index 52354cc75c3..50b286bfcc9 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts @@ -378,65 +378,44 @@ describe('DsDynamicFormControlContainerComponent test suite', () => { }); describe('unique id generation', () => { - it('should return the base element ID when the model has no parent', () => { - // testModel (formModel[8]) is a root-level DynamicInputModel with id 'input' - // and has no parent, so id should equal the base element ID. - expect(component.model.parent).toBeFalsy(); - expect(component.id).toBe(component.model.id); + afterEach(() => { + DsDynamicFormControlContainerComponent.resetIdCounters(); }); - it('should append the parent ID when the model has a parent', () => { - const parentGroup = new DynamicFormGroupModel({ - id: 'parentSection', - group: [new DynamicInputModel({ id: 'email' })] - }); - const childModel = parentGroup.group[0]; - // parent is set by DynamicFormService.createFormGroup, not by the constructor - childModel.parent = parentGroup; - - component.model = childModel; - expect(component.id).toBe('email_parentSection'); + it('should return the base element ID for the first instance of a given id', () => { + // testModel (formModel[8]) has id 'input' — first instance keeps original + expect(component.id).toBe(component.model.id); }); - it('should produce different IDs for the same field in different parent groups', () => { - const group1 = new DynamicFormGroupModel({ - id: 'sectionA', - group: [new DynamicInputModel({ id: 'title' })] - }); - const group2 = new DynamicFormGroupModel({ - id: 'sectionB', - group: [new DynamicInputModel({ id: 'title' })] - }); - // parent is set by DynamicFormService.createFormGroup, not by the constructor - group1.group[0].parent = group1; - group2.group[0].parent = group2; - - component.model = group1.group[0]; - const id1 = component.id; - - component.model = group2.group[0]; - const id2 = component.id; - - expect(id1).toBe('title_sectionA'); - expect(id2).toBe('title_sectionB'); - expect(id1).not.toBe(id2); + it('should return a suffixed ID for the second instance of the same base id', () => { + // Simulate two separate container instances with the same base id. + // The first call to the getter registers 'input' → suffix 0 (original). + expect(component.id).toBe('input'); + + // Create a second component-like access: directly exercise the getter + // on a fresh component that shares the same model id. + const secondComponent = Object.create(component); + secondComponent._cachedId = undefined; + secondComponent._baseId = undefined; + // model with the same id but a new instance + secondComponent.model = new DynamicInputModel({ id: 'input' }); + expect(secondComponent.id).toBe('input_1'); }); - it('should return the same id on repeated access (idempotent)', () => { - const parentGroup = new DynamicFormGroupModel({ - id: 'myGroup', - group: [new DynamicInputModel({ id: 'field' })] - }); - const childModel = parentGroup.group[0]; - // parent is set by DynamicFormService.createFormGroup, not by the constructor - childModel.parent = parentGroup; + it('should not interfere between different base ids', () => { + expect(component.id).toBe('input'); // registers 'input' - component.model = childModel; + const otherComponent = Object.create(component); + otherComponent._cachedId = undefined; + otherComponent._baseId = undefined; + otherComponent.model = new DynamicInputModel({ id: 'email' }); + expect(otherComponent.id).toBe('email'); // first 'email' → original + }); + it('should return the same id on repeated access (idempotent)', () => { const first = component.id; const second = component.id; expect(first).toBe(second); - expect(first).toBe('field_myGroup'); }); }); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts index 4466dea93e9..478ef54c8e7 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts @@ -255,16 +255,41 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo */ fetchThumbnail: boolean; + /** + * Tracks per-baseId state for unique ID generation. + * nextSuffix: the next numeric suffix to assign (0 means keep original ID). + * activeCount: number of live component instances using this baseId + * — when it drops to 0 the entry is removed so that a later page visit + * starts fresh. + */ + private static _idState = new Map(); + private _cachedId: string; + private _baseId: string; + /** * Returns a unique element ID for this component instance. - * When the model has a parent (e.g., it lives inside a form group), - * the parent's ID is appended to distinguish duplicate field IDs - * that appear in different form sections. + * The first instance of every base ID keeps the original value; + * subsequent instances get a numeric suffix (_1, _2, …). */ get id(): string { - const baseId = this.layoutService.getElementId(this.model); - const parentId = this.model?.parent?.id; - return parentId ? `${baseId}_${parentId}` : baseId; + if (!this._cachedId) { + this._baseId = this.layoutService.getElementId(this.model); + const state = DsDynamicFormControlContainerComponent._idState.get(this._baseId) + || { nextSuffix: 0, activeCount: 0 }; + this._cachedId = state.nextSuffix === 0 ? this._baseId : `${this._baseId}_${state.nextSuffix}`; + state.nextSuffix++; + state.activeCount++; + DsDynamicFormControlContainerComponent._idState.set(this._baseId, state); + } + return this._cachedId; + } + + /** + * Clears the global ID counter state. Used in tests to prevent + * state leaking between test cases. + */ + static resetIdCounters(): void { + DsDynamicFormControlContainerComponent._idState.clear(); } get componentType(): Type | null { @@ -417,21 +442,23 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo (instance as any).formGroup = this.formGroup; } - // Propagate the container's unique ID to the child form control component - // so that the child's rendered element id matches the container's label[for]. - // Without this, child components use layoutService.getElementId(model) which - // returns the base ID, while the container appends the parent's ID. + // When this container's unique ID differs from the base model ID + // (i.e., this is a duplicate instance), propagate the suffixed ID + // to the child form control component so that the rendered element + // id matches the container's label[for]. // - // Object.defineProperty is used because many child components come from the - // third-party @ng-dynamic-forms/ui-ng-bootstrap library and cannot be modified - // to accept an @Input() override. The instance-level property takes precedence - // over the prototype getter in all child components (both third-party and custom). - if (this.componentRef?.instance && this.model?.parent?.id) { - const uniqueId = this.id; - Object.defineProperty(this.componentRef.instance, 'id', { - get: () => uniqueId, - configurable: true, - }); + // Object.defineProperty is used because child components from the + // third-party @ng-dynamic-forms library expose `id` as a getter + // on the prototype; an instance-level property takes precedence. + if (this.componentRef?.instance) { + const baseId = this.layoutService.getElementId(this.model); + if (this.id !== baseId) { + const uniqueId = this.id; + Object.defineProperty(this.componentRef.instance, 'id', { + get: () => uniqueId, + configurable: true, + }); + } } } } @@ -529,9 +556,19 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo } /** - * Unsubscribe from all subscriptions. + * Unsubscribe from all subscriptions and clean up the ID counter + * for this instance's base ID. */ ngOnDestroy(): void { + if (this._baseId) { + const state = DsDynamicFormControlContainerComponent._idState.get(this._baseId); + if (state) { + state.activeCount--; + if (state.activeCount <= 0) { + DsDynamicFormControlContainerComponent._idState.delete(this._baseId); + } + } + } this.subs .filter((sub) => hasValue(sub)) .forEach((sub) => sub.unsubscribe()); diff --git a/src/test.ts b/src/test.ts index 2f07cf0d1da..c401ca80eb7 100644 --- a/src/test.ts +++ b/src/test.ts @@ -8,6 +8,9 @@ import { platformBrowserDynamicTesting } from '@angular/platform-browser-dynamic/testing'; import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; +import { + DsDynamicFormControlContainerComponent +} from './app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component'; // First, initialize the Angular testing environment. getTestBed().initTestEnvironment( @@ -21,4 +24,6 @@ jasmine.getEnv().afterEach(() => { getTestBed().inject(MockStore, null)?.resetSelectors(); // Close any leftover modals getTestBed().inject(NgbModal, null)?.dismissAll?.(); + // Reset unique-ID counters so state does not leak between test cases + DsDynamicFormControlContainerComponent.resetIdCounters(); }); From 4d0bf5eb826ff49052f87f11573656612693b149 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Mon, 9 Mar 2026 11:02:49 +0100 Subject: [PATCH 09/10] refactor: replace UniqueIdRegistry with inline static counter for unique form IDs --- ...-dynamic-form-control-container.component.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts index 478ef54c8e7..4af38be1077 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts @@ -211,6 +211,15 @@ export function dsDynamicFormControlMapFn(model: DynamicFormControlModel): Type< }) export class DsDynamicFormControlContainerComponent extends DynamicFormControlContainerComponent implements OnInit, OnChanges, OnDestroy { + /** + * Tracks per-baseId state for unique ID generation. + * nextSuffix: the next numeric suffix to assign (0 means keep original ID). + * activeCount: number of live component instances using this baseId + * — when it drops to 0 the entry is removed so that a later page visit + * starts fresh. + */ + private static _idState = new Map(); + @ContentChildren(DynamicTemplateDirective) contentTemplateList: QueryList; // eslint-disable-next-line @angular-eslint/no-input-rename @Input('templates') inputTemplateList: QueryList; @@ -255,14 +264,6 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo */ fetchThumbnail: boolean; - /** - * Tracks per-baseId state for unique ID generation. - * nextSuffix: the next numeric suffix to assign (0 means keep original ID). - * activeCount: number of live component instances using this baseId - * — when it drops to 0 the entry is removed so that a later page visit - * starts fresh. - */ - private static _idState = new Map(); private _cachedId: string; private _baseId: string; From a7a93cab657d8e08ab1cdb03b0eda076afc4c658 Mon Sep 17 00:00:00 2001 From: Matus Kasak Date: Mon, 9 Mar 2026 12:50:35 +0100 Subject: [PATCH 10/10] Add tests to verify _idState cleanup and reuse of base id after ngOnDestroy --- ...c-form-control-container.component.spec.ts | 32 +++++++++++++++++++ ...ynamic-form-control-container.component.ts | 3 +- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts index 50b286bfcc9..bb6f33c290f 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts @@ -417,6 +417,38 @@ describe('DsDynamicFormControlContainerComponent test suite', () => { const second = component.id; expect(first).toBe(second); }); + + it('should remove the id state entry so the next instance reuses the base id', () => { + expect(component.id).toBe('input'); + + // Destroy the only active instance — state entry should be deleted + component.ngOnDestroy(); + // A new component with the same base id should receive the original base id (no suffix) + const newComponent = Object.create(component); + newComponent._cachedId = undefined; + newComponent._baseId = undefined; + newComponent.model = new DynamicInputModel({ id: 'input' }); + expect(newComponent.id).toBe('input'); + }); + + it('should keep the id state entry when other instances with the same base id are still active', () => { + // Register two instances with the same base id + expect(component.id).toBe('input'); + + const secondComponent = Object.create(component); + secondComponent._cachedId = undefined; + secondComponent._baseId = undefined; + secondComponent.model = new DynamicInputModel({ id: 'input' }); + expect(secondComponent.id).toBe('input_1'); + + component.ngOnDestroy();// destroy only the first instance + // A third component with the same base id should still get a suffixed id + const thirdComponent = Object.create(component); + thirdComponent._cachedId = undefined; + thirdComponent._baseId = undefined; + thirdComponent.model = new DynamicInputModel({ id: 'input' }); + expect(thirdComponent.id).toBe('input_2'); + }); }); }); diff --git a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts index 4af38be1077..15ca2912e0b 100644 --- a/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts +++ b/src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts @@ -452,8 +452,7 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo // third-party @ng-dynamic-forms library expose `id` as a getter // on the prototype; an instance-level property takes precedence. if (this.componentRef?.instance) { - const baseId = this.layoutService.getElementId(this.model); - if (this.id !== baseId) { + if (this.id !== this._baseId) { const uniqueId = this.id; Object.defineProperty(this.componentRef.instance, 'id', { get: () => uniqueId,