Skip to content

fix(table-chart): fix missing table header IDs #35968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sadpandajoe merged 5 commits into from
Nov 12, 2025
Merged

fix(table-chart): fix missing table header IDs #35968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
790fe5c
fix(table-chart): fix table header IDs and accessibility (#35783)
sadpandajoe Nov 4, 2025
9fcddc5
perf(table-chart): optimize header ID sanitization and add unit tests
sadpandajoe Nov 4, 2025
f680611
refactor(table-chart): improve header ID sanitization based on review…
sadpandajoe Nov 6, 2025
5407932
refactor(table-chart): add comments and edge case handling for saniti…
sadpandajoe Nov 6, 2025
ebfb2a9
docs(table-chart): improve comments and test consistency
sadpandajoe Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,31 @@ function cellWidth({
return perc2;
}

/**
* Sanitize a column identifier for use in HTML id attributes and CSS selectors.
* Replaces characters that are invalid in CSS selectors with safe alternatives.
*
* Note: The returned value should be prefixed with a string (e.g., "header-")
* to ensure it forms a valid HTML ID (IDs cannot start with a digit).
*
* Exported for testing.
*/
export function sanitizeHeaderId(columnId: string): string {
return (
columnId
// Semantic replacements first: preserve meaning in IDs for readability
// (e.g., '%pct_nice' → 'percentpct_nice' instead of '_pct_nice')
.replace(/%/g, 'percent')
.replace(/#/g, 'hash')
.replace(/△/g, 'delta')
// Generic sanitization for remaining special characters
.replace(/\s+/g, '_')
.replace(/[^a-zA-Z0-9_-]/g, '_')
.replace(/_+/g, '_') // Collapse consecutive underscores
.replace(/^_+|_+$/g, '') // Trim leading/trailing underscores
);
}
Comment on lines +153 to +167

Copilot AI Nov 6, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sanitization logic has an ordering issue. Line 163 replaces all non-alphanumeric characters (except underscore and hyphen) with underscores, which means semantic replacements from lines 158-160 can introduce characters that are then immediately replaced. For example, sanitizeHeaderId('%') replaces % with 'percent' (✓), but if a column name is '%', it becomes 'percent' correctly. However, consider '%@': it becomes 'percent@' → 'percent_' → 'percent'. This works, but the flow could be clearer. The logic is correct but the comment on line 157 is misleading - it says the semantic replacement avoids '_pct_nice', but actually '%pct_nice' → 'percentpct_nice' → 'percentpct_nice' (no underscores between 'percent' and 'pct'). Consider updating the comment to accurately reflect the behavior.

Copilot uses AI. Check for mistakes.

/**
* Cell left margin (offset) calculation for horizontal bar chart elements
* when alignPositiveNegative is not set
Expand Down Expand Up @@ -844,6 +869,9 @@ export default function TableChart<D extends DataRecord = DataRecord>(
}
}

// Cache sanitized header ID to avoid recomputing it multiple times
const headerId = sanitizeHeaderId(column.originalLabel ?? column.key);

return {
id: String(i), // to allow duplicate column keys
// must use custom accessor to allow `.` in column names
Expand Down Expand Up @@ -969,7 +997,7 @@ export default function TableChart<D extends DataRecord = DataRecord>(
}

const cellProps = {
'aria-labelledby': `header-${column.key}`,
'aria-labelledby': `header-${headerId}`,
role: 'cell',
// show raw number in title in case of numeric values
title: typeof value === 'number' ? String(value) : undefined,
Expand Down Expand Up @@ -1056,7 +1084,7 @@ export default function TableChart<D extends DataRecord = DataRecord>(
},
Header: ({ column: col, onClick, style, onDragStart, onDrop }) => (
<th
id={`header-${column.originalLabel}`}
id={`header-${headerId}`}
title={t('Shift + Click to sort by multiple columns')}
className={[className, col.isSorted ? 'is-sorted' : ''].join(' ')}
style={{
Expand Down
Loading
Loading