UFAL/Resolve duplicate html IDs in submission form (#1234)

* Resolve duplicate HTML IDs in submission form.

* refactor for unique ID registry

* fix: remove unused baseId param from release in UniqueIdRegistry

* fix: propagate unique ID from container to child form controls for accessibility

* fix: recycle released suffixes in UniqueIdRegistry to preserve stable IDs across re-renders

* refactor: deduplicate UniqueIdRegistry usage, store instanceKey, and add unit tests

* Refactored the code to make this fix more simple

* Updated components to make IT pass

* refactor: replace UniqueIdRegistry with inline static counter for unique form IDs

* Add tests to verify _idState cleanup and reuse of base id after ngOnDestroy

---------

Co-authored-by: Matus Kasak <matus.kasak@dataquest.sk>
Co-authored-by: milanmajchrak <milan.majchrak@dataquest.sk>

Author: 3 people

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')]">
   <label *ngIf="!isCheckbox && hasLabel"
-         [id]="'label_' + model.id"
+         [id]="'label_' + id"
          [for]="id"
          [innerHTML]="(model.required && model.label) ? (model.label | translate) + ' *' : (model.label | translate)"
          [ngClass]="[getClass('element', 'label'), getClass('grid', 'label')]"></label>

src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.spec.ts

@@ -377,4 +377,78 @@ describe('DsDynamicFormControlContainerComponent test suite', () => {
     expect(testFn(formModel[25])).toEqual(DsDynamicFormGroupComponent);
   });
 
+  describe('unique id generation', () => {
+    afterEach(() => {
+      DsDynamicFormControlContainerComponent.resetIdCounters();
+    });
+
+    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 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 not interfere between different base ids', () => {
+      expect(component.id).toBe('input'); // registers 'input'
+
+      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);
+    });
+
+    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');
+    });
+  });
+
 });

src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.ts

@@ -210,6 +210,16 @@ export function dsDynamicFormControlMapFn(model: DynamicFormControlModel): Type<
   changeDetection: ChangeDetectionStrategy.Default
 })
 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<string, { nextSuffix: number; activeCount: number }>();
+
   @ContentChildren(DynamicTemplateDirective) contentTemplateList: QueryList<DynamicTemplateDirective>;
   // eslint-disable-next-line @angular-eslint/no-input-rename
   @Input('templates') inputTemplateList: QueryList<DynamicTemplateDirective>;
@@ -254,6 +264,35 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo
    */
   fetchThumbnail: boolean;
 
+  private _cachedId: string;
+  private _baseId: string;
+
+  /**
+   * Returns a unique element ID for this component instance.
+   * The first instance of every base ID keeps the original value;
+   * subsequent instances get a numeric suffix (_1, _2, …).
+   */
+  get id(): string {
+    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<DynamicFormControl> | null {
     return dsDynamicFormControlMapFn(this.model);
   }
@@ -403,6 +442,24 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo
         (instance as any).formModel = this.formModel;
         (instance as any).formGroup = this.formGroup;
       }
+
+      // 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 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) {
+        if (this.id !== this._baseId) {
+          const uniqueId = this.id;
+          Object.defineProperty(this.componentRef.instance, 'id', {
+            get: () => uniqueId,
+            configurable: true,
+          });
+        }
+      }
     }
   }
 
@@ -499,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());

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">
                 <span [class]="'label label-default label-' + license.licenseLabel">{{license.licenseLabel}}</span>
                 <b class="pl-1">{{license.name}}</b>
               </li>

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();
 });