Repository navigation
Conversation
The extended variant pinned a fixed height per size and used no vertical padding, so a label that wrapped was laid out taller than the button and rendered outside it. In Chromium a two-line label sits 0.5px, 4.5px and 7.5px outside the large, medium and small buttons; a three-line label 12.8px, 16.8px and 19.8px. Swapping the fixed height for the same value as a minimum, plus vertical padding, lets the button grow instead. A one-line label still measures 48, 40 and 34px, so nothing moves for the common case.
Deploy previewBundle size
PerformanceNo regressions in 32 benchmarks · ran 4m 06s · details Check out the code infra dashboard for more information about this PR. |
ZeeshanTamboli
added
type: bug
ZeeshanTamboli
left a comment
ZeeshanTamboli
left a comment
There was a problem hiding this comment.
Thanks for this! The fix works: a wrapped label now stays inside the button, and with the default theme a one-line label still measures 48, 40 and 34px.
However, this is a breaking change, and the description's "nothing moves for the common case" doesn't hold. I measured master and this branch in Chromium and Firefox, with identical results in both:
| Case | master | this PR |
|---|---|---|
| Large, single line | 48 | 48 |
| Medium / small, single line | 40 / 34 | 40 / 34 |
| Large, inside a flex row 100px tall | 48 | 100 |
| Small, flex row next to a 56px sibling | 34 | 56 |
| Large, inside a grid row 80px tall | 48 | 80 |
Large with sx={{ height: 30 }} |
30 | 48 |
Small with sx={{ height: 28 }} |
28 | 34 |
Small, theme typography.fontSize: 16 |
34 | 36 |
| Small with a 35px child (large icon) | 34 | 43 |
Details are in the inline comments. In short:
- Regression: with
height: 'auto', a flex or grid parent now stretches the Fab to match its tallest sibling.height: 'fit-content'avoids this. - Breaking change: the new
minHeightoverrides any smallerheightset throughsx,styleOverridesor a theme variant. Custom sizes added through module augmentation inheritminHeight: 48. As far as I can tell, no CSS lets the button grow with its content and still keep a smaller explicitheight. So this either needs a breaking-change note in the changelog and migration guide, or should wait for the next major.
The visual regression suite won't catch either one, because both extended Fab demos use block containers.
| padding: '8px 16px', | ||
| width: 'auto', | ||
| minHeight: 'auto', | ||
| height: 'auto', |
There was a problem hiding this comment.
Regression: the Fab now stretches inside flex rows and grid cells.
Flex and grid parents stretch a child by default, but only when its height is auto. The fixed height: 48 on master prevented that. Now an extended Fab in a <Stack direction="row">, a display: flex row or a grid cell grows to match its tallest sibling:
- flex row 100px tall: 48 → 100
- small Fab next to a 56px sibling: 34 → 56
- grid row 80px tall: 48 → 80
fit-content isn't auto, so the parent doesn't stretch it, and it still resolves to the content height, so the button still grows. On top of this branch it gives 48 in both the flex and grid cases and 65 for the wrapped label. All browsers in our browserslist support it (Chrome 117, Firefox 121, Safari 17).
| height: 'auto', | |
| // `fit-content` rather than `auto`: grows with a wrapping label, but | |
| // isn't stretched by a flex or grid parent. | |
| height: 'fit-content', |
| height: 'auto', | ||
| minWidth: 48, | ||
| height: 48, | ||
| minHeight: 48, |
There was a problem hiding this comment.
Breaking change: a smaller height customization no longer applies.
On master the extended variant set minHeight: 'auto', so a smaller height from sx, styleOverrides.extended or a theme variant took effect. Now this minHeight wins, and so do the 34 and 40 below for the small and medium sizes:
<Fab variant="extended" sx={{ height: 30 }}>: 30 → 48<Fab variant="extended" size="small" sx={{ height: 28 }}>: 28 → 34
Custom sizes added through module augmentation are hit hardest. No built-in variant matches them, so they inherit minHeight: 48 and padding: '8px 16px'. A custom xsmall size with height: 28 renders at 48.
Users can fix it by also setting minHeight, but nothing warns them, so their layouts just change. I don't see a CSS approach that grows with the content and still keeps a smaller explicit height. So I think this has to be called out as a breaking change, with a changelog entry and a migration note, or held for the next major. Please also update the PR description, which currently says nothing moves.
| style: { | ||
| width: 'auto', | ||
| padding: '0 8px', | ||
| padding: '4px 8px', |
There was a problem hiding this comment.
The small size leaves 26px for content (34 − 2 × 4px), and the default label line is 24.5px, so there's only 1.5px of headroom. Anything slightly taller now changes the default height:
createTheme({ typography: { fontSize: 16 } }): 34 → 36- a 35px child, such as a
fontSize="large"icon: 34 → 43. A default 40pxCircularProgressorAvatarwould behave the same way.
You could argue this is the fix working as intended, since that content used to overflow. But it's another visible change for apps with a custom theme. To keep it smaller, 2px vertical padding leaves 30px of room, which fits 16px type. The cost is tighter spacing around a wrapped label:
| padding: '4px 8px', | |
| padding: '2px 8px', |
|
|
||
| // JSDOM does no layout, so the overflow this guards against is only visible | ||
| // in a real browser. | ||
| it.skipIf(isJsdom())('keeps a wrapping label inside the extended button', () => { |
There was a problem hiding this comment.
This only covers the large size, and no test checks that a one-line label keeps its height. Could you add:
- a check that a one-line label measures 48, 40 and 34px for the large, medium and small sizes
- a check that a flex parent doesn't stretch the Fab, so the regression flagged in
Fab.jscan't come back. This one fails on the current branch, which measures 100px:
it.skipIf(isJsdom())('is not stretched by a flex parent', () => {
render(
<div style={{ display: 'flex', height: 100 }}>
<Fab variant="extended">Add</Fab>
</div>,
);
expect(screen.getByRole('button').getBoundingClientRect().height).to.equal(48);
});
ZeeshanTamboli
added
the
breaking change
height: auto let a flex or grid parent stretch the button: in a flex row or a grid with a 160px track it grew from 48 to 160. fit-content keeps it sized to the label while still letting it grow when the label wraps. Also drops the small size's vertical padding to 2px, which left only 1.5px of headroom under its 34px minimum, and adds tests for the one-line height of each size and for the flex-parent case.
The extended variant sets a fixed height per size and no vertical padding, so once a label wraps the text is laid out taller than the button and renders outside it. Measured in Chromium, a two-line label sits 0.5px outside a large extended Fab, 4.5px outside a medium one and 7.5px outside a small one; at three lines it is 12.8px, 16.8px and 19.8px.
This keeps the same numbers but as a minimum rather than a fixed height, and adds vertical padding so the button grows to fit. A one-line label still measures 48, 40 and 34px, so nothing moves for the common case and the docs demos, which all use short labels, should render identically. The root still sets
height: 56for the circular variants, so the extended variant keeps an explicitheight: 'auto'.This covers the Fab height item from the Wave 1 follow-ups in #49477.
The test is guarded with
skipIf(isJsdom())because jsdom does no layout. It passes in the browser scope and fails on master withexpected 48 to be above 48. I have not run the visual regression suite