Copilot commented on code in PR #36149:
URL: https://github.com/apache/superset/pull/36149#discussion_r2539464901
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -632,24 +632,41 @@ describe('plugin-chart-table', () => {
// IDs should only contain valid characters: alphanumeric,
underscore, hyphen
expect(header.id).toMatch(/^header-[a-zA-Z0-9_-]+$/);
});
+ });
- // CRITICAL: Verify ALL cells reference valid headers (no broken ARIA)
+ test('should validate ARIA references for time-comparison table cells',
() => {
+ // Test that ALL cells with aria-labelledby have valid references
+ // This is critical for screen reader accessibility
+ const props = transformProps(testData.comparison);
+
+ const { container } = render(<TableChart {...props} sticky={false} />);
+
+ const allCells = container.querySelectorAll('tbody td');
const cellsWithLabels = container.querySelectorAll(
- 'td[aria-labelledby]',
+ 'tbody td[aria-labelledby]',
);
+
+ // First assertion: Table must render data cells (catch empty table
regression)
+ expect(allCells.length).toBeGreaterThan(0);
+
+ // Second assertion: ALL data cells must have aria-labelledby (no
unlabeled cells)
+ expect(cellsWithLabels.length).toBe(allCells.length);
+
+ // Third assertion: ALL aria-labelledby values should be valid
cellsWithLabels.forEach(cell => {
const labelledBy = cell.getAttribute('aria-labelledby');
- if (labelledBy) {
- // Check that the ID doesn't contain spaces (would be interpreted
as multiple IDs)
- expect(labelledBy).not.toMatch(/\s/);
- // Check that the ID doesn't contain special characters
- expect(labelledBy).not.toMatch(/[%#△]/);
- // Verify the referenced header actually exists
- const referencedHeader = container.querySelector(
- `#${CSS.escape(labelledBy)}`,
- );
- expect(referencedHeader).toBeTruthy();
- }
+ expect(labelledBy).not.toBeNull();
+ expect(labelledBy).toEqual(expect.stringMatching(/\S/));
+ const labelledByValue = labelledBy as string;
+ // Check that the ID doesn't contain spaces (would be interpreted as
multiple IDs)
+ expect(labelledByValue).not.toMatch(/\s/);
+ // Check that the ID doesn't contain special characters
+ expect(labelledByValue).not.toMatch(/[%#△]/);
+ // Verify the referenced header actually exists
+ const referencedHeader = container.querySelector(
+ `#${CSS.escape(labelledByValue)}`,
+ );
+ expect(referencedHeader).toBeTruthy();
});
Review Comment:
This validation logic is duplicated in both the time-comparison test (lines
656-670) and the regular table test (lines 756-770). Consider extracting this
into a helper function to reduce code duplication and improve maintainability.
For example:
```typescript
const validateAriaReferences = (cellsWithLabels: NodeListOf<Element>,
container: HTMLElement) => {
cellsWithLabels.forEach(cell => {
const labelledByValue = cell.getAttribute('aria-labelledby') as string;
expect(labelledByValue).not.toMatch(/\s/);
expect(labelledByValue).not.toMatch(/[%#△]/);
const referencedHeader =
container.querySelector(`#${CSS.escape(labelledByValue)}`);
expect(referencedHeader).toBeTruthy();
});
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
