Repository: deephaven/deephaven-plugins (language: python)

Issue:
Text strings in Spectrum `Item` components are not implicitly wrapped in `Text` components
<!-- Please read our Code of Conduct: https://github.com/deephaven/deephaven-core/blob/main/CODE_OF_CONDUCT.md -->
<!-- Please search existing issues to avoid creating duplicates. -->
<!-- Use clear and concise text to keep the issue brief and to the point. -->

**Description**
Previously if you passed in a text string as a child to a Spectrum `Item` component, we would implicitly wrap it using a `Text` Spectrum component: https://github.com/deephaven/deephaven-plugins/blob/cea544ba86de83f15106f23fc9c170f5fa863742/plugins/ui/src/js/src/elements/spectrum/mapSpectrumChildren.tsx#L5

However that only gets called from `SpectrumElementView`, which is only triggered when a component is mapped from the `SpectrumElementUtils`. However with the UI Picker changes, now it's just treated like any other component: https://github.com/deephaven/deephaven-plugins/pull/333/files#diff-bf6fab0a489dcf0835b1be8cb5af8265982c19e7d1b853257db43f5d9283d30d

We should be implicitly wrapping raw text/numbers passed in to a `ui.item` with a `Text` component.

**Steps to reproduce**
1. Run the following snippet showing tabs with/without the `ui.text` around the text in `ui.item`:
```python
from deephaven import ui

@ui.component
def bad_table_tabs():
    return ui.tabs(
        ui.tab_list(
            ui.item(ui.icon("vsGithubAlt"), "CAT", key="CAT"),
            ui.item("DOG", key="DOG"),
        ),
    )

@ui.component
def good_table_tabs():
    return ui.tabs(
        ui.tab_list(
            ui.item(ui.icon("vsGithubAlt"), ui.text("CAT"), key="CAT"),
            ui.item(ui.text("DOG"), key="DOG"),
        ),
    )


bad_result = bad_table_tabs()
good_result = good_table_tabs()
```

**Expected results**
1. The two panels that appear should be identical

**Actual results**
1. The tabs with the explicit wrapping (`good_result`) appear correct, but `bad_result` has the icon above the word "CAT". See screenshot below.

**Additional details and attachments**

