UoE: fix duplicate + cleared values in repeatable submission dropdowns

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

@@ -306,7 +306,10 @@ export class DsDynamicFormControlContainerComponent extends DynamicFormControlCo
   }
 
   ngOnChanges(changes: SimpleChanges) {
-    if (changes && !this.isRelationship && hasValue(this.group.get(this.model.id))) {
+    // `group` can legitimately be null for one tick while a repeatable field is being re-rendered
+    // (a row was added or removed). Dereferencing it there threw inside change detection and
+    // aborted the pass for the whole field, so the remaining rows stopped updating.
+    if (changes && !this.isRelationship && hasValue(this.group) && hasValue(this.group.get(this.model.id))) {
       super.ngOnChanges(changes);
       if (this.model && this.model.placeholder) {
         this.model.placeholder = this.translateService.instant(this.model.placeholder);

src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.row-binding.spec.ts

@@ -0,0 +1,217 @@
+// Regression specs for the row -> FormControl binding of a repeatable field.
+//
+// `getControlOfGroup()` used to stamp a `startingIndex` on the group model the first time a row
+// rendered and then resolve that row's FormGroup as `control.get([startingIndex])` forever.
+// Nothing re-synced it when the FormArray was mutated, so after removing a non-last row the
+// surviving rows bound to the wrong FormGroup (or to null), and after remove+insert two rows
+// could alias onto the same FormGroup.
+//
+// That is what let a value the user never touched be overwritten, and what made the duplicate
+// detection in DsDynamicScrollableDropdownComponent read stale sibling values.
+import { HttpClient } from '@angular/common/http';
+import { EventEmitter } from '@angular/core';
+import {
+  ComponentFixture,
+  inject,
+  TestBed,
+} from '@angular/core/testing';
+import {
+  ReactiveFormsModule,
+  UntypedFormArray,
+} from '@angular/forms';
+import { By } from '@angular/platform-browser';
+import {
+  DYNAMIC_FORM_CONTROL_MAP_FN,
+  DynamicFormLayoutService,
+  DynamicFormService,
+  DynamicFormValidationService,
+  DynamicInputModel,
+} from '@ng-dynamic-forms/core';
+import { provideMockStore } from '@ngrx/store/testing';
+import {
+  TranslateModule,
+  TranslateService,
+} from '@ngx-translate/core';
+import { NgxMaskModule } from 'ngx-mask';
+import { of } from 'rxjs';
+
+import {
+  APP_CONFIG,
+  APP_DATA_SERVICES_MAP,
+} from '../../../../../../../config/app-config.interface';
+import { environment } from '../../../../../../../environments/environment.test';
+import { SubmissionService } from '../../../../../../submission/submission.service';
+import { LiveRegionService } from '../../../../../live-region/live-region.service';
+import { getLiveRegionServiceStub } from '../../../../../live-region/live-region.service.stub';
+import { DsDynamicFormControlContainerComponent } from '../../ds-dynamic-form-control-container.component';
+import { dsDynamicFormControlMapFn } from '../../ds-dynamic-form-control-map-fn';
+import { DynamicRowArrayModel } from '../ds-dynamic-row-array-model';
+import { DsDynamicFormArrayComponent } from './dynamic-form-array.component';
+
+describe('DsDynamicFormArrayComponent row/control binding', () => {
+  const translateServiceStub = {
+    get: () => of('translated-text'),
+    instant: () => 'translated-text',
+    onLangChange: new EventEmitter(),
+    onTranslationChange: new EventEmitter(),
+    onDefaultLangChange: new EventEmitter(),
+  };
+
+  let component: DsDynamicFormArrayComponent;
+  let fixture: ComponentFixture<DsDynamicFormArrayComponent>;
+  // FormBuilderService only inherits these from DynamicFormService; using the base service keeps
+  // the fixture free of the whole submission dependency graph.
+  let forms: DynamicFormService;
+
+  /** The FormArray backing the rows of the repeatable field. */
+  const formArray = (): UntypedFormArray => component.group.get(component.model.id) as UntypedFormArray;
+
+  /** Value currently held by the control that row `i` is bound to. */
+  const boundValue = (i: number): any =>
+    (component.getControlOfGroup(component.model.groups[i]) as any)?.get('rowInput')?.value;
+
+  beforeEach(async () => {
+    await TestBed.configureTestingModule({
+      imports: [
+        ReactiveFormsModule,
+        DsDynamicFormArrayComponent,
+        NgxMaskModule.forRoot(),
+        TranslateModule.forRoot(),
+      ],
+      providers: [
+        DynamicFormLayoutService,
+        DynamicFormValidationService,
+        provideMockStore(),
+        { provide: APP_DATA_SERVICES_MAP, useValue: {} },
+        { provide: TranslateService, useValue: translateServiceStub },
+        { provide: HttpClient, useValue: {} },
+        { provide: SubmissionService, useValue: {} },
+        { provide: APP_CONFIG, useValue: environment },
+        { provide: DYNAMIC_FORM_CONTROL_MAP_FN, useValue: dsDynamicFormControlMapFn },
+        { provide: LiveRegionService, useValue: getLiveRegionServiceStub() },
+      ],
+    }).overrideComponent(DsDynamicFormArrayComponent, {
+      remove: { imports: [DsDynamicFormControlContainerComponent] },
+    }).compileComponents();
+  });
+
+  beforeEach(inject([DynamicFormService],
+    (service: DynamicFormService) => {
+      forms = service;
+
+      const formModel = [
+        new DynamicRowArrayModel({
+          id: 'typeArray',
+          initialCount: 3,
+          notRepeatable: false,
+          relationshipConfig: undefined,
+          submissionId: '1234',
+          isDraggable: true,
+          groupFactory: () => [new DynamicInputModel({ id: 'rowInput' })],
+          required: false,
+          metadataKey: 'dc.type',
+          metadataFields: ['dc.type'],
+          hasSelectableMetadata: true,
+          showButtons: true,
+        }),
+      ];
+
+      fixture = TestBed.createComponent(DsDynamicFormArrayComponent);
+      component = fixture.componentInstance;
+      component.model = formModel[0] as DynamicRowArrayModel;
+      component.group = service.createFormGroup(formModel);
+      fixture.detectChanges();
+
+      // Give each row a distinct value, as if the user had picked three Types.
+      ['Article', 'Book', 'Dataset'].forEach((value, i) => {
+        formArray().at(i).get('rowInput').setValue(value);
+        (component.model.groups[i].group[0] as any).value = value;
+      });
+
+      // Render once so every row gets its binding resolved (this is what used to freeze it).
+      component.model.groups.forEach((g) => component.getControlOfGroup(g));
+    }));
+
+  it('binds every row to its own control before any mutation', () => {
+    expect([boundValue(0), boundValue(1), boundValue(2)]).toEqual(['Article', 'Book', 'Dataset']);
+  });
+
+  it('rebinds the surviving rows after the MIDDLE row is removed', () => {
+    forms.removeFormArrayGroup(1, formArray(), component.model);
+    fixture.detectChanges();
+
+    expect(component.model.groups.length).withContext('one row removed').toBe(2);
+    expect(boundValue(0)).withContext('row 0 keeps Article').toBe('Article');
+    expect(boundValue(1)).withContext('row 1 must now resolve to Dataset, not to Book or null').toBe('Dataset');
+  });
+
+  it('never resolves two rows to the same control after remove + insert', () => {
+    forms.removeFormArrayGroup(1, formArray(), component.model);
+    forms.insertFormArrayGroup(component.model.groups.length, formArray(), component.model);
+    fixture.detectChanges();
+
+    const controls = component.model.groups.map((g) => component.getControlOfGroup(g));
+    expect(controls.length).toBe(3);
+    controls.forEach((c, i) => expect(c).withContext(`row ${i} must be bound to a control`).not.toBeNull());
+    expect(new Set(controls).size).withContext('each row must own a distinct FormGroup').toBe(controls.length);
+  });
+
+  it('keeps the surviving row\'s value intact after remove + insert', () => {
+    forms.removeFormArrayGroup(1, formArray(), component.model);
+    forms.insertFormArrayGroup(component.model.groups.length, formArray(), component.model);
+    fixture.detectChanges();
+
+    expect(boundValue(0)).withContext('Article untouched').toBe('Article');
+    expect(boundValue(1)).withContext('Dataset must survive the delete+add').toBe('Dataset');
+    expect(boundValue(2)).withContext('the freshly added row is empty').toBeFalsy();
+  });
+
+  it('rebinds after the FIRST row is removed', () => {
+    forms.removeFormArrayGroup(0, formArray(), component.model);
+    fixture.detectChanges();
+
+    expect(boundValue(0)).withContext('Book moved up into position 0').toBe('Book');
+    expect(boundValue(1)).withContext('Dataset moved up into position 1').toBe('Dataset');
+  });
+
+  it('keeps rows and controls in sync when a keyboard reorder is cancelled', () => {
+    const dropList = fixture.debugElement.query(By.css('.cdk-drop-list')).nativeElement;
+    const rowEl = dropList.querySelectorAll('[cdkDrag]')[0] as HTMLDivElement;
+
+    // Pick row 0 up, move it down twice, then abandon the reorder with Escape.
+    component.toggleKeyboardDragAndDrop(new KeyboardEvent('keydown', { key: ' ' }), rowEl, 0, 3);
+    component.handleArrowPress(new KeyboardEvent('keydown', { key: 'ArrowDown' }), dropList, 3, 0, 'down');
+    component.handleArrowPress(new KeyboardEvent('keydown', { key: 'ArrowDown' }), dropList, 3, 1, 'down');
+    fixture.detectChanges();
+
+    component.cancelKeyboardDragAndDrop(rowEl, 2, 3);
+    fixture.detectChanges();
+
+    expect([boundValue(0), boundValue(1), boundValue(2)])
+      .withContext('cancelling must restore the original order for models AND controls')
+      .toEqual(['Article', 'Book', 'Dataset']);
+  });
+
+  it('keeps rows and controls in sync through a completed keyboard reorder', () => {
+    const dropList = fixture.debugElement.query(By.css('.cdk-drop-list')).nativeElement;
+    const rowEl = dropList.querySelectorAll('[cdkDrag]')[0] as HTMLDivElement;
+
+    component.toggleKeyboardDragAndDrop(new KeyboardEvent('keydown', { key: ' ' }), rowEl, 0, 3);
+    component.handleArrowPress(new KeyboardEvent('keydown', { key: 'ArrowDown' }), dropList, 3, 0, 'down');
+    fixture.detectChanges();
+
+    expect([boundValue(0), boundValue(1), boundValue(2)])
+      .withContext('Article moved down one place, controls followed')
+      .toEqual(['Book', 'Article', 'Dataset']);
+  });
+
+  it('rebinds after a row is inserted in the middle', () => {
+    forms.insertFormArrayGroup(1, formArray(), component.model);
+    fixture.detectChanges();
+
+    expect(boundValue(0)).toBe('Article');
+    expect(boundValue(1)).withContext('the inserted row is empty').toBeFalsy();
+    expect(boundValue(2)).withContext('Book shifted down').toBe('Book');
+    expect(boundValue(3)).withContext('Dataset shifted down').toBe('Dataset');
+  });
+});

src/app/shared/form/builder/ds-dynamic-form-ui/models/array-group/dynamic-form-array.component.ts

@@ -19,10 +19,12 @@ import {
 } from '@angular/core';
 import {
   ReactiveFormsModule,
+  UntypedFormArray,
   UntypedFormGroup,
 } from '@angular/forms';
 import {
   DynamicFormArrayComponent,
+  DynamicFormArrayGroupModel,
   DynamicFormControlCustomEvent,
   DynamicFormControlEvent,
   DynamicFormControlLayout,
@@ -96,7 +98,7 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent {
       return;
     }
 
-    this.model.moveGroup(event.previousIndex, event.currentIndex - event.previousIndex);
+    this.moveGroupAndControl(event.previousIndex, event.currentIndex);
     const prevIndex = event.previousIndex;
     const index = event.currentIndex;
 
@@ -127,16 +129,38 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent {
   }
 
   /**
-   * Gets the control of the specified group model. It adds the startingIndex property to the group model if it does not
-   * already have it. This ensures that the controls are always linked to the correct group model.
+   * Moves a row, keeping the group models and the form controls in the same order.
+   *
+   * Both are addressed by the same live index (see {@link getControlOfGroup}), so reordering only
+   * the models would leave every row from the drop position onwards pointing at another row's
+   * control — which is how values ended up moving between rows.
+   *
+   * @param from The index the row is moved from.
+   * @param to The index the row is moved to.
+   */
+  protected moveGroupAndControl(from: number, to: number): void {
+    const controls = this.control as unknown as UntypedFormArray;
+    if (hasValue(controls?.at(from))) {
+      const moved = controls.at(from);
+      controls.removeAt(from, { emitEvent: false });
+      controls.insert(to, moved, { emitEvent: false });
+    }
+    this.model.moveGroup(from, to - from);
+  }
+
+  /**
+   * Gets the control of the specified group model.
+   *
+   * The group's *live* index is the only authority: `DynamicFormArrayModel` re-indexes its groups
+   * on every insert/remove/move, and the template binds `formGroupName` to that same index. Caching
+   * the index of the first render instead would leave a row pointing at another row's control (or at
+   * none) as soon as a row is added or removed, which silently moves values between rows.
+   *
    * @param groupModel The group model to get the control for.
    * @returns The form control of the specified group model.
    */
-  getControlOfGroup(groupModel: any) {
-    if (!groupModel.hasOwnProperty('startingIndex')) {
-      groupModel.startingIndex = groupModel.index;
-    }
-    return this.control.get([groupModel.startingIndex]);
+  getControlOfGroup(groupModel: DynamicFormArrayGroupModel) {
+    return this.control.get([groupModel.index]);
   }
 
   /**
@@ -200,7 +224,7 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent {
     }
 
     if (this.elementBeingSorted) {
-      this.model.moveGroup(idx, newIndex - idx);
+      this.moveGroupAndControl(idx, newIndex);
       if (hasValue(this.model.groups[newIndex]) && hasValue((this.control as any).controls[newIndex])) {
         this.onCustomEvent({
           previousIndex: idx,
@@ -228,7 +252,7 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent {
   }
 
   cancelKeyboardDragAndDrop(sortableElement: HTMLDivElement, index: number, length: number) {
-    this.model.moveGroup(index, this.elementBeingSortedStartingIndex - index);
+    this.moveGroupAndControl(index, this.elementBeingSortedStartingIndex);
     if (hasValue(this.model.groups[this.elementBeingSortedStartingIndex]) && hasValue((this.control as any).controls[this.elementBeingSortedStartingIndex])) {
       this.onCustomEvent({
         previousIndex: index,

src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html

@@ -1,4 +1,5 @@
-<div #sdRef="ngbDropdown" ngbDropdown display="dynamic" placement="bottom-right" class="w-100">
+<div #sdRef="ngbDropdown" ngbDropdown display="dynamic" placement="bottom-right" class="w-100"
+     (openChange)="onOpenChange($event)">
   <div class="position-relative right-addon"
        role="combobox"
        [attr.aria-label]="model.label"
@@ -41,12 +42,10 @@
          [scrollWindow]="false">
 
       <button class="dropdown-item disabled" type="button" *ngIf="optionsList && optionsList.length === 0">{{'form.no-results' | translate}}</button>
-      <button class="dropdown-item collection-item text-truncate"
-              (click)="onSelect(undefined); sdRef.close()" (mousedown)="onSelect(undefined); sdRef.close()"
-              title="{{ 'dropdown.clear.tooltip' | translate }}" role="option"
-              type="button">
-        <i>{{ 'dropdown.clear' | translate }}</i>
-      </button>
+      <!-- Options commit on (mousedown) rather than (click) by design: blurring the input flips
+           showErrorMessages on a required field, and DsDynamicFormControlContainerComponent then
+           destroys and re-creates this control (forceShowErrorDetection), so the option element is
+           already gone by the time a click event could be produced. -->
       <button class="dropdown-item collection-item text-truncate" *ngFor="let listEntry of optionsList; let i = index"
               [class.active]="i === selectedIndex"
               [attr.aria-selected]="inputFormatter(listEntry) === (currentValue | async)"
@@ -57,6 +56,18 @@
         {{inputFormatter(listEntry)}}
       </button>
       <div class="scrollable-dropdown-loading text-center" *ngIf="loading"><p>{{'form.loading' | translate}}</p></div>
+      <!-- Clearing is destructive and must sit at the END of the menu. As the first entry it was
+           rendered directly beneath the input, exactly where a click meant to close the menu lands,
+           so dismissing the dropdown wiped the value instead.
+           (mousedown) + (keydown.enter) mirrors the options above: carrying (click) as well fired
+           it twice per mouse interaction, but dropping it outright left clearing mouse-only. -->
+      <button class="dropdown-item collection-item text-truncate scrollable-dropdown-clear"
+              (keydown.enter)="onSelect(undefined); sdRef.close()"
+              (mousedown)="onSelect(undefined); sdRef.close()"
+              title="{{ 'dropdown.clear.tooltip' | translate }}" role="option"
+              type="button">
+        <i>{{ 'dropdown.clear' | translate }}</i>
+      </button>
     </div>
 
   </div>

src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.scss

@@ -30,6 +30,16 @@
   left: 0 !important;
   margin-bottom: var(--bs-spacer);
   z-index: 1000;
+  // The menu is positioned flush against the input and covers the rows underneath it. Keeping a
+  // small gap leaves a strip right below the field where a click closes the menu instead of
+  // activating whichever entry happens to be drawn there.
+  margin-top: 0.35rem;
+}
+
+// Clearing is separated from the options so it cannot be hit by accident.
+.collection-item.scrollable-dropdown-clear {
+  border-bottom: 0;
+  border-top: calc(var(--bs-dropdown-border-width) * 2) solid var(--bs-dropdown-border-color);
 }
 
 .scrollable-dropdown-toggle {