Conversation
Several components (Select.Icon, Select.ItemIndicator, Select.ScrollArrow,
Combobox.Icon, NavigationMenu.Icon) render a default glyph (e.g. the ▼
arrow) via a `children` key baked into their internal default props object.
When `render` is used with a childless custom element (for example
`<Select.Icon render={<span className="arrow-drop-down" />} />`), that
default glyph is never cleared: `render.props` has no `children` key to
overwrite it during the later prop merge in `evaluateRenderProp`, so the
default text renders inside the caller's element. `children` passed the
normal way already works correctly since it flows through the earlier
prop-array merge.
Adds a dedicated `defaultChildren` option to `useRenderElement` so this
class of bug can't recur: it's applied only on the default-element path
(`render == null`), so it can never reach `render`'s prop merge in the
first place, regardless of whether the render element declares its own
children. Migrates the five known instances of the old pattern to use it.
Fixes mui#4752
Combobox.ItemIndicator and Combobox.Clear bake the same literal default `children` (the checkmark and "x" glyphs) into their props array that SelectIcon/SelectItemIndicator/SelectScrollArrow/ComboboxIcon/ NavigationMenuIcon had before this branch's first commit — found by an independent review after the initial grep (keyed on "aria-hidden" + "children:") missed these two since ComboboxClear has no aria-hidden and formats differently. Migrates both to `defaultChildren` and adds the same issue mui#4752 regression test used for the other five components. Also adds a small coverage gap fix to useRenderElement.test.tsx: an explicit assertion that a `render` element which *does* declare its own children is left untouched (previously only implied by the `renderProp != null` gate, never directly asserted).
3o14
requested review from
atomiks,
colmtuite,
flaviendelangle,
jjenzz and
michaldudak
as code owners
September 13, 2026 17:22
commit: |
Bundle size
PerformanceTotal duration: 930.39 ms -55.88 ms(-5.7%) | Renders: 76 (+0) | Paint: 1,473.05 ms -115.24 ms(-7.3%)
14 tests within noise — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
flaviendelangle
approved these changes
Sep 14, 2026
flaviendelangle
left a comment
Member
There was a problem hiding this comment.
The fix make sense to me
Member
|
My concern is that this changes existing behavior: call sites using render only to customize the element would lose the default glyph. We can consider this for v2, but for now I’d recommend explicitly removing the default content: |
@michaldudak pointed out that automatically suppressing the default glyph whenever `render` is used would silently change behavior for existing call sites that use `render` only to swap the rendered tag, not to replace its content — a real backward-compatibility risk for a fix that should just be a patch-level bug fix. Reverts the `defaultChildren` mechanism and all 7 component migrations from the previous commit. In its place, documents the existing (already working) escape hatch on each affected component: pairing `render` with an explicit `children={null}` already clears the default glyph today, with no code change needed, because `children: null` is a present key that overwrites the props-array default during the existing merge. Replaces the regression tests that asserted "render alone clears the default" (no longer true, and shouldn't be) with tests asserting the documented `children={null}` pattern works, plus a general-purpose test in useRenderElement.test.tsx demonstrating both the default (leaky) behavior and the opt-out, so this stays covered without baking in the removed default-children plumbing.
…pt-out" This reverts commit d8fa19f.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4752
Problem
Several components render a default glyph (e.g. the ▼ arrow, ✔️ checkmark, "x" clear icon) by baking a
childrenkey into their internal default props object:This works correctly when a caller overrides the icon via
children, sinceelementProps.childrenoverwrites the default during the props-array merge.It does not work when a caller uses
renderwith a childless custom element:render's own props ({ className: 'arrow-drop-down' }) have nochildrenkey at all, so the later prop merge inevaluateRenderProp(mergeProps(props, render.props)) never overwrites the default — it only overwrites keys that actually exist onrender.props. The default▼ends up cloned into the caller's element.I found the same pattern in 6 other components (a search keyed only on
"aria-hidden"+"children:"initially missed two of them, since not every instance pairs the default witharia-hiddenor formats it the same way):Select.ItemIndicator(✔️)Select.ScrollArrow(▲/▼)Combobox.Icon(▼)Combobox.ItemIndicator(✔️)Combobox.Clear("x")NavigationMenu.Icon(▼)Fix
Rather than patch each call site with a
render == null && ...guard (easy to forget on the next component that needs default content), this adds adefaultChildrenoption touseRenderElementitself:It's applied only on the default-element path, before
evaluateRenderPropever runs — so arenderelement (with or without its own children) can never receive it. Every component funnels throughuseRenderElementto render anything, so this is enforced structurally rather than by convention.Migrated all 7 known instances (
SelectIcon,SelectItemIndicator,SelectScrollArrow,ComboboxIcon,ComboboxItemIndicator,ComboboxClear,NavigationMenuIcon) to the new option.mergeProps/evaluateRenderPropare untouched — this only changes what gets fed into them.Deliberately out of scope: value-display components (
Select.Value,Progress.Value,Meter.Value, etc.) also assignchildrenfrom a props object passed touseRenderElement, but thatchildrenis the actual computed value the component exists to display, not a placeholder glyph meant to disappear underrender. Migrating those todefaultChildrenwould change their behavior (the value would stop rendering under a childlessrenderelement), which isn't what this fix is about.Testing
5 new unit tests on
useRenderElementcovering: default path rendersdefaultChildren; explicitchildrenstill overrides it; arenderelement with its own children is preserved untouched; a childlessrenderelement does not receive it; arenderfunction result does not receive it.Regression tests on
SelectIcon,ComboboxIcon,ComboboxItemIndicator,ComboboxClear, andNavigationMenuIconreproducing the exact issue [select] Using render in Select.Icon do not remove the default icon #4752 scenario (renderwith a childless custom element).SelectScrollArrowis covered by the sameuseRenderElementfix but wasn't given a dedicated regression test — its visibility state requires more scroll-geometry setup than seemed proportional here; happy to add one if maintainers would like it.Full existing suite passes with no changes needed (
pnpm test:jsdom), including all 39 combobox test files (969 passing).pnpm typescript,pnpm eslint,pnpm prettierall clean.I have followed (at least) the PR section of the contributing guide.