Skip to content

Commit 3ca2011

Browse files
milanmajchrakclaude
andcommitted
fix(submission): keep keyboard reorder-cancel and clear-entry consistent
Both from the Copilot review of this PR; both are regressions this PR introduced. cancelKeyboardDragAndDrop() still moved the group models only, so Escape after a keyboard reorder restored the model order while leaving the FormArray as it was - the exact desync this PR set out to remove. It now goes through moveGroupAndControl() like the drag and arrow-key paths. Dropping the clear entry's (click) handler to stop it firing twice per mouse interaction also removed its keyboard activation, since a native button turns Enter into a click. It now carries (keydown.enter) alongside (mousedown), which mirrors how the options are bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a123f12 commit 3ca2011

4 files changed

Lines changed: 47 additions & 2 deletions

File tree

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
ReactiveFormsModule,
2020
UntypedFormArray,
2121
} from '@angular/forms';
22+
import { By } from '@angular/platform-browser';
2223
import {
2324
DYNAMIC_FORM_CONTROL_MAP_FN,
2425
DynamicFormLayoutService,
@@ -173,6 +174,37 @@ describe('DsDynamicFormArrayComponent row/control binding', () => {
173174
expect(boundValue(1)).withContext('Dataset moved up into position 1').toBe('Dataset');
174175
});
175176

177+
it('keeps rows and controls in sync when a keyboard reorder is cancelled', () => {
178+
const dropList = fixture.debugElement.query(By.css('.cdk-drop-list')).nativeElement;
179+
const rowEl = dropList.querySelectorAll('[cdkDrag]')[0] as HTMLDivElement;
180+
181+
// Pick row 0 up, move it down twice, then abandon the reorder with Escape.
182+
component.toggleKeyboardDragAndDrop(new KeyboardEvent('keydown', { key: ' ' }), rowEl, 0, 3);
183+
component.handleArrowPress(new KeyboardEvent('keydown', { key: 'ArrowDown' }), dropList, 3, 0, 'down');
184+
component.handleArrowPress(new KeyboardEvent('keydown', { key: 'ArrowDown' }), dropList, 3, 1, 'down');
185+
fixture.detectChanges();
186+
187+
component.cancelKeyboardDragAndDrop(rowEl, 2, 3);
188+
fixture.detectChanges();
189+
190+
expect([boundValue(0), boundValue(1), boundValue(2)])
191+
.withContext('cancelling must restore the original order for models AND controls')
192+
.toEqual(['Article', 'Book', 'Dataset']);
193+
});
194+
195+
it('keeps rows and controls in sync through a completed keyboard reorder', () => {
196+
const dropList = fixture.debugElement.query(By.css('.cdk-drop-list')).nativeElement;
197+
const rowEl = dropList.querySelectorAll('[cdkDrag]')[0] as HTMLDivElement;
198+
199+
component.toggleKeyboardDragAndDrop(new KeyboardEvent('keydown', { key: ' ' }), rowEl, 0, 3);
200+
component.handleArrowPress(new KeyboardEvent('keydown', { key: 'ArrowDown' }), dropList, 3, 0, 'down');
201+
fixture.detectChanges();
202+
203+
expect([boundValue(0), boundValue(1), boundValue(2)])
204+
.withContext('Article moved down one place, controls followed')
205+
.toEqual(['Book', 'Article', 'Dataset']);
206+
});
207+
176208
it('rebinds after a row is inserted in the middle', () => {
177209
forms.insertFormArrayGroup(1, formArray(), component.model);
178210
fixture.detectChanges();

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ export class DsDynamicFormArrayComponent extends DynamicFormArrayComponent {
252252
}
253253

254254
cancelKeyboardDragAndDrop(sortableElement: HTMLDivElement, index: number, length: number) {
255-
this.model.moveGroup(index, this.elementBeingSortedStartingIndex - index);
255+
this.moveGroupAndControl(index, this.elementBeingSortedStartingIndex);
256256
if (hasValue(this.model.groups[this.elementBeingSortedStartingIndex]) && hasValue((this.control as any).controls[this.elementBeingSortedStartingIndex])) {
257257
this.onCustomEvent({
258258
previousIndex: index,

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,10 @@
5959
<!-- Clearing is destructive and must sit at the END of the menu. As the first entry it was
6060
rendered directly beneath the input, exactly where a click meant to close the menu lands,
6161
so dismissing the dropdown wiped the value instead.
62-
A single handler: carrying both (click) and (mousedown) fired it twice per interaction. -->
62+
(mousedown) + (keydown.enter) mirrors the options above: carrying (click) as well fired
63+
it twice per mouse interaction, but dropping it outright left clearing mouse-only. -->
6364
<button class="dropdown-item collection-item text-truncate scrollable-dropdown-clear"
65+
(keydown.enter)="onSelect(undefined); sdRef.close()"
6466
(mousedown)="onSelect(undefined); sdRef.close()"
6567
title="{{ 'dropdown.clear.tooltip' | translate }}" role="option"
6668
type="button">

src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.duplicate.spec.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,17 @@ describe('DsDynamicScrollableDropdownComponent duplicate/clearing regressions',
291291
.withContext('the first entry of the menu must not be the clear action').toBeFalsy();
292292
});
293293

294+
it('clears from the keyboard as well as the mouse', () => {
295+
// The options are reachable with Enter; dropping the clear entry's (click) handler in favour
296+
// of (mousedown) must not leave it mouse-only.
297+
const clear = fixture.debugElement.query(By.css('button.dropdown-item.scrollable-dropdown-clear'));
298+
spyOn(comp, 'onSelect');
299+
300+
clear.nativeElement.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true }));
301+
302+
expect(comp.onSelect).toHaveBeenCalledTimes(1);
303+
});
304+
294305
it('keeps the clear entry available (further down the menu)', () => {
295306
const clear = fixture.debugElement.query(By.css('button.dropdown-item.scrollable-dropdown-clear'));
296307
expect(clear).withContext('clearing must still be possible').not.toBeNull();

0 commit comments

Comments
 (0)