Skip to content

Commit 6a19260

Browse files
milanmajchrakclaude
andcommitted
Port #1221 to dtq-dev-9-base: UFAL/Resolve duplicate HTML element IDs across pages
Card X-02a, part A of three. The v9 upgrade took these three templates wholesale from vanilla 9.3, so the fork's hunks from 6628aaf were never applied and the pages emit duplicate DOM ids again. ds-select renders one dropdown per sort option on a browse toolbar and one per pool task on /mydspace, but hardcoded its three ids (dsSelectMenuLabel, dsSelectMenuButton, dsSelectDropdownMenu). Every instance after the first therefore duplicated them, and each instance's aria-describedby / aria-labelledby resolved to the *first* instance's elements - a screen reader announced the wrong label for every dropdown but one. A module-level counter now gives each instance a uniqueId and the three ids are suffixed with it. Two adaptations beyond the source hunk, both because 9-base is not 7.x here: * aria-describedby is bound as [attr.aria-describedby]="label ? 'dsSelectMenuLabel-' + uniqueId : null" rather than left hardcoded. The label span is inside @if (label), so without the null branch the button points at an element that does not exist whenever no label is set - a dangling aria reference. The source commit made the same choice; it is restated here because 9-base uses @if where 7.x used *ngIf. * the pool-task row id is id="actions-{{ dso?.id }}" as in the source. Tests added (the source commit ships none): a host rendering two ds-select instances asserts that the document has no duplicate ids, that each button's aria-describedby resolves to a label inside its own instance and to nothing else, and that the two button ids differ; a third case asserts aria-describedby is null when no label is set; and the pool-task spec asserts the actions id is row-specific rather than the constant "actions". Each was proven load-bearing with a negative control - see the PR description. Source: 6628aaf (dtq-dev PR #1221) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 8ac588e commit 6a19260

5 files changed

Lines changed: 94 additions & 7 deletions

File tree

src/app/shared/ds-select/ds-select.component.html

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,13 @@
33
<div ngbDropdown class="btn-group" (openChange)="toggled.emit($event)">
44

55
@if (label) {
6-
<span id="dsSelectMenuLabel" class="input-group-text">
6+
<span id="dsSelectMenuLabel-{{ uniqueId }}" class="input-group-text">
77
{{ label | translate }}
88
</span>
99
}
1010

11-
<button aria-describedby="dsSelectMenuLabel"
12-
id="dsSelectMenuButton"
11+
<button [attr.aria-describedby]="label ? 'dsSelectMenuLabel-' + uniqueId : null"
12+
id="dsSelectMenuButton-{{ uniqueId }}"
1313
class="btn btn-outline-primary selection"
1414
(blur)="close.emit($event)"
1515
(click)="close.emit($event)"
@@ -20,8 +20,8 @@
2020

2121
<div ngbDropdownMenu
2222
class="dropdown-menu"
23-
id="dsSelectDropdownMenu"
24-
aria-labelledby="dsSelectMenuButton">
23+
id="dsSelectDropdownMenu-{{ uniqueId }}"
24+
[attr.aria-labelledby]="'dsSelectMenuButton-' + uniqueId">
2525
<div>
2626
<ng-content select=".menu"></ng-content>
2727
</div>

src/app/shared/ds-select/ds-select.component.spec.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { Component } from '@angular/core';
12
import {
23
ComponentFixture,
34
TestBed,
@@ -7,6 +8,23 @@ import { TranslateModule } from '@ngx-translate/core';
78

89
import { DsSelectComponent } from './ds-select.component';
910

11+
/**
12+
* Two ds-select instances on one page - the situation that produced duplicate DOM ids
13+
* (a browse toolbar renders one per sort option, MyDSpace one per pool task).
14+
*/
15+
@Component({
16+
selector: 'ds-test-host',
17+
template: `
18+
<ds-select label="first.label"><span class="selection">A</span></ds-select>
19+
<ds-select label="second.label"><span class="selection">B</span></ds-select>
20+
`,
21+
imports: [
22+
DsSelectComponent,
23+
],
24+
})
25+
class TestHostComponent {
26+
}
27+
1028
describe('DsSelectComponent', () => {
1129
let component: DsSelectComponent;
1230
let fixture: ComponentFixture<DsSelectComponent>;
@@ -30,4 +48,55 @@ describe('DsSelectComponent', () => {
3048
it('should create', () => {
3149
expect(component).toBeTruthy();
3250
});
51+
52+
it('should not reference a label element when no label is set', () => {
53+
const button: HTMLElement = fixture.nativeElement.querySelector('button.selection');
54+
55+
expect(button).toBeTruthy();
56+
expect(button.getAttribute('aria-describedby')).toBeNull();
57+
// the button still names its own menu
58+
const menu: HTMLElement = fixture.nativeElement.querySelector('[ngbDropdownMenu]');
59+
expect(menu.getAttribute('aria-labelledby')).toEqual(button.id);
60+
});
61+
62+
describe('with two instances on the same page', () => {
63+
let hostFixture: ComponentFixture<TestHostComponent>;
64+
let hostElement: HTMLElement;
65+
66+
beforeEach(() => {
67+
hostFixture = TestBed.createComponent(TestHostComponent);
68+
hostFixture.detectChanges();
69+
hostElement = hostFixture.nativeElement;
70+
});
71+
72+
it('should not emit duplicate DOM ids', () => {
73+
const ids: string[] = Array.from(hostElement.querySelectorAll('[id]')).map((element: Element) => element.id);
74+
75+
expect(ids.length).toBeGreaterThan(0);
76+
expect(ids.length).toEqual(new Set(ids).size);
77+
});
78+
79+
it('should resolve every aria reference inside its own instance', () => {
80+
const selects: HTMLElement[] = Array.from(hostElement.querySelectorAll('ds-select'));
81+
expect(selects.length).toEqual(2);
82+
83+
const buttonIds: string[] = [];
84+
selects.forEach((select: HTMLElement) => {
85+
const button: HTMLElement = select.querySelector('button.selection');
86+
const menu: HTMLElement = select.querySelector('[ngbDropdownMenu]');
87+
const describedBy: string = button.getAttribute('aria-describedby');
88+
89+
expect(describedBy).toBeTruthy();
90+
// the label the button points at is this instance's own label ...
91+
expect(select.querySelector('[id="' + describedBy + '"]')).toBeTruthy();
92+
// ... and no other element in the document answers to that id
93+
expect(hostElement.querySelectorAll('[id="' + describedBy + '"]').length).toEqual(1);
94+
expect(menu.getAttribute('aria-labelledby')).toEqual(button.id);
95+
96+
buttonIds.push(button.id);
97+
});
98+
99+
expect(buttonIds[0]).not.toEqual(buttonIds[1]);
100+
});
101+
});
33102
});

src/app/shared/ds-select/ds-select.component.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import { TranslateModule } from '@ngx-translate/core';
1010

1111
import { BtnDisabledDirective } from '../btn-disabled.directive';
1212

13+
let nextDsSelectId = 0;
14+
1315
/**
1416
* Component which represent a DSpace dropdown selector.
1517
*/
@@ -25,6 +27,13 @@ import { BtnDisabledDirective } from '../btn-disabled.directive';
2527
})
2628
export class DsSelectComponent {
2729

30+
/**
31+
* Unique identifier for the component instance. Several ds-select instances are rendered on the
32+
* same page (browse toolbars, MyDSpace), so the dropdown's DOM ids have to be per-instance or the
33+
* document carries duplicate ids and every aria reference resolves to the first instance.
34+
*/
35+
uniqueId = `ds-select-${nextDsSelectId++}`;
36+
2837
/**
2938
* An optional label for the dropdown selector.
3039
*/

src/app/shared/object-list/my-dspace-result-list-element/pool-search-result/pool-search-result-list-element.component.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
</ng-container>
2020
<div class="row">
2121
<div [ngClass]="showThumbnails ? 'offset-3 offset-md-2 col-9 col-md-10 ps-3' : ''">
22-
<ds-pool-task-actions id="actions"
22+
<ds-pool-task-actions id="actions-{{ dso?.id }}"
2323
[item]="item$.value"
2424
[object]="dso"
2525
[workflowitem]="workflowitem$.value"

src/app/shared/object-list/my-dspace-result-list-element/pool-search-result/pool-search-result-list-element.component.spec.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ const environmentUseThumbs = {
102102
const rdItem = createSuccessfulRemoteDataObject(item);
103103
const workflowitem = Object.assign(new WorkflowItem(), { item: of(rdItem) });
104104
const rdWorkflowitem = createSuccessfulRemoteDataObject(workflowitem);
105-
mockResultObject.indexableObject = Object.assign(new PoolTask(), { workflowitem: of(rdWorkflowitem) });
105+
mockResultObject.indexableObject = Object.assign(new PoolTask(), { id: 'pool-task-1', workflowitem: of(rdWorkflowitem) });
106106
const linkService = getMockLinkService();
107107
const objectCacheServiceMock = jasmine.createSpyObj('ObjectCacheService', {
108108
remove: jasmine.createSpy('remove'),
@@ -171,4 +171,13 @@ describe('PoolSearchResultListElementComponent', () => {
171171
const thumbnail = fixture.debugElement.query(By.css('.offset-3'));
172172
expect(thumbnail).toBeTruthy();
173173
});
174+
175+
it('should give the pool task actions a row-specific id', () => {
176+
const actions = fixture.debugElement.query(By.css('ds-pool-task-actions'));
177+
178+
expect(actions).toBeTruthy();
179+
// /mydspace renders one of these per pool task, so a constant id duplicates across rows
180+
expect(actions.nativeElement.id).not.toEqual('actions');
181+
expect(actions.nativeElement.id).toContain(component.dso.id);
182+
});
174183
});

0 commit comments

Comments
 (0)