![image](https://github.com/deephaven/deephaven-plugins/assets/4505624/6f0581bc-d22c-41e1-bd85-955c98e187f3)


**Versions**

<!-- Version information. These versions can be copied from the Settings panel in the Deephaven UI. -->
Engine Version: 0.34.0-SNAPSHOT
Web UI Version: 0.68.0
Java Version: 11.0.22
Barrage Version: 0.6.0
Browser Name: Chrome 122
OS Name: Linux
deephaven.ui: 0.9.0



Discussion / hints:

Fix patch:
diff --git a/plugins/ui/src/js/src/elements/ElementUtils.ts b/plugins/ui/src/js/src/elements/ElementUtils.ts
deleted file mode 100644
index 218e6e4c0..000000000
--- a/plugins/ui/src/js/src/elements/ElementUtils.ts
+++ /dev/null
@@ -1,76 +0,0 @@
-import type { WidgetExportedObject } from '@deephaven/jsapi-types';
-
-export const CALLABLE_KEY = '__dhCbid';
-export const OBJECT_KEY = '__dhObid';
-export const ELEMENT_KEY = '__dhElemName';
-
-export type CallableNode = {
-  /** The name of the callable to call */
-  [CALLABLE_KEY]: string;
-};
-
-export type ObjectNode = {
-  /** The index of the object in the exported objects array */
-  [OBJECT_KEY]: number;
-};
-
-/**
- * Describes an element that can be rendered in the UI.
- * Extend this type with stricter rules on the element key type to provide types.
- * See `SpectrumElementNode` for an example.
- */
-export type ElementNode<
-  K extends string = string,
-  P extends Record<string, unknown> = Record<string, unknown>
-> = {
-  /**
-   * The type of this element. Can be something like `deephaven.ui.components.Panel`, or
-   * a custom component type defined by the user in their plugin.
-   */
-  [ELEMENT_KEY]: K;
-  props?: P;
-};
-
-export type ElementNodeWithChildren<
-  K extends string = string,
-  P extends Record<string, unknown> = Record<string, unknown>
-> = ElementNode<K, P> & {
-  props: React.PropsWithChildren<P>;
-};
-
-export function isObjectNode(obj: unknown): obj is ObjectNode {
-  return obj != null && typeof obj === 'object' && OBJECT_KEY in obj;
-}
-
-export function isElementNode(obj: unknown): obj is ElementNode {
-  return obj != null && typeof obj === 'object' && ELEMENT_KEY in obj;
-}
-
-export function isCallableNode(obj: unknown): obj is CallableNode {
-  return obj != null && typeof obj === 'object' && CALLABLE_KEY in obj;
-}
-
-export function isExportedObject(obj: unknown): obj is WidgetExportedObject {
-  return (
-    obj != null &&
-    typeof obj === 'object' &&
-    typeof (obj as WidgetExportedObject).fetch === 'function' &&
-    typeof (obj as WidgetExportedObject).type === 'string'
-  );
-}
-
-/**
- * Gets the type of an element object, or `unknown` if it is not an element.
- * @param node A node in a document, element or unknown object.
- * @returns The type of the element
- */
-export function getElementType(node: unknown): string {
-  return isElementNode(node) ? node[ELEMENT_KEY] : 'unknown';
-}
-
-export function getElementKey(node: unknown, defaultKey: string): string {
-  if (!isElementNode(node) || node.props?.key == null) {
-    return defaultKey;
-  }
-  return `${node.props?.key}`;
-}
diff --git a/plugins/ui/src/js/src/elements/ElementUtils.tsx b/plugins/ui/src/js/src/elements/ElementUtils.tsx
new file mode 100644
index 000000000..009a13331
--- /dev/null
+++ b/plugins/ui/src/js/src/elements/ElementUtils.tsx
@@ -0,0 +1,197 @@
+import React from 'react';
+import { Text } from '@adobe/react-spectrum';
+import type { WidgetExportedObject } from '@deephaven/jsapi-types';
+import { ITEM_ELEMENT_NAME } from './ElementConstants';
+import ObjectView from './ObjectView';
+
+export const CALLABLE_KEY = '__dhCbid';
+export const OBJECT_KEY = '__dhObid';
+export const ELEMENT_KEY = '__dhElemName';
+
+export type CallableNode = {
+  /** The name of the callable to call */
+  [CALLABLE_KEY]: string;
+};
+
+export type ObjectNode = {
+  /** The index of the object in the exported objects array */
+  [OBJECT_KEY]: number;
+};
+
+/**
+ * Describes an element that can be rendered in the UI.
+ * Extend this type with stricter rules on the element key type to provide types.
+ * See `SpectrumElementNode` for an example.
+ */
+export type ElementNode<
+  K extends string = string,
+  P extends Record<string, unknown> = Record<string, unknown>
+> = {
+  /**
+   * The type of this element. Can be something like `deephaven.ui.components.Panel`, or
+   * a custom component type defined by the user in their plugin.
+   */
+  [ELEMENT_KEY]: K;
+  props?: P;
+};
+
+export type ElementNodeWithChildren<
+  K extends string = string,
+  P extends Record<string, unknown> = Record<string, unknown>
+> = ElementNode<K, P> & {
+  props: React.PropsWithChildren<P>;
+};
+
+export function isObjectNode(obj: unknown): obj is ObjectNode {
+  return obj != null && typeof obj === 'object' && OBJECT_KEY in obj;
+}
+
+/**
+ * Type guard for `ElementNode` objects. If `name` is provided, it will also check
+ * that the element name matches the provided name.
+ * @param obj The object to check
+ * @param name Optional name to check
+ * @returns `true` if the object matches the `ElementNode` type and optionally
+ * a given element name
+ */
+export function isElementNode(obj: unknown, name?: string): obj is ElementNode {
+  const isElement =
+    obj != null && typeof obj === 'object' && ELEMENT_KEY in obj;
+
+  if (name == null) {
+    return isElement;
+  }
+
+  return isElement && obj[ELEMENT_KEY] === name;
+}
+
+export function isCallableNode(obj: unknown): obj is CallableNode {
+  return obj != null && typeof obj === 'object' && CALLABLE_KEY in obj;
+}
+
+export function isExportedObject(obj: unknown): obj is WidgetExportedObject {
+  return (
+    obj != null &&
+    typeof obj === 'object' &&
+    typeof (obj as WidgetExportedObject).fetch === 'function' &&
+    typeof (obj as WidgetExportedObject).type === 'string'
+  );
+}
+
+/**
+ * Typeguard for primitive values.
+ * @param value The value to check
+ * @returns True if given value is a string, number, or boolean
+ */
+export function isPrimitive(
+  value: unknown
+): value is string | number | boolean {
+  return (
+    typeof value === 'string' ||
+    typeof value === 'number' ||
+    typeof value === 'boolean'
+  );
+}
+
+/**
+ * Gets the type of an element object, or `unknown` if it is not an element.
+ * @param node A node in a document, element or unknown object.
+ * @returns The type of the element
+ */
+export function getElementType(node: unknown): string {
+  return isElementNode(node) ? node[ELEMENT_KEY] : 'unknown';
+}
+
+export function getElementKey(node: unknown, defaultKey: string): string {
+  if (!isElementNode(node) || node.props?.key == null) {
+    return defaultKey;
+  }
+  return `${node.props?.key}`;
+}
+
+export function mapItemOrArray<T>(itemOrArray: T, mapItem: (item: T) => T): T;
+export function mapItemOrArray<T>(
+  itemOrArray: T[],
+  mapItem: (item: T) => T
+): T[];
+export function mapItemOrArray<T>(
+  itemOrArray: T | T[],
+  mapItem: (item: T) => T
+): T | T[] {
+  if (Array.isArray(itemOrArray)) {
+    return itemOrArray.map(mapItem);
+  }
+
+  return mapItem(itemOrArray);
+}
+
+/**
+ * Wrap certain children of an element node with the appropriate React components.
+ * 1. Exported objects are wrapped with `ObjectView`
+ * 2. Primitive children of `Item` elements are wrapped with `Text`, and the
+ * `textValue` prop of `Item` element is set if it doesn't exist and the
+ * original had a single primitive child
+ * @param element The element node to wrap children for
+ * @returns A new element node with wrapped children or the original if no
+ * children are present
+ */
+export function wrapElementChildren(element: ElementNode): ElementNode {
+  if (element.props?.children == null) {
+    return element;
+  }
+
+  const newProps = { ...element.props };
+
+  const isItemElement = isElementNode(element, ITEM_ELEMENT_NAME);
+
+  // We will be wrapping all primitive `Item` children in a `Text` element to
+  // ensure proper layout. Since `Item` components require a `textValue` prop
+  // if they don't contain exactly 1 `string` child, this will trigger a11y
+  // warnings. We can set a default `textValue` prop in cases where the
+  // original had a single primitive child.
+  if (isItemElement) {
+    if (!('textValue' in newProps) && isPrimitive(newProps.children)) {
+      newProps.textValue = String(newProps.children);
+    }
+
+    if (!('key' in newProps)) {
+      newProps.key = newProps.textValue;
+    }
+  }
+
+  // Derive child keys based on type + index of the occurrence of the type
+  const typeMap = new Map<string, number>();
+  const getChildKey = (type: string): string => {
+    const typeCount = typeMap.get(type) ?? 0;
+    typeMap.set(type, typeCount + 1);
+    return `${type}-${typeCount}`;
+  };
+
+  const children = Array.isArray(newProps.children)
+    ? newProps.children
+    : [newProps.children];
+
+  const wrappedChildren = children.map(child => {
+    // Exported objects need to be converted to `ObjectView` to be rendered
+    if (isExportedObject(child)) {
+      return <ObjectView key={getChildKey(child.type)} object={child} />;
+    }
+
+    // Auto wrap primitive children of `Item` elements in `Text` elements
+    if (isItemElement && isPrimitive(child)) {
+      return <Text key={String(child)}>{String(child)}</Text>;
+    }
+
+    return child;
+  });
+
+  // Keep the children as an array or single item based on the original value
+  newProps.children = Array.isArray(newProps.children)
+    ? wrappedChildren
+    : wrappedChildren[0];
+
+  return {
+    ...element,
+    props: newProps,
+  };
+}
diff --git a/plugins/ui/src/js/src/widget/WidgetUtils.tsx b/plugins/ui/src/js/src/widget/WidgetUtils.tsx
index cc404b52a..c04153978 100644
--- a/plugins/ui/src/js/src/widget/WidgetUtils.tsx
+++ b/plugins/ui/src/js/src/widget/WidgetUtils.tsx
@@ -8,7 +8,7 @@ import {
   ElementNode,
   ELEMENT_KEY,
   isElementNode,
-  isExportedObject,
+  wrapElementChildren,
 } from '../elements/ElementUtils';
 import HTMLElementView from '../elements/HTMLElementView';
 import { isHTMLElementNode } from '../elements/HTMLElementUtils';
@@ -30,7 +30,6 @@ import {
   UITABLE_ELEMENT_TYPE,
 } from '../elements/ElementConstants';
 import ReactPanel from '../layout/ReactPanel';
-import ObjectView from '../elements/ObjectView';
 import Row from '../layout/Row';
 import Stack from '../layout/Stack';
 import Column from '../layout/Column';
@@ -62,27 +61,8 @@ export function getComponentTypeForElement<P extends Record<string, unknown>>(
 }
 
 export function getComponentForElement(element: ElementNode): React.ReactNode {
-  // Need to convert the children of the element if they are exported objects to an ObjectView
-  // Else React won't be able to render them
-  const newElement = { ...element };
-  if (newElement.props?.children != null) {
-    const { children } = newElement.props;
-    if (Array.isArray(children)) {
-      const typeMap = new Map<string, number>();
-      newElement.props.children = children.map((child, i) => {
-        if (isExportedObject(child)) {
-          const { type } = child;
-          const typeCount = typeMap.get(type) ?? 0;
-          typeMap.set(type, typeCount + 1);
-          const key = `${type}-${typeCount}`;
-          return <ObjectView key={key} object={child} />;
-        }
-        return child;
-      });
-    } else if (isExportedObject(children)) {
-      newElement.props.children = <ObjectView object={children} />;
-    }
-  }
+  const newElement = wrapElementChildren({ ...element });
+
   if (isHTMLElementNode(newElement)) {
     return HTMLElementView({ element: newElement });
   }


Test patch:
diff --git a/plugins/ui/src/js/src/elements/ElementUtils.test.tsx b/plugins/ui/src/js/src/elements/ElementUtils.test.tsx
new file mode 100644
index 000000000..14223a112
--- /dev/null
+++ b/plugins/ui/src/js/src/elements/ElementUtils.test.tsx
@@ -0,0 +1,131 @@
+import React from 'react';
+import { Text } from '@adobe/react-spectrum';
+import type { WidgetExportedObject } from '@deephaven/jsapi-types';
+import { TestUtils } from '@deephaven/utils';
+import { ELEMENT_KEY, isPrimitive, wrapElementChildren } from './ElementUtils';
+import ObjectView from './ObjectView';
+import { ITEM_ELEMENT_NAME } from './ElementConstants';
+
+const { createMockProxy } = TestUtils;
+
+describe('isPrimitive', () => {
+  it.each(['test', 444, true, false])(
+    'should return true primitive values: %s',
+    value => {
+      const actual = isPrimitive(value);
+      expect(actual).toBe(true);
+    }
+  );
+
+  it('should return false for non-primitive values', () => {
+    const actual = isPrimitive({});
+    expect(actual).toBe(false);
+  });
+});
+
+describe('wrapElementChildren', () => {
+  const mock = {
+    exportedA1: createMockProxy<WidgetExportedObject>({
+      type: 'mock.exported.a',
+    }),
+    exportedA2: createMockProxy<WidgetExportedObject>({
+      type: 'mock.exported.a',
+    }),
+    exportedB1: createMockProxy<WidgetExportedObject>({
+      type: 'mock.exported.b',
+    }),
+  };
+
+  it('should return given element if it has no children', () => {
+    const actual = wrapElementChildren({ [ELEMENT_KEY]: 'mock.element' });
+    expect(actual).toBe(actual);
+  });
+
+  it.each([
+    [
+      mock.exportedA1,
+      <ObjectView key={`${mock.exportedA1.type}-0`} object={mock.exportedA1} />,
+    ],
+    [
+      [mock.exportedA1, mock.exportedA2, mock.exportedB1],
+      [
+        <ObjectView
+          key={`${mock.exportedA1.type}-0`}
+          object={mock.exportedA1}
+        />,
+        <ObjectView
+          key={`${mock.exportedA1.type}-1`}
+          object={mock.exportedA1}
+        />,
+        <ObjectView
+          key={`${mock.exportedB1.type}-0`}
+          object={mock.exportedB1}
+        />,
+      ],
+    ],
+  ])(
+    'should wrap exported object children in ObjectView: %s, %s',
+    (children, expectedChildren) => {
+      const actual = wrapElementChildren({
+        [ELEMENT_KEY]: 'mock.element',
+        props: { children },
+      });
+
+      expect(actual.props?.children).toEqual(expectedChildren);
+    }
+  );
+
+  describe.each([
+    ['Some text value', undefined],
+    [undefined, 'itemKey'],
+  ])('Item element `textValue`:`%s`, `key`:`%s`', (textValue, itemKey) => {
+    it.each([
+      ['String', <Text key="String">String</Text>],
+      [999, <Text key="999">999</Text>],
+      [true, <Text key="true">true</Text>],
+      [false, <Text key="false">false</Text>],
+      [
+        ['String', 999, true, false],
+        [
+          <Text key="String">String</Text>,
+          <Text key="999">999</Text>,
+          <Text key="true">true</Text>,
+          <Text key="false">false</Text>,
+        ],
+      ],
+    ])(
+      'should wrap primitive item element children in Text elements: %s, %s',
+      (children, expectedChildren) => {
+        const givenProps: Record<string, unknown> = { children };
+        if (textValue != null) {
+          givenProps.textValue = textValue;
+        }
+
+        if (itemKey != null) {
+          givenProps.key = itemKey;
+        }
+
+        const expectedTextValue =
+          textValue == null && isPrimitive(children)
+            ? String(children)
+            : textValue;
+
+        const expected = {
+          [ELEMENT_KEY]: ITEM_ELEMENT_NAME,
+          props: {
+            key: itemKey ?? textValue,
+            textValue: expectedTextValue,
+            children: expectedChildren,
+          },
+        };
+
+        const actual = wrapElementChildren({
+          [ELEMENT_KEY]: ITEM_ELEMENT_NAME,
+          props: givenProps,
+        });
+
+        expect(actual).toEqual(expected);
+      }
+    );
+  });
+});
diff --git a/plugins/ui/src/js/src/widget/WidgetUtils.test.tsx b/plugins/ui/src/js/src/widget/WidgetUtils.test.tsx
index 4d5ffdbd4..7266a1cfc 100644
--- a/plugins/ui/src/js/src/widget/WidgetUtils.test.tsx
+++ b/plugins/ui/src/js/src/widget/WidgetUtils.test.tsx
@@ -1,10 +1,12 @@
 import React from 'react';
+import { Text } from '@adobe/react-spectrum';
 import {
   FRAGMENT_ELEMENT_NAME,
+  ITEM_ELEMENT_NAME,
   HTML_ELEMENT_NAME_PREFIX,
   ICON_ELEMENT_TYPE_PREFIX,
 } from '../elements/ElementConstants';
-import { ELEMENT_KEY } from '../elements/ElementUtils';
+import { ElementNode, ELEMENT_KEY } from '../elements/ElementUtils';
 import HTMLElementView from '../elements/HTMLElementView';
 import IconElementView from '../elements/IconElementView';
 import { SPECTRUM_ELEMENT_TYPE_PREFIX } from '../elements/SpectrumElementUtils';
@@ -46,17 +48,21 @@ describe('getComponentForElement', () => {
 
   it.each(
     Object.keys(elementComponentMap) as (keyof typeof elementComponentMap)[]
-  )('should spread props for element nodes', elementKey => {
-    const element = {
-      [ELEMENT_KEY]: elementKey,
-      props:
-        elementKey === FRAGMENT_ELEMENT_NAME
-          ? { key: 'mock.key', children: ['Some child'] }
-          : {
-              'data-test': 'mock.value',
-              children: ['Some child'],
-            },
-    };
+  )('should spread props for element nodes: %s', elementKey => {
+    let element: ElementNode = { [ELEMENT_KEY]: elementKey };
+
+    if (elementKey === FRAGMENT_ELEMENT_NAME) {
+      element = {
+        ...element,
+        props: { key: 'mock.key', children: ['Some child'] },
+      };
+    } else if (elementKey === ITEM_ELEMENT_NAME) {
+      element = {
+        ...element,
+        props: { children: <Text>Some child</Text> },
+      };
+    }
+
     const actual = getComponentForElement(element);
 
     const Expected = elementComponentMap[elementKey] as (