Repository navigation
Conversation
After the Ant Design v6 upgrade, the placeholder, value and search caret in a native filter bar Select value control render centered instead of at the start of the field (reported and confirmed in Firefox devtools on a multiple Select filter, light and dark themes). Root cause is antd v6 itself: it injects a rule targeting .ant-select-content directly for multiple-mode Selects (:where(.cssHash).ant-select-multiple .ant-select-content, specificity 0,2,0) that sets text-align: center. The Select search input lives inside .ant-select-content and inherits that alignment, so the caret and typed text sit in the middle. Because antd targets .ant-select-content directly, a text-align inherited from an ancestor wrapper cannot override it. Override the element itself, scoped to the filter value wrapper: .ant-select .ant-select-content under the FilterPluginStyle class resolves to specificity 0,3,0, beating antds 0,2,0 without !important. text-align: start keeps it RTL-safe. The wrapper also keeps text-align: start for the Input/InputNumber value controls (is-like text filter, numeric range) which resolve alignment by inheritance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dosubot
Bot
added
change:frontend
Code Review Agent Run #2cab05Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| /* Input / InputNumber filter value controls resolve text-align by */ | ||
| /* inheritance, so pin them to the inline start to keep the caret and typed */ | ||
| /* text at the left edge. */ |
There was a problem hiding this comment.
Suggestion: The new comment says text is kept at the “left edge,” but the implemented rule uses text-align: start, which maps to the right edge in RTL; update the comment to describe “inline start” instead to avoid misleading future changes and tests. [comment mismatch]
Severity Level: Minor 🧹
- ⚠️ Comment misleading about RTL alignment of filter inputs.
- ⚠️ No runtime behavior affected; UI works as intended.Steps of Reproduction ✅
1. Open `superset-frontend/src/filters/components/common.ts` and locate
`FilterPluginStyle` definition at lines 25-44 in the PR diff.
2. Observe the comments at lines 28-30: they mention "inline start" (correct for
`text-align: start`) but also say "text at the left edge."
3. Note the implementation at line 31: `text-align: start;` which, per CSS spec, aligns
text to the right edge in RTL contexts.
4. In an RTL dashboard (where inline start is right), native filter value inputs render
caret/text at the right edge, while the comment still describes "left edge", so the
comment is mildly misleading but behavior is correct.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/filters/components/common.ts
**Line:** 28:30
**Comment:**
*Comment Mismatch: The new comment says text is kept at the “left edge,” but the implemented rule uses `text-align: start`, which maps to the right edge in RTL; update the comment to describe “inline start” instead to avoid misleading future changes and tests.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The CSS property Here is the corrected code snippet: /* Input / InputNumber filter value controls resolve text-align by */
/* inheritance, so pin them to the inline start to keep the caret and typed */
/* text at the inline start. */
text-align: start;I have checked the available PR comments, and there are no other review comments to address. Would you like me to perform any other tasks? superset-frontend/src/filters/components/common.ts |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42266 +/- ##
=======================================
Coverage 65.22% 65.22%
=======================================
Files 2768 2768
Lines 156280 156280
Branches 35774 35774
=======================================
Hits 101933 101933
Misses 52382 52382
Partials 1965 1965
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /* Input / InputNumber filter value controls resolve text-align by */ | ||
| /* inheritance, so pin them to the inline start to keep the caret and typed */ | ||
| /* text at the left edge. */ | ||
| text-align: start; |
There was a problem hiding this comment.
Can we add a focused Jest regression test for this fix? Rendering FilterPluginStyle and asserting each retained text-align: start rule with toHaveStyleRule would protect the corrective CSS; the existing DOM-contract test only protects the Ant Design class hook.
| /* text at the left edge. */ | ||
| text-align: start; | ||
|
|
||
| /* antd v6 sets 'text-align: center' directly on '.ant-select-content' for */ |
There was a problem hiding this comment.
The locked Ant Design 6.5.1 CSS does not set text-align: center on .ant-select-multiple .ant-select-content; that rule only sets wrapping, item alignment, and line height. Can we keep the wrapper-level rule, remove this nested selector, and document the actual inherited source instead?
yousoph
left a comment
yousoph
left a comment
There was a problem hiding this comment.
Reviewed the CSS change and dug into the root cause. The diff itself is safe — text-align: start is the CSS initial value, RTL-safe, and scoped to native filter value controls — so this is low-risk to land. Two substantive points, plus inline suggestions on the comment wording.
1. The stated root cause doesn't match stock antd. I checked the compiled Select styles across antd 6.0.0 → 6.5.1 (this PR resolves to 6.5.1). In every one of those versions the only text-align: center in the Select style layer is on .ant-select-clear (the clear icon) — there is no rule setting text-align: center on .ant-select-content, for multiple-mode or otherwise. So the comment's claim that "antd v6 sets text-align: center directly on .ant-select-content … specificity 0,2,0" isn't accurate for the version being shipped; a centered alignment on that element would have to come from a theme-level / css-in-js override on the affected deployment rather than upstream antd.
That doesn't make the change wrong — pinning text-align: start is reasonable defensive hardening — but it means on stock OSS this is likely a no-op, and the comment overstates what upstream does. Could you confirm the centered caret actually reproduces on a vanilla OSS build (antd 6.5.1)? If it only shows up under a custom theme, worth saying so. I've suggested reworded comments inline that keep the (sound) specificity reasoning without the upstream claim.
2. There's a byte-identical copy of this wrapper that isn't patched. superset-frontend/src/chartCustomizations/components/common.ts defines the same FilterPluginStyle (used by DynamicGroupBy / TimeColumn / TimeGrain / DeckglLayerVisibility). If this alignment issue is real for those controls too, the fix should be applied there as well — or a note on why it's out of scope. (It's not in this diff, so flagging here rather than inline.)
Minor: the "left edge" phrasing reads as physical-left; "inline start" is the RTL-correct term (covered in the inline suggestion).
| /* Input / InputNumber filter value controls resolve text-align by */ | ||
| /* inheritance, so pin them to the inline start to keep the caret and typed */ | ||
| /* text at the left edge. */ |
There was a problem hiding this comment.
Reword to avoid the physical-"left" framing (RTL-correct) and cover all three value controls:
| /* Input / InputNumber filter value controls resolve text-align by */ | |
| /* inheritance, so pin them to the inline start to keep the caret and typed */ | |
| /* text at the left edge. */ | |
| /* Filter value controls (Input / InputNumber / Select search) resolve */ | |
| /* 'text-align' by inheritance, so pin them to the inline start to keep the */ | |
| /* caret and typed text at the inline-start edge. */ |
| /* antd v6 sets 'text-align: center' directly on '.ant-select-content' for */ | ||
| /* multiple-mode Selects (its selector '.ant-select-multiple */ | ||
| /* .ant-select-content' has specificity 0,2,0). The Select search input */ | ||
| /* lives inside '.ant-select-content' and inherits that value, so the caret */ | ||
| /* and typed text render centered. A value inherited from this wrapper */ | ||
| /* cannot beat a rule that targets '.ant-select-content' directly, so */ | ||
| /* override the element itself: this wrapper class plus the two antd classes */ | ||
| /* give specificity 0,3,0, which wins without !important. RTL-safe. */ |
There was a problem hiding this comment.
Stock antd 6.0.0–6.5.1 doesn't set text-align: center on .ant-select-content (only on .ant-select-clear), so this rewording drops the inaccurate "antd v6 sets…" claim while keeping the specificity reasoning, which is correct:
| /* antd v6 sets 'text-align: center' directly on '.ant-select-content' for */ | |
| /* multiple-mode Selects (its selector '.ant-select-multiple */ | |
| /* .ant-select-content' has specificity 0,2,0). The Select search input */ | |
| /* lives inside '.ant-select-content' and inherits that value, so the caret */ | |
| /* and typed text render centered. A value inherited from this wrapper */ | |
| /* cannot beat a rule that targets '.ant-select-content' directly, so */ | |
| /* override the element itself: this wrapper class plus the two antd classes */ | |
| /* give specificity 0,3,0, which wins without !important. RTL-safe. */ | |
| /* Some deployments (custom themes / css-in-js overrides) can leak a centered */ | |
| /* 'text-align' onto '.ant-select-content'. The Select search <input> lives */ | |
| /* inside '.ant-select-content' and resolves 'text-align' by inheritance, so */ | |
| /* that would center the caret and typed text. Overriding '.ant-select-content' */ | |
| /* directly (this wrapper class + two antd classes = specificity 0,3,0) beats */ | |
| /* both an inherited value and a '.ant-select-multiple .ant-select-content' */ | |
| /* rule without '!important'. 'start' is the CSS default, so this is a no-op */ | |
| /* unless a centered value leaked in, and it's RTL-safe. */ |
|
Superseded by #42323, which reworks this same fix to address review feedback: it keeps the wrapper-level |
SUMMARY
When clicking into a dashboard native-filter value control (e.g. a multi-select filter), the caret and the placeholder/typed text render centered instead of left-aligned.
This is a regression from the Ant Design v6 upgrade. antd v6 emits
text-align: centerdirectly on.ant-select-multiple .ant-select-content(its own css-in-js). The Select search<input>lives inside.ant-select-contentand resolvestext-alignby inheritance, so the caret/text inherit the centered value.Fix: in the shared
FilterPluginStylewrapper used by all native filter value plugins, pintext-align: startand override.ant-select .ant-select-contentdirectly. Overriding the element itself is required because a value inherited from an ancestor cannot beat a rule that targets.ant-select-contentdirectly. The override's specificity (wrapper class + two antd classes =0,3,0) beats antd's.ant-select-multiple .ant-select-content(0,2,0) without!important.startis the CSS default, so this is a no-op except where antd leaked the centered alignment, and it's RTL-safe.Scoped to native filter value controls (Select, Range, Time, TimeColumn, TimeGrain) — no app-wide blast radius.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION