Fix cell menu not showing in non-editable dataframes (#10819)

* remove editable condition

* - add test
- improve html semantics

* add changeset

* fix test

* fix test

* - fix test
- fix column widths changing on sort

* swap e2e for story

---------

Co-authored-by: gradio-pr-bot <gradio-pr-bot@users.noreply.github.com>

Author: hannahblair

.changeset/tired-coats-sing.md

@@ -0,0 +1,6 @@
+---
+"@gradio/dataframe": patch
+"gradio": patch
+---
+
+fix:Fix cell menu not showing in non-editable dataframes

js/dataframe/Dataframe.stories.svelte

@@ -675,7 +675,7 @@
 />
 
 <Story
-	name="Dataframe with sorting by multiple columns"
+	name="Non-interactive dataframe with sorting by multiple columns"
 	args={{
 		values: [
 			[1, 2, 3],
@@ -685,7 +685,7 @@
 		headers: ["A", "B", "C"],
 		col_count: [3, "dynamic"],
 		row_count: [3, "dynamic"],
-		editable: true,
+		editable: false,
 		sort_columns: [
 			{ col: 0, direction: "asc" },
 			{ col: 1, direction: "desc" }
@@ -708,7 +708,7 @@
 		const cell_menu_button = canvas.getAllByLabelText("Open cell menu")[0];
 		await userEvent.click(cell_menu_button);
 
-		const sort_ascending_button = canvas.getByRole("button", {
+		const sort_ascending_button = canvas.getByRole("menuitem", {
 			name: "Sort ascending"
 		});
 		await userEvent.click(sort_ascending_button);
@@ -719,7 +719,7 @@
 		const cell_menu_button_2 = canvas.getAllByLabelText("Open cell menu")[1];
 		await userEvent.click(cell_menu_button_2);
 
-		const sort_descending_button = canvas.getByRole("button", {
+		const sort_descending_button = canvas.getByRole("menuitem", {
 			name: "Sort descending"
 		});
 		await userEvent.click(sort_descending_button);
@@ -730,7 +730,7 @@
 		const cell_menu_button_3 = canvas.getAllByLabelText("Open cell menu")[2];
 		await userEvent.click(cell_menu_button_3);
 
-		const sort_ascending_button_3 = canvas.getByRole("button", {
+		const sort_ascending_button_3 = canvas.getByRole("menuitem", {
 			name: "Sort ascending"
 		});
 		await userEvent.click(sort_ascending_button_3);

js/dataframe/shared/CellMenu.svelte

@@ -21,13 +21,14 @@
 	export let on_clear_sort: () => void = () => {};
 	export let sort_direction: SortDirection | null = null;
 	export let sort_priority: number | null = null;
+	export let editable = true;
 
 	export let i18n: I18nFormatter;
 	let menu_element: HTMLDivElement;
 
 	$: is_header = row === -1;
-	$: can_add_rows = row_count[1] === "dynamic";
-	$: can_add_columns = col_count[1] === "dynamic";
+	$: can_add_rows = editable && row_count[1] === "dynamic";
+	$: can_add_columns = editable && col_count[1] === "dynamic";
 
 	onMount(() => {
 		position_menu();
@@ -56,9 +57,10 @@
 	}
 </script>
 
-<div bind:this={menu_element} class="cell-menu">
+<div bind:this={menu_element} class="cell-menu" role="menu">
 	{#if is_header}
 		<button
+			role="menuitem"
 			on:click={() => on_sort("asc")}
 			class:active={sort_direction === "asc"}
 		>
@@ -69,6 +71,7 @@
 			{/if}
 		</button>
 		<button
+			role="menuitem"
 			on:click={() => on_sort("desc")}
 			class:active={sort_direction === "desc"}
 		>
@@ -78,39 +81,65 @@
 				<span class="priority">{sort_priority}</span>
 			{/if}
 		</button>
-		<button on:click={on_clear_sort}>
+		<button role="menuitem" on:click={on_clear_sort}>
 			<CellMenuIcons icon="clear-sort" />
 			{i18n("dataframe.clear_sort")}
 		</button>
 	{/if}
 
 	{#if !is_header && can_add_rows}
-		<button on:click={() => on_add_row_above()}>
+		<button
+			role="menuitem"
+			on:click={() => on_add_row_above()}
+			aria-label="Add row above"
+		>
 			<CellMenuIcons icon="add-row-above" />
 			{i18n("dataframe.add_row_above")}
 		</button>
-		<button on:click={() => on_add_row_below()}>
+		<button
+			role="menuitem"
+			on:click={() => on_add_row_below()}
+			aria-label="Add row below"
+		>
 			<CellMenuIcons icon="add-row-below" />
 			{i18n("dataframe.add_row_below")}
 		</button>
 		{#if can_delete_rows}
-			<button on:click={on_delete_row} class="delete">
+			<button
+				role="menuitem"
+				on:click={on_delete_row}
+				class="delete"
+				aria-label="Delete row"
+			>
 				<CellMenuIcons icon="delete-row" />
 				{i18n("dataframe.delete_row")}
 			</button>
 		{/if}
 	{/if}
 	{#if can_add_columns}
-		<button on:click={() => on_add_column_left()}>
+		<button
+			role="menuitem"
+			on:click={() => on_add_column_left()}
+			aria-label="Add column to the left"
+		>
 			<CellMenuIcons icon="add-column-left" />
 			{i18n("dataframe.add_column_left")}
 		</button>
-		<button on:click={() => on_add_column_right()}>
+		<button
+			role="menuitem"
+			on:click={() => on_add_column_right()}
+			aria-label="Add column to the right"
+		>
 			<CellMenuIcons icon="add-column-right" />
 			{i18n("dataframe.add_column_right")}
 		</button>
 		{#if can_delete_cols}
-			<button on:click={on_delete_col} class="delete">
+			<button
+				role="menuitem"
+				on:click={on_delete_col}
+				class="delete"
+				aria-label="Delete column"
+			>
 				<CellMenuIcons icon="delete-column" />
 				{i18n("dataframe.delete_column")}
 			</button>

js/dataframe/shared/CellMenuButton.svelte

@@ -5,6 +5,7 @@
 <button
 	aria-label="Open cell menu"
 	class="cell-menu-button"
+	aria-haspopup="menu"
 	on:click={on_click}
 	on:touchstart={(event) => {
 		event.preventDefault();

js/dataframe/shared/Table.svelte

@@ -38,8 +38,7 @@
 	import {
 		copy_table_data,
 		get_max,
-		handle_file_upload,
-		sort_table_data
+		handle_file_upload
 	} from "./utils/table_utils";
 	import { make_headers, process_data } from "./utils/data_processing";
 	import { handle_keydown } from "./utils/keyboard_utils";
@@ -48,6 +47,7 @@
 		type DragState,
 		type DragHandlers
 	} from "./utils/drag_utils";
+	import { sort_data_and_preserve_selection } from "./utils/sort_utils";
 
 	export let datatype: Datatype | Datatype[];
 	export let label: string | null = null;
@@ -177,10 +177,23 @@
 
 	$: if (!dequal(values, old_val)) {
 		if (parent) {
-			for (let i = 0; i < 50; i++) {
-				parent.style.removeProperty(`--cell-width-${i}`);
+			// only clear column widths when the data structure changes
+			const is_reset =
+				values.length === 0 || (values.length === 1 && values[0].length === 0);
+			const is_different_structure =
+				old_val !== undefined &&
+				(values.length !== old_val.length ||
+					(values[0] && old_val[0] && values[0].length !== old_val[0].length));
+
+			if (is_reset || is_different_structure) {
+				for (let i = 0; i < 50; i++) {
+					parent.style.removeProperty(`--cell-width-${i}`);
+				}
+				last_width_data_length = 0;
+				last_width_column_count = 0;
 			}
 		}
+
 		// only reset sort state when values are changed
 		const is_reset =
 			values.length === 0 || (values.length === 1 && values[0].length === 0);
@@ -366,12 +379,28 @@
 
 	$: max = get_max(data);
 
-	$: cells[0] && set_cell_widths();
+	// Modify how we trigger cell width calculations
+	// Only recalculate when cells actually change, not during sort
+	$: cells[0] && cells[0]?.clientWidth && set_cell_widths();
 	let cells: HTMLTableCellElement[] = [];
 	let parent: HTMLDivElement;
 	let table: HTMLTableElement;
+	let last_width_data_length = 0;
+	let last_width_column_count = 0;
 
 	function set_cell_widths(): void {
+		const column_count = data[0]?.length || 0;
+		if (
+			last_width_data_length === data.length &&
+			last_width_column_count === column_count &&
+			$df_state.sort_state.sort_columns.length > 0
+		) {
+			return;
+		}
+
+		last_width_data_length = data.length;
+		last_width_column_count = column_count;
+
 		const widths = cells.map((el) => el?.clientWidth || 0);
 		if (widths.length === 0) return;
 
@@ -412,23 +441,17 @@
 		_display_value: string[][] | null,
 		_styling: string[][] | null
 	): void {
-		let id = null;
-		if (selected && selected[0] in _data && selected[1] in _data[selected[0]]) {
-			id = _data[selected[0]][selected[1]].id;
-		}
-
-		sort_table_data(
+		const result = sort_data_and_preserve_selection(
 			_data,
 			_display_value,
 			_styling,
-			$df_state.sort_state.sort_columns
+			$df_state.sort_state.sort_columns,
+			selected,
+			get_current_indices
 		);
-		data = data;
 
-		if (id) {
-			const [i, j] = get_current_indices(id, data);
-			selected = [i, j];
-		}
+		data = result.data;
+		selected = result.selected;
 	}
 
 	$: sort_data(data, display_value, styling);
@@ -968,8 +991,9 @@
 		on_delete_row={() => delete_row_at(active_cell_menu?.row ?? -1)}
 		on_delete_col={() =>
 			delete_col_at(active_cell_menu?.col ?? active_header_menu?.col ?? -1)}
-		can_delete_rows={!active_header_menu && data.length > 1}
-		can_delete_cols={data.length > 0 && data[0]?.length > 1}
+		{editable}
+		can_delete_rows={!active_header_menu && data.length > 1 && editable}
+		can_delete_cols={data.length > 0 && data[0]?.length > 1 && editable}
 		{i18n}
 		on_sort={active_header_menu
 			? (direction) => {

js/dataframe/shared/TableHeader.svelte

@@ -135,7 +135,7 @@
 				<Padlock />
 			{/if}
 		</div>
-		{#if editable && can_add_columns}
+		{#if can_add_columns}
 			<CellMenuButton on_click={(event) => toggle_header_menu(event, i)} />
 		{/if}
 	</div>

js/dataframe/shared/utils/sort_utils.ts

@@ -1,4 +1,5 @@
 import type { Headers } from "../types";
+import { sort_table_data } from "./table_utils";
 
 export type SortDirection = "asc" | "desc";
 
@@ -61,3 +62,30 @@ export function sort_data(
 	}
 	return [...Array(data.length)].map((_, i) => i);
 }
+
+export function sort_data_and_preserve_selection(
+	data: { id: string; value: string | number }[][],
+	display_value: string[][] | null,
+	styling: string[][] | null,
+	sort_columns: { col: number; direction: SortDirection }[],
+	selected: [number, number] | false,
+	get_current_indices: (
+		id: string,
+		data: { id: string; value: string | number }[][]
+	) => [number, number]
+): { data: typeof data; selected: [number, number] | false } {
+	let id = null;
+	if (selected && selected[0] in data && selected[1] in data[selected[0]]) {
+		id = data[selected[0]][selected[1]].id;
+	}
+
+	sort_table_data(data, display_value, styling, sort_columns);
+
+	let new_selected = selected;
+	if (id) {
+		const [i, j] = get_current_indices(id, data);
+		new_selected = [i, j];
+	}
+
+	return { data, selected: new_selected };
+}

js/spa/test/dataframe_events.spec.ts

@@ -1,5 +1,10 @@
 import { test, expect } from "@self/tootils";
 import { Locator } from "@playwright/test";
+import { toBeVisible } from "@testing-library/jest-dom/matchers";
+
+function get_header_cell(element: Locator, col: number) {
+	return element.locator(`th[title='col_${col}']`).nth(1);
+}
 
 // returns a cell in a dataframe by row and column indices
 function get_cell(element: Locator, row: number, col: number) {
@@ -133,7 +138,7 @@ test("Tall dataframe has vertical scrolling", async ({ page }) => {
 	expect(column_count).toBe(4);
 });
 
-test("Tall dataframe updates with buttons", async ({ page }) => {
+test("Dataframe can be cleared and updated indirectly", async ({ page }) => {
 	await page.getByRole("button", { name: "Clear dataframe" }).click();
 	await page.waitForTimeout(500);
 
@@ -152,7 +157,7 @@ test("Tall dataframe updates with buttons", async ({ page }) => {
 		.allTextContents();
 
 	const trimmed_headers = headers.slice(1).map((header) => header.trim());
-	expect(trimmed_headers).toEqual(["A", "B", "C"]);
+	expect(trimmed_headers).toEqual(["A    ⋮", "B    ⋮", "C    ⋮"]);
 });
 
 test("Non-interactive dataframe cannot be edited", async ({ page }) => {