Copilot commented on code in PR #36149:
URL: https://github.com/apache/superset/pull/36149#discussion_r2536010517
##########
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).not.toBe('');
Review Comment:
[nitpick] The assertions on lines 658-659 are redundant after the type
casting on line 660. Since `labelledByValue` is cast to `string` on line 660,
these checks for `null` and empty string are unnecessary—TypeScript will ensure
it's a string type. Consider removing lines 658-659 or moving the type cast
before these checks.
The same pattern exists in the test for regular table cells (lines 758-760).
```suggestion
```
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -711,24 +728,45 @@ describe('plugin-chart-table', () => {
// IDs should only contain valid CSS selector characters
expect(header.id).toMatch(/^header-[a-zA-Z0-9_-]+$/);
});
+ });
- // Test 6: Verify ALL cells reference valid headers (no broken ARIA)
+ test('should validate ARIA references for regular table cells', () => {
+ // Test that ALL cells with aria-labelledby have valid references
+ // This is critical for screen reader accessibility
+ const props = transformProps(testData.advanced);
+
+ const { container } = render(
+ ProviderWrapper({
+ children: <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) {
- // Verify no spaces (would be interpreted as multiple IDs)
- expect(labelledBy).not.toMatch(/\s/);
- // Verify no 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).not.toBe('');
Review Comment:
[nitpick] The assertions on lines 758-759 are redundant after the type
casting on line 760. Since `labelledByValue` is cast to `string` on line 760,
these checks for `null` and empty string are unnecessary—TypeScript will ensure
it's a string type. Consider removing lines 758-759 or moving the type cast
before these checks.
This is the same pattern as in the time-comparison test (lines 658-660).
```suggestion
```
--
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]
