[DataGrid] Do not ellipsize non-text cell content - #23336
Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf1e91b259
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (children != null && typeof children !== 'string' && typeof children !== 'number') { | ||
| classNames.push(gridClasses['cell--element']); |
There was a problem hiding this comment.
Apply the documented custom class
When a user customizes the new slot with classes={{ 'cell--element': 'myElementCell' }}, element cells only receive the utility class and never get the custom class from rootProps.classes. Since this commit adds cell--element to GridClasses/the API docs, the documented classes hook silently does nothing for this new state; please append rootClasses?.['cell--element'] (or compose this state) alongside the utility class.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the next commit.
Worth noting the file has both precedents: cell--editing pushes rootClasses?.['cell--editing'] alongside the utility class, while cell--flex pushes only the utility class. Since cell--element is new documented API, I have followed the cell--editing pattern:
classNames.push(gridClasses['cell--element']);
classNames.push(rootClasses?.['cell--element']);Added a test asserting classes={{ 'cell--element': 'foobar' }} lands on the cell, which fails without the second push.
cell--flex has had the same gap since #12013, but I have left it alone to keep this PR scoped. Happy to open a separate issue if maintainers want the three manual pushes aligned.
|
ready for review whenever you have a moment. |
| whiteSpace: 'nowrap', | ||
| textOverflow: 'ellipsis', | ||
| // Avoids painting a clipped "…" next to overflowing widget content. | ||
| [`&.${c['cell--element']}`]: { |
There was a problem hiding this comment.
This is too specific and will override user's css. We probably need to put it at default level so the following works:
sx={{ [& .${gridClasses.cell}]: { textOverflow: 'ellipsis' } }}
There was a problem hiding this comment.
You are right, and it was worse than too specific: nested inside the .cell block it resolved to .root .MuiDataGrid-cell.MuiDataGrid-cell--nonText at (0,3,0), so an sx targeting .cell at (0,2,0) could never win.
Moved it out to a sibling rule next to cell--flex. It now sits at the same specificity as the base cell rule and only wins by source order, which leaves sx free to override it.
Added a browser test using your snippet:
sx={{ [`& .${gridClasses.cell}`]: { textOverflow: 'ellipsis' } }}It fails against the previous commit with expected 'clip' to equal 'ellipsis', and passes now.
| /** | ||
| * Styles applied to the cell element if its content is not text. | ||
| */ | ||
| 'cell--element': string; |
There was a problem hiding this comment.
cell--element might be too generic. Maybe cell--customElement, cell--nonText or cell--renderElement?
There was a problem hiding this comment.
Agreed, renamed to cell--nonText.
I went with that one of your three because it states the condition exactly and matches the existing display: 'text' | 'flex' vocabulary on GridColDef. I skipped cell--customElement since "custom element" is an established Web Components term, and cell--renderElement since the class also applies to edit cells, not just renderCell output. Happy to switch if you prefer one of the others.
| }); | ||
|
|
||
| // See https://github.com/mui/mui-x/issues/23332 | ||
| describe('text overflow', () => { |
There was a problem hiding this comment.
Probably good to test renderCell: ()=>null and also returning false
There was a problem hiding this comment.
Good call, false was actually broken. null was already handled, but typeof false is neither 'string' nor 'number', so a cell rendering nothing still got the class.
Tightened the check to:
if (typeof children === 'object' && children !== null) {That is correct across every React child type: elements and arrays are objects, while null, undefined, false, true, strings, numbers and bigints are excluded. It fixes bigint as a side effect, which the previous check also got wrong. Tests added for both null and false, and the false one fails against the previous commit.
| title = valueString; | ||
| } | ||
|
|
||
| if (children != null && typeof children !== 'string' && typeof children !== 'number') { |
There was a problem hiding this comment.
Btw, edit cells now always carry cell--element too, not sure we want to do anything about it though? 🤔
probably makes sense
There was a problem hiding this comment.
Yes, and I think it is correct to leave as is. Edit cells already carry cell--editing, which sets display: flex, so text-overflow was already inert on them before this PR. The class is accurate there too since the content genuinely is an element, so it costs nothing and keeps the rule uniform.
Closes #23332
Problem
.MuiDataGrid-cellsetstext-overflow: ellipsis. That property does not only affect text: per the CSS Overflow spec it also applies to overflowing atomic inlines (inline-block,inline-flex,img).A MUI
IconButtonisinline-flex, so when arenderCellwidget is wider than the cell content box the browser paints a literal…after it, whichoverflow: hiddenthen slices in half. The result is a column of stray half-dots beside every row.It only shows up in a narrow band of column widths, which is why it reads as a rendering glitch rather than as truncation. Reproducing the reporter's case (a 36px
IconButton, cell padding0 10px) in Chromium:........At 56px the content box is exactly 36px, so the widget stops overflowing and the artifact disappears. Below 48px the ellipsis is pushed past the clip edge and disappears again.
Fix
Ellipsis is meaningful for truncated text, not for widget content. This turns it off for non-text content and changes nothing else:
GridCelladds a newcell--elementclass when the resolved children is neither a string nor a number.GridRootStylesneststext-overflow: clipunder that class inside the existing.cellblock.text-overflowis paint-only (the spec states that ellipsing must not affect layout), so this touches no layout, no vertical alignment, no column autosizing and adds no DOM node. The perf win from #12013, which removed the.cellContentwrapper, is preserved.Cells still ellipsize when
renderCellreturns a string or a number, and when the value is rendered by default.display: 'flex'columns are unaffected, since a flex cell already ignorestext-overflow.Behavior change worth calling out
renderCell: () => <span>{longText}</span>, an inline element child, loses the cell level ellipsis and is hard clipped instead. This is the only regression class and it cannot be avoided without measuring the DOM, since React cannot distinguish a span of text from a widget.It matches the guidance already in the v6 to v7 migration guide, which tells users to bring their own wrapper for element content:
Block level children never received the cell ellipsis in the first place, so nothing regresses there. That includes the built in
treeDataGroupingCellandgroupingCriteriaCell, which are bothdisplay: flex.Notes on the approach
Two heavier alternatives were considered and rejected:
display: 'flex'when the content is an element. This was proposed during the review of [data grid] Performance: DOM changes #12013 and set aside in favour of the explicitdisplayprop. It silently re-centers every existingrenderCellcolumn, so the visual blast radius is large.Testing
Five cases added to
cells.DataGrid.test.tsx, covering element content,renderCellreturning text, and the default text cell. Two of them are browser only and assert the resolvedgetComputedStyle(cell).textOverflowisclipfor element content andellipsisfor text.Changelog
Cells rendering a custom widget through
renderCellno longer paint a clipped text ellipsis next to overflowing content. The ellipsis is now applied only to text content.