Skip to content

Avoid nesting a button inside another button. #54

@AStevensTaylor

Description

@AStevensTaylor

⚠️ Potential issue | 🟠 Major

Avoid nesting a button inside another button.
The remove button is a descendant of the thumbnail button, which is invalid HTML and can break focus/click behaviour for assistive tech. Use a non-button wrapper with button semantics (or separate the remove control outside the selectable element).

♿ Suggested fix
-	return (
-		<button
-			type="button"
-			className={cn(
-				"relative rounded-md overflow-hidden cursor-pointer border-2 transition-all",
-				"aspect-square min-w-[120px] md:min-w-0",
-				isSelected
-					? "border-primary ring-2 ring-primary/20"
-					: "border-transparent hover:border-muted-foreground/30",
-				"bg-transparent p-0 h-auto appearance-none",
-			)}
-			onClick={onSelect}
-			aria-pressed={isSelected}
-			aria-label={`Image ${image.file.name}`}
-		>
+	return (
+		<div
+			role="button"
+			tabIndex={0}
+			className={cn(
+				"relative rounded-md overflow-hidden cursor-pointer border-2 transition-all",
+				"aspect-square min-w-[120px] md:min-w-0",
+				isSelected
+					? "border-primary ring-2 ring-primary/20"
+					: "border-transparent hover:border-muted-foreground/30",
+				"bg-transparent p-0 h-auto appearance-none",
+			)}
+			onClick={onSelect}
+			onKeyDown={(e) => {
+				if (e.key === "Enter" || e.key === " ") {
+					e.preventDefault();
+					onSelect();
+				}
+			}}
+			aria-pressed={isSelected}
+			aria-label={`Image ${image.file.name}`}
+		>
 			<img
 				src={image.url}
 				alt={image.file.name}
 				className="w-full h-full object-cover"
 			/>
 			<button
 				type="button"
 				aria-label={`Remove image ${image.file.name}`}
 				onClick={(e) => {
 					e.stopPropagation();
 					onRemove();
 				}}
 				className="absolute top-1 right-1 p-1 rounded-full bg-black/50 hover:bg-black/70 text-white transition-colors"
 			>
 				<X className="size-3" />
 			</button>
 			{dimensions && (
 				<div className="absolute bottom-0 left-0 right-0 bg-black/60 text-white text-xs px-1.5 py-1 text-center">
 					{dimensions.width}×{dimensions.height} •{" "}
 					{getAspectRatio(dimensions.width, dimensions.height)}
 				</div>
 			)}
-		</button>
+		</div>
 	);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

		<div
			role="button"
			tabIndex={0}
			className={cn(
				"relative rounded-md overflow-hidden cursor-pointer border-2 transition-all",
				"aspect-square min-w-[120px] md:min-w-0",
				isSelected
					? "border-primary ring-2 ring-primary/20"
					: "border-transparent hover:border-muted-foreground/30",
				"bg-transparent p-0 h-auto appearance-none",
			)}
			onClick={onSelect}
			onKeyDown={(e) => {
				if (e.key === "Enter" || e.key === " ") {
					e.preventDefault();
					onSelect();
				}
			}}
			aria-pressed={isSelected}
			aria-label={`Image ${image.file.name}`}
		>
			<img
				src={image.url}
				alt={image.file.name}
				className="w-full h-full object-cover"
			/>
			<button
				type="button"
				aria-label={`Remove image ${image.file.name}`}
				onClick={(e) => {
					e.stopPropagation();
					onRemove();
				}}
				className="absolute top-1 right-1 p-1 rounded-full bg-black/50 hover:bg-black/70 text-white transition-colors"
			>
				<X className="size-3" />
			</button>
			{dimensions && (
				<div className="absolute bottom-0 left-0 right-0 bg-black/60 text-white text-xs px-1.5 py-1 text-center">
					{dimensions.width}×{dimensions.height} •{" "}
					{getAspectRatio(dimensions.width, dimensions.height)}
				</div>
			)}
		</div>
🤖 Prompt for AI Agents
In `@src/components/ImageGallery.tsx` around lines 95 - 124, The thumbnail button
in ImageGallery.tsx currently nests the remove <button> (inside the outer
selectable button handling onSelect/isSelected), which is invalid HTML; move the
remove control out of the clickable thumbnail element or replace the inner
<button> with a non-button element that exposes button semantics. Specifically,
keep the outer selectable element (the button with onSelect, isSelected,
aria-pressed) and change the inner control to a focusable element (e.g., a <div>
or <span>) with role="button", tabIndex={0}, aria-label matching the current
string, onClick that calls stopPropagation() and onRemove(), and keyboard
handlers (onKeyDown handling Enter/Space to invoke onRemove) so the existing X
icon and stopPropagation behavior remain accessible and do not interfere with
the thumbnail's focus/click behavior.

Originally posted by @coderabbitai[bot] in #44 (comment)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions