codeant-ai-for-open-source[bot] commented on code in PR #32995:
URL: https://github.com/apache/superset/pull/32995#discussion_r3557897417
##########
superset-frontend/packages/superset-ui-core/src/components/EditableTitle/index.tsx:
##########
@@ -60,7 +60,7 @@ const StyledEditableTitle = styled.span<{
font-size: ${({ theme }) => theme.fontSizeLG}px;
padding: ${({ theme }) => theme.sizeUnit / 2}px;
min-height: 100px;
- width: 95%;
+ width: 100%;
Review Comment:
**Suggestion:** This forces text inputs/textareas inside editable titles to
full width and can override the component’s measured width behavior
(`inputWidth`), causing title editors to stretch unexpectedly and break compact
layouts. Keep the width rule scoped to only the specific context that needs
full width instead of applying it globally here. [css layout issue]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard header title editor stretches, breaking compact layout.
- ⚠️ Tab title editor disregards auto-measured width configuration.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dashboard in edit mode so the header grid components render,
which uses
`EditableTitle` for the title (see
`superset-frontend/src/dashboard/components/gridComponents/Header/Header.tsx:271`
where
`EditableTitle` is instantiated with `title={component.meta.text}` and
`canEdit={editMode}`).
2. In `EditableTitle`
(`superset-frontend/packages/superset-ui-core/src/components/EditableTitle/index.tsx:69-76`),
`useLayoutEffect` measures the current title text width and stores it in
`inputWidth`, and
the `Input.TextArea` at lines 88-110 applies `css={theme => css\`&& { width:
${inputWidth}px; ... }\`}`, intending to auto-size the editor to the text
width.
3. The same file’s `StyledEditableTitle` styles (lines 17-25; hunk line 63)
include a
global rule for `input[type='text'], textarea` that sets `width: 100%`,
which applies to
the `Input.TextArea`’s underlying `textarea` element inside the editable
title.
4. In practice, when focusing the dashboard title to edit it, the text area
matches both
the measured-width rule and the global `width: 100%` rule; the full-width
rule causes the
editor to stretch across the container instead of respecting `inputWidth`,
visually
breaking the compact autosized title layout in dashboard headers and tabs.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=da8e6f79cd824ff4bfe9347a5902d1fa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=da8e6f79cd824ff4bfe9347a5902d1fa&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/EditableTitle/index.tsx
**Line:** 63:63
**Comment:**
*Css Layout Issue: This forces text inputs/textareas inside editable
titles to full width and can override the component’s measured width behavior
(`inputWidth`), causing title editors to stretch unexpectedly and break compact
layouts. Keep the width rule scoped to only the specific context that needs
full width instead of applying it globally here.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F32995&comment_hash=1d7b8962eb73c7c54f7cc2eb5d80a61cfc3cad31378e6d453019bed3221e502c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F32995&comment_hash=1d7b8962eb73c7c54f7cc2eb5d80a61cfc3cad31378e6d453019bed3221e502c&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/IconButton.tsx:
##########
@@ -16,41 +16,65 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { MouseEventHandler } from 'react';
-import { styled } from '@apache-superset/core/theme';
+import { forwardRef, HTMLAttributes, MouseEventHandler } from 'react';
+import { styled, SupersetTheme } from '@apache-superset/core/theme';
-interface IconButtonProps {
+interface IconButtonProps extends HTMLAttributes<HTMLDivElement> {
icon: JSX.Element;
label?: string;
onClick: MouseEventHandler<HTMLDivElement>;
+ disabled?: boolean;
+ 'data-test'?: string;
}
-const StyledDiv = styled.div`
+const disabledCss = `
+ cursor: not-allowed;
+ opacity: 0.5;
+`;
+
+const activeCss = ({ theme }: { theme: SupersetTheme }) => `
+ &:hover {
+ color: ${theme.colorPrimary};
+ background: ${theme.colorBgTextHover};
+ }
+`;
+
+const StyledDiv = styled.div<{ isDisabled?: boolean }>`
display: flex;
align-items: center;
cursor: pointer;
color: ${({ theme }) => theme.colorIcon};
- &:hover {
- color: ${({ theme }) => theme.colorPrimary};
- }
+ padding: ${({ theme }) => theme.paddingXXS}px;
+ border-radius: ${({ theme }) => theme.borderRadiusXS}px;
+
+ ${({ isDisabled, theme }) => (isDisabled ? disabledCss : activeCss({ theme
}))}
`;
const StyledSpan = styled.span`
margin-left: ${({ theme }) => theme.sizeUnit * 2}px;
`;
-const IconButton = ({ icon, label, onClick }: IconButtonProps) => (
- <StyledDiv
- tabIndex={0}
- role="button"
- onClick={e => {
- e.preventDefault();
- onClick(e);
- }}
- >
- {icon}
- {label && <StyledSpan>{label}</StyledSpan>}
- </StyledDiv>
+const IconButton = forwardRef<HTMLDivElement, IconButtonProps>(
+ ({ icon, label, onClick, disabled, 'data-test': dataTest, ...rest }, ref) =>
(
+ <StyledDiv
+ {...rest}
+ ref={ref}
+ tabIndex={0}
+ role="button"
+ isDisabled={disabled}
+ aria-disabled={disabled}
Review Comment:
**Suggestion:** Disabled icons remain in the tab order because `tabIndex` is
hardcoded to `0` even when `disabled` is true. In list views this creates many
non-actionable keyboard stops and violates disabled control behavior; set
`tabIndex` conditionally (for example `-1` when disabled). [incorrect condition
logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Disabled dashboard action icons remain focusable, cluttering tab order.
- ⚠️ Keyboard navigation experiences stops on non-actionable controls.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The `IconButton` component
(`superset-frontend/src/dashboard/components/IconButton.tsx:57-78`) is
defined as a
focusable pseudo-button: it renders a `StyledDiv` with `tabIndex={0}` and
`role="button"`
(lines 62-64), and exposes a `disabled` prop that drives `isDisabled` and
`aria-disabled`
(lines 64-65).
2. Tests already exercise the `disabled` prop by rendering `<IconButton
icon={icon}
onClick={onClick} disabled data-test="my-button" />` in
`superset-frontend/src/dashboard/components/IconButton.test.tsx:35-37`,
confirming that
`aria-disabled="true"` is set and click events are suppressed (lines 39-43).
3. Because `tabIndex` is hardcoded to `0` regardless of the `disabled` prop,
a disabled
`IconButton` remains in the keyboard tab order; the element is still
focusable even though
it is marked `aria-disabled` and styled with `cursor: not-allowed` via
`disabledCss`
(IconButton.tsx:30-33, 50).
4. When a page uses `IconButton` to represent disabled edit/delete actions
(e.g., for
users without write permissions), keyboard users will tab onto these
disabled icons,
encounter non-actionable focus stops, and have to tab past them manually,
which violates
expected disabled-control behavior and clutters keyboard navigation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=952cd77b1cbc49a7950972d208ded8cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=952cd77b1cbc49a7950972d208ded8cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/IconButton.tsx
**Line:** 62:65
**Comment:**
*Incorrect Condition Logic: Disabled icons remain in the tab order
because `tabIndex` is hardcoded to `0` even when `disabled` is true. In list
views this creates many non-actionable keyboard stops and violates disabled
control behavior; set `tabIndex` conditionally (for example `-1` when disabled).
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F32995&comment_hash=01b4cd2fe8ca417533e2614dbbc187febda74d03a1e72af9a6341530a79c99c2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F32995&comment_hash=01b4cd2fe8ca417533e2614dbbc187febda74d03a1e72af9a6341530a79c99c2&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
