feat: Column organization overflow and undo/redo

packages/components/src/spectrum/SpectrumMenu.scss

@@ -4,10 +4,10 @@
   kbd {
     // Unsetting bootstrap overrides
     padding: unset;
-    font-size: revert;
-    color: revert;
-    background-color: revert;
-    border-radius: revert;
+    font-size: unset;
+    color: unset;
+    background-color: unset;
+    border-radius: unset;
 
     // From Spectrum styles to match the label
     padding-inline-start: var(--spectrum-global-dimension-size-125);

packages/grid/src/GridUtils.ts

@@ -753,13 +753,26 @@ export class GridUtils {
    * @param from The visible index to move from
    * @param to The visible index to move the item to
    * @param oldMovedItems The old reordered items
-   * @returns The new reordered items
+   * @returns The new reordered items. The original array if the operation is a no-op.
    */
-  static moveItem<T extends readonly MoveOperation[]>(
+  static moveItem(
     from: VisibleIndex,
     to: VisibleIndex,
-    oldMovedItems: T
-  ): T {
+    oldMovedItems: MoveOperation[]
+  ): MoveOperation[];
+
+  static moveItem(
+    from: VisibleIndex,
+    to: VisibleIndex,
+    oldMovedItems: readonly MoveOperation[]
+  ): readonly MoveOperation[];
+
+  // The overloads are so we can return the original array if the operation is a no-op
+  static moveItem(
+    from: VisibleIndex,
+    to: VisibleIndex,
+    oldMovedItems: MoveOperation[] | readonly MoveOperation[]
+  ): MoveOperation[] | readonly MoveOperation[] {
     if (from === to) {
       return oldMovedItems;
     }
@@ -787,7 +800,7 @@ export class GridUtils {
       movedItems.push({ from, to });
     }
 
-    return movedItems as unknown as T;
+    return movedItems;
   }
 
   /**
@@ -806,14 +819,29 @@ export class GridUtils {
    *                    E.g. Move range [0, 2] 1 item down (after element 3)
    *                    The move is [0, 2] -> 1 if this is false. [0, 2] -> 3 if this is true
    *                    Both will result in [0, 2] -> 1
-   * @returns The new reordered items
+   * @returns The new reordered items. The original array if the operation is a no-op.
    */
-  static moveRange<T extends readonly MoveOperation[]>(
+  static moveRange(
+    from: BoundedAxisRange,
+    to: VisibleIndex,
+    oldMovedItems: MoveOperation[],
+    isPreMoveTo?: boolean
+  ): MoveOperation[];
+
+  static moveRange(
     from: BoundedAxisRange,
     toParam: VisibleIndex,
-    oldMovedItems: T,
+    oldMovedItems: readonly MoveOperation[],
+    isPreMoveTo?: boolean
+  ): readonly MoveOperation[];
+
+  // The overloads are so we can return the original array if the operation is a no-op
+  static moveRange(
+    from: BoundedAxisRange,
+    toParam: VisibleIndex,
+    oldMovedItems: MoveOperation[] | readonly MoveOperation[],
     isPreMoveTo = false
-  ): T {
+  ): MoveOperation[] | readonly MoveOperation[] {
     if (from[0] === from[1]) {
       return GridUtils.moveItem(from[0], toParam, oldMovedItems);
     }
@@ -856,15 +884,30 @@ export class GridUtils {
       movedItems.pop();
     }
 
-    return movedItems as unknown as T;
+    return movedItems;
   }
 
-  static moveItemOrRange<T extends readonly MoveOperation[]>(
+  static moveItemOrRange(
+    from: VisibleIndex | BoundedAxisRange,
+    to: VisibleIndex,
+    oldMovedItems: MoveOperation[],
+    isPreMoveTo?: boolean
+  ): MoveOperation[];
+
+  static moveItemOrRange(
+    from: VisibleIndex | BoundedAxisRange,
+    to: VisibleIndex,
+    oldMovedItems: readonly MoveOperation[],
+    isPreMoveTo?: boolean
+  ): readonly MoveOperation[];
+
+  // The overloads are so we can return the original array if the operation is a no-op
+  static moveItemOrRange(
     from: VisibleIndex | BoundedAxisRange,
     to: VisibleIndex,
-    oldMovedItems: T,
+    oldMovedItems: MoveOperation[] | readonly MoveOperation[],
     isPreMoveTo = false
-  ): T {
+  ): MoveOperation[] | readonly MoveOperation[] {
     return Array.isArray(from)
       ? GridUtils.moveRange(from, to, oldMovedItems, isPreMoveTo)
       : GridUtils.moveItem(from, to, oldMovedItems);

packages/iris-grid/src/IrisGrid.tsx

@@ -3426,12 +3426,13 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
       {
         // undo/redo will pass already parsed groups
         // Parsing them again causes a loop with undo/redo that makes it unusable
-        columnHeaderGroups:
-          columnHeaderGroups.every(isColumnHeaderGroup) &&
-          columnHeaderGroups.every(group => group.isValid())
-            ? columnHeaderGroups
-            : IrisGridUtils.parseColumnHeaderGroups(model, columnHeaderGroups)
-                .groups,
+        columnHeaderGroups: columnHeaderGroups.every(
+          (group): group is ColumnHeaderGroup =>
+            isColumnHeaderGroup(group) && group.isValid()
+        )
+          ? columnHeaderGroups
+          : IrisGridUtils.parseColumnHeaderGroups(model, columnHeaderGroups)
+              .groups,
       },
       () => this.grid?.forceUpdate()
     );

packages/iris-grid/src/sidebar/visibility-ordering-builder/VisibilityOrderingBuilder.test.tsx

@@ -1410,6 +1410,50 @@ describe('Undo/redo', () => {
     await user.keyboard('{Control>}{Shift>}z{/Control}{/Shift}');
     expect(mockFrozenHandler).toHaveBeenCalledTimes(2);
   });
+
+  it('multiple changes', async () => {
+    const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });
+    const mockMoveHandler = jest.fn();
+    const mockGroupHandler = jest.fn();
+    render(
+      <BuilderWithStateManagement
+        onMovedColumnsChanged={mockMoveHandler}
+        onColumnHeaderGroupChanged={mockGroupHandler}
+      />
+    );
+
+    await clickItem(user, 1);
+    const moveDownBtn = screen.getByLabelText('Move selection down');
+    await user.click(moveDownBtn);
+
+    await selectItems(user, [1, 3]);
+    const createGroupBtn = screen.getByText('Group');
+    await user.click(createGroupBtn);
+
+    await user.type(screen.getByPlaceholderText('Group Name'), 'TestGroup');
+    await user.keyboard('{Enter}');
+
+    mockMoveHandler.mockReset();
+    mockGroupHandler.mockReset();
+    await user.keyboard('{Control>}z{/Control}');
+    expect(mockMoveHandler).toHaveBeenCalledWith(
+      [{ from: 1, to: 2 }],
+      undefined
+    );
+    expect(mockGroupHandler).toHaveBeenCalledWith([]);
+    expect(mockMoveHandler).toHaveBeenCalledTimes(1);
+    expect(mockGroupHandler).toHaveBeenCalledTimes(1);
+
+    await user.keyboard('{Control>}z{/Control}');
+    expect(mockMoveHandler).toHaveBeenCalledWith([], undefined);
+    expect(mockGroupHandler).toHaveBeenCalledWith([]);
+    expect(mockMoveHandler).toHaveBeenCalledTimes(2);
+    expect(mockGroupHandler).toHaveBeenCalledTimes(2);
+
+    mockMoveHandler.mockReset();
+    mockGroupHandler.mockReset();
+    await user.keyboard('{Control>}{Shift>}z{/Control}{/Shift}');
+  });
 });
 
 test('Show hidden columns option', async () => {
@@ -1425,6 +1469,31 @@ test('Show hidden columns option', async () => {
   expect(screen.getAllByText(COLUMN_PREFIX, { exact: false }).length).toBe(5);
 });
 
+test('Maintain focus after group creation and removal', async () => {
+  const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });
+  render(<BuilderWithStateManagement />);
+
+  await selectItems(user, [1]);
+  expect(
+    screen.getByText(`${COLUMN_PREFIX}1`).closest('.tree-item')
+  ).toHaveFocus();
+
+  const createGroupBtn = screen.getByText('Group');
+  await user.click(createGroupBtn);
+
+  await user.type(screen.getByPlaceholderText('Group Name'), 'TestGroup');
+  await user.keyboard('{Enter}');
+
+  expect(screen.getByText('TestGroup').closest('.tree-item')).toHaveFocus();
+
+  await user.click(screen.getByLabelText('Delete group'));
+
+  // Focus goes back to the first item in the deleted group
+  expect(
+    screen.getByText(`${COLUMN_PREFIX}1`).closest('.tree-item')
+  ).toHaveFocus();
+});
+
 test('Sets drag item display string on multi-select', async () => {
   // This is a hacky test and calls the method directly
   // RTL can't simulate drag and drop (in jsdom at least)

packages/iris-grid/src/sidebar/visibility-ordering-builder/VisibilityOrderingBuilder.tsx

@@ -53,6 +53,7 @@ import {
 import clamp from 'lodash.clamp';
 import throttle from 'lodash.throttle';
 import { useUndoRedo } from '@deephaven/react-hooks';
+import Log from '@deephaven/log';
 import './VisibilityOrderingBuilder.scss';
 import { type DisplayColumn } from '../../IrisGridModel';
 import type IrisGridModel from '../../IrisGridModel';
@@ -73,6 +74,8 @@ import {
 } from './VisibilityOrderingBuilderUtils';
 import IrisGridUtils from '../../IrisGridUtils';
 
+const log = Log.module('VisibilityOrderingBuilder');
+
 const DEBOUNCE_SEARCH_COLUMN = 150;
 
 interface IndexRange {
@@ -169,25 +172,70 @@ class VisibilityOrderingBuilderInner extends PureComponent<
     this.list = null;
   }
 
+  componentDidMount(): void {
+    assertNotNull(this.wrapperRef.current);
+    // This fixes focus loss when editing a group because we use the name as the key for rendering.
+    // When the name is changed or group deleted, we lose focus entirely which is indicated by
+    // the focusout relatedTarget being null.
+    this.wrapperRef.current.addEventListener('focusin', (e: FocusEvent) => {
+      if (e.target instanceof HTMLElement) {
+        const treeItem = e.target.closest('.tree-item') as HTMLElement | null;
+        if (treeItem != null) {
+          this.lastFocusedItemIndex =
+            typeof treeItem.dataset.index === 'string'
+              ? parseInt(treeItem.dataset.index, 10)
+              : null;
+        }
+      }
+    });
+  }
+
   componentDidUpdate(prevProps: VisibilityOrderingBuilderInnerProps): void {
     const { movedColumns } = this.props;
     if (movedColumns !== prevProps.movedColumns) {
       const { searchFilter } = this.state;
       this.searchColumns(searchFilter, false);
     }
+
+    // document.activeElement is either body or html when nothing is focused.
+    // If there is no focused element, then we probably deleted or renamed a group
+    // resulting in focus loss. Try to re-establish focus.
+    // Cannot use focusout event for this because it doesn't fire when the focused element is deleted
+    // (except in Chrome which is against the spec here).
+    if (
+      (document.activeElement === document.body ||
+        document.activeElement === document.documentElement) &&
+      this.lastFocusedItemIndex != null
+    ) {
+      const itemToFocus = this.list?.querySelector(
+        `.item-wrapper:nth-child(${this.lastFocusedItemIndex + 1}) .tree-item`
+      );
+
+      if (itemToFocus == null || !(itemToFocus instanceof HTMLElement)) {
+        log.warn('Could not maintain focus');
+        this.lastFocusedItemIndex = null;
+        return;
+      }
+
+      itemToFocus.focus();
+    }
   }
 
   componentWillUnmount(): void {
     this.debouncedSearchColumns.cancel();
 
     const { columnHeaderGroups, onColumnHeaderGroupChanged } = this.props;
     // Clean up unnamed groups on unmount
-    const newGroups = columnHeaderGroups.filter(group => !group.isNew);
-    if (newGroups.length !== columnHeaderGroups.length) {
-      onColumnHeaderGroupChanged(newGroups);
+    const filteredGroups = columnHeaderGroups.filter(group => !group.isNew);
+    if (filteredGroups.length !== columnHeaderGroups.length) {
+      onColumnHeaderGroupChanged(filteredGroups);
     }
   }
 
+  wrapperRef = React.createRef<HTMLDivElement>();
+
+  lastFocusedItemIndex: number | null = null;
+
   list: HTMLDivElement | null;
 
   debouncedSearchColumns = debounce(this.searchColumns, DEBOUNCE_SEARCH_COLUMN);
@@ -877,10 +925,7 @@ class VisibilityOrderingBuilderInner extends PureComponent<
     } = this.props;
 
     const selectedParentItems = this.getSelectedParentItems();
-    const flattenedItems = flattenTree(this.getTreeItems()).map((item, i) => ({
-      ...item,
-      index: i,
-    }));
+    const flattenedItems = flattenTree(this.getTreeItems());
     const firstMovableIndex = this.getFirstMovableIndex();
     const lastMovableIndex = this.getLastMovableIndex();
 
@@ -949,7 +994,8 @@ class VisibilityOrderingBuilderInner extends PureComponent<
   );
 
   handleGroupDelete(group: ColumnHeaderGroup): void {
-    const { columnHeaderGroups, onColumnHeaderGroupChanged } = this.props;
+    const { columnHeaderGroups, endUndoGroup, onColumnHeaderGroupChanged } =
+      this.props;
     const newGroups = columnHeaderGroups.filter(g => g.name !== group.name);
     const parentIndex = newGroups.findIndex(g => g.name === group.parent);
     if (parentIndex >= 0) {
@@ -959,6 +1005,8 @@ class VisibilityOrderingBuilderInner extends PureComponent<
       newGroups.splice(parentIndex, 1, newParent);
     }
     onColumnHeaderGroupChanged(newGroups);
+    // Could be started from editing a new group which is deleted without saving
+    endUndoGroup();
   }
 
   handleGroupCreate(): void {
@@ -1374,6 +1422,7 @@ class VisibilityOrderingBuilderInner extends PureComponent<
     return (
       <div
         role="menu"
+        ref={this.wrapperRef}
         className="visibility-ordering-builder"
         tabIndex={0}
         onKeyUp={this.handleKeyboardShortcut}
@@ -1409,14 +1458,14 @@ class VisibilityOrderingBuilderInner extends PureComponent<
               )}
             >
               <Section aria-label="Undo and Redo">
-                <Item key="undo">
+                <Item key="undo" textValue="Undo">
                   <Icon>
                     <FontAwesomeIcon icon={vsBlank} />
                   </Icon>
                   <Text>Undo</Text>
                   <Keyboard>{GLOBAL_SHORTCUTS.UNDO.getDisplayText()}</Keyboard>
                 </Item>
-                <Item key="redo">
+                <Item key="redo" textValue="Redo">
                   <Icon>
                     <FontAwesomeIcon icon={vsBlank} />
                   </Icon>
@@ -1425,7 +1474,7 @@ class VisibilityOrderingBuilderInner extends PureComponent<
                 </Item>
               </Section>
               <Section aria-label="More actions">
-                <Item key="showHidden">
+                <Item key="showHidden" textValue="Show hidden columns">
                   <Icon>
                     <FontAwesomeIcon
                       icon={showHiddenColumns ? vsCheck : vsBlank}
@@ -1559,7 +1608,6 @@ class VisibilityOrderingBuilderInner extends PureComponent<
   }
 }
 
-// The forwardRef is for a hacky unit test to check the
 // drag and drop display
 const VisibilityOrderingBuilder = memo(
   (props: VisibilityOrderingBuilderProps) => {
@@ -1571,6 +1619,7 @@ const VisibilityOrderingBuilder = memo(
       onColumnVisibilityChanged,
       onColumnHeaderGroupChanged,
       onFrozenColumnsChanged,
+      // Used for unit tests only
       __testRef,
     } = props;