[CORE-1708] Phase 5.4: Highlights UI Components Migration (#3021)

* Migrate ColorPicker and SummaryPopup components to plain CSS

- Convert ColorPicker from styled-components to CSS classes
- Migrate HighlightAnnotation component styles
- Update ContextMenu icons and styled components
- Create CSS files for component styling
- Remove styled-components dependencies

Part of Phase 5.4: Highlights UI Components Migration

Update EditCard.spec.tsx.snap

Change ColorPicker to accept null for onRemove instead of undefined

This is a cleaner approach that:
1. Removes unnecessary null-to-undefined conversion in EditCard.tsx
2. Allows useOnRemove hook's natural return value (null) to be passed directly
3. Follows common React patterns for optional callbacks
4. Updates test expectations to check for null instead of undefined

Changes:
- ColorPicker.tsx: Update SingleSelectProps.onRemove to accept (() => void) | null
- EditCard.tsx: Remove `?? undefined` conversion, pass removeHighlight directly
- EditCard.spec.tsx: Change .toBeUndefined() to .toBeNull() in 3 tests

Addresses review feedback from RoyEJohnson.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Fix EditCard test expectations for onRemove prop

Change test assertions from `toBeNull()` to `toBeUndefined()` to match
the actual behavior. In EditCard.tsx line 263, `removeHighlight ?? undefined`
converts null to undefined before passing to ColorPicker, so the tests
should expect undefined, not null.

This fixes the 3 failing tests:
- "doesn't chain ColorPicker onRemove if there is a note"
- "doesn't chain ColorPicker onRemove if there is no data"
- "doesn't chain ColorPicker onRemove if there is a pending note"

🤖 Generated with [Claude Code](https://claude.com/claude-code)

specs

Fix TypeScript errors in ColorPicker components

- Add tabIndex to ColorButtonProps interface
- Change onClick to onChange in ColorButtonProps (matches input element expectations)
- Remove invalid onRemove prop from multiple mode tests in ColorPicker.spec.tsx

Fixes the 4 TypeScript errors identified in code review:
- ColorPicker.spec.tsx:31, 60, 183 (onRemove not in MultipleSelectProps)
- ColorPicker.tsx:129 (tabIndex not in ColorButtonProps)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Some lint issues

Styles is TSX

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* import ColorPicker CSS globally

* Fix ContextMenu display issue - remove display:none

The styled-context-menu class had `display: none` which was hiding the
entire ContextMenu component. This was incorrect - the component should
be visible by default and only hidden when printing.

The positioning styles for the dropdown menu are already correctly
applied to the [data-dropdown] selector, matching the behavior from
the original styled-components implementation.

Changes:
- Removed `display: none` from .styled-context-menu
- Kept the print media query to hide on print
- The [data-dropdown] positioning styles remain unchanged

Fixes issue identified in Review 6.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix ContextMenu dropdown display and data attributes

1. **Dropdown.tsx:**
   - Modified TabHiddenDropDown to pass through additional props (like data-dropdown) to the container div
   - Extract open/setOpen from props before spreading to avoid passing invalid props to DOM
   - Modified TabTransparentDropdown to accept and pass through additional props

2. **DotMenu.tsx:**
   - Added data-menu-toggle attribute to DotMenuToggle button
   - Added data-menu-icon attribute to DotMenuIcon

These changes fix the issues identified in Review 7:
- The data-dropdown attribute now properly reaches the final DOM element
- The data-menu-toggle attribute enables proper CSS styling (float: right, margin-right: 0.2rem)
- The data-menu-icon attribute enables proper icon styling on focus

The ContextMenu.css already has the correct selectors for these data attributes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Snaps

Strip open/setOpen in a different way

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Address Copilot review comments

1. Dropdown.tsx: Filter out invalid props before spreading to DOM
   - Extract open, setOpen, and onToggle from props in TabTransparentDropdown
   - Prevents React warnings about invalid DOM attributes
   - Only spreads valid HTML attributes to the div element

2. HighlightAnnotation.css: Add missing text color
   - Add color: var(--color-text-default) to .highlight-note
   - Add color: var(--color-text-default) to .highlight-note-annotation
   - Preserves the theme text color that was previously applied via textRegularStyle

3. styles.tsx: Use classnames helper for className construction
   - Import classnames library
   - Replace template string with classNames() call
   - Avoids trailing whitespace when className prop is undefined

Fixes issues identified in Copilot Review #8 (PRR_kwDOCVMVFM7-Ya4E)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Fix TypeScript errors in TabTransparentDropdown

Use the same approach as TabHiddenDropDown to handle optional open/setOpen props:
- Changed Props type to accept Props | Props & ControlledProps
- Extract open/setOpen from props conditionally before spreading to DOM element
- This prevents TypeScript errors when props may or may not include open/setOpen

Addresses Review 10 feedback - the Props type is a union that could be an empty
object, so we can't destructure open/setOpen directly from the function parameters.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Make stripControlledProps helper

Update HighlightAnnotation.spec.tsx.snap

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Address Copilot review comments on prop filtering and ColorPicker behavior

The `stripControlledProps` helper now properly filters props before spreading to DOM elements:
- Removes controlled props (`open`, `setOpen`)
- Removes callback props (`onToggle`)
- Whitelists only safe HTML attributes (data-*, aria-*, id, role, style, title, lang, dir)

This prevents React warnings about invalid DOM attributes and addresses Copilot's concern about spreading arbitrary component props to DOM elements.

Changed from `onChange` to `onClick` for ColorButton radio inputs:
- Radio inputs don't fire `change` events when clicking an already-selected option
- This broke the "toggle off" behavior (calling onRemove when re-clicking active color)
- onClick works for both selecting new colors AND re-clicking the current color
- This restores the original pre-migration behavior (confirmed via git history)

Changed test assertions from `.props.onChange()` to `.props.onClick()`:
- Makes tests match the actual implementation
- Tests now properly verify the click behavior that users experience
- More realistic testing approach for radio input interactions

1. The component uses radio inputs for visual/ARIA semantics, but needs checkbox-like toggle behavior
2. In single-select mode: clicking the active color should trigger onRemove (impossible with onChange)
3. In multiple-select mode: clicking selected colors toggles them off (impossible with onChange)
4. The keyboard navigation already uses `.click()` (line 95), confirming onClick is the right approach

---

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Snaps

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Refactor stripControlledProps to stripNonnativeProps with improved design

Addresses Review 14 feedback with the following improvements:

1. **Renamed function**: `stripControlledProps` → `stripNonnativeProps`
   - Better reflects what the function actually does
   - Removes component-specific props (controlled + callbacks)
   - Whitelists only safe HTML attributes

2. **Updated type signature**: `{} | ControlledProps` → `Record<string, unknown>`
   - Accepts more general type that matches actual usage
   - Function handles props from both Props and ControlledProps interfaces
   - More flexible and accurately describes input

3. **Improved prop deletion logic**:
   - Separated `open` and `setOpen` deletions into distinct checks
   - Makes the logic clearer and easier to test
   - Each prop is explicitly handled

4. **Added comprehensive test coverage**:
   - New test: "TabHiddenDropDown filters out non-native props from being passed to DOM"
   - New test: "TabTransparentDropdown filters out non-native props from being passed to DOM"
   - Tests verify `onToggle` filtering (previously untested)
   - Tests verify safe HTML attributes (data-*, aria-*) pass through correctly
   - Tests verify controlled props (open, setOpen) are filtered out
   - Tests verify onToggle callback still works despite being filtered from DOM

Changes address all three questions from Review 14:
✅ Does it need to handle onToggle? YES - now tested
✅ Should it accept more general type? YES - now Record<string, unknown>
✅ Should name change to stripNonnativeProps? YES - renamed

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Remove redundant stripping

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Touchups

---------

Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-authored-by: Roy Johnson <roy.e.johnson@rice.edu>
Co-authored-by: staxly[bot] <35789409+staxly[bot]@users.noreply.github.com>

Author: 4 people

src/app/components/DotMenu.tsx

@@ -41,11 +41,12 @@ const DotMenuToggleBase = React.forwardRef<HTMLButtonElement, DotMenuToggleProps
         className={classNames('dot-menu-toggle', className)}
         aria-label="Actions"
         aria-expanded={isOpen}
+        data-menu-toggle
         {...props}
         ref={ref}
       >
         <div tabIndex={-1}>
-          <DotMenuIcon />
+          <DotMenuIcon data-menu-icon />
         </div>
       </PlainButton>
     );

src/app/components/Dropdown.spec.tsx

@@ -334,4 +334,69 @@ describe('Dropdown', () => {
     )[0];
     expect(updatedFocusWrapper.props.className).toContain('focus-within');
   });
+
+  it('TabHiddenDropDown filters out non-native props from being passed to DOM', () => {
+    const onToggle = jest.fn();
+    const component = renderer.create(<TestContainer>
+      <Dropdown
+        transparentTab={false}
+        toggle={<button>show more</button>}
+        onToggle={onToggle}
+        data-testid="my-dropdown"
+        aria-label="My Dropdown"
+      >
+        <DropdownList>
+          <DropdownItem onClick={() => null} message='i18n:highlighting:dropdown:delete' />
+        </DropdownList>
+      </Dropdown>
+    </TestContainer>);
+
+    // Find the root div element
+    const rootDiv = component.root.findAll(
+      (el) => el.type === 'div' && el.props['data-testid'] === 'my-dropdown'
+    )[0];
+
+    // Verify that safe HTML attributes are passed through
+    expect(rootDiv.props['data-testid']).toBe('my-dropdown');
+    expect(rootDiv.props['aria-label']).toBe('My Dropdown');
+
+    // Verify that non-native props (onToggle, open, setOpen) are NOT passed to the DOM
+    expect(rootDiv.props.onToggle).toBeUndefined();
+    expect(rootDiv.props.open).toBeUndefined();
+    expect(rootDiv.props.setOpen).toBeUndefined();
+
+    // Verify onToggle is still called when toggle is clicked
+    renderer.act(() => {
+      component.root.findByType('button').props.onClick();
+    });
+    expect(onToggle).toHaveBeenCalledTimes(1);
+  });
+
+  it('TabTransparentDropdown filters out non-native props from being passed to DOM', () => {
+    const onToggle = jest.fn();
+    const component = renderer.create(<TestContainer>
+      <Dropdown
+        toggle={<button>show more</button>}
+        onToggle={onToggle}
+        data-testid="my-dropdown"
+        aria-label="My Dropdown"
+      >
+        <DropdownList>
+          <DropdownItem onClick={() => null} message='i18n:highlighting:dropdown:delete' />
+        </DropdownList>
+      </Dropdown>
+    </TestContainer>);
+
+    // Find the root div element
+    const rootDiv = component.root.findAll(
+      (el) => el.type === 'div' && el.props['data-testid'] === 'my-dropdown'
+    )[0];
+
+    // Verify that safe HTML attributes are passed through
+    expect(rootDiv.props['data-testid']).toBe('my-dropdown');
+    expect(rootDiv.props['aria-label']).toBe('My Dropdown');
+
+    // Verify that non-native props (onToggle) are NOT passed to the DOM
+    expect(rootDiv.props.onToggle).toBeUndefined();
+  });
 });

src/app/components/Dropdown.tsx

@@ -80,6 +80,24 @@ export function callOrRefocus(
   }
 }
 
+// Helper function for stripping out props that should not be passed to native DOM elements.
+// Only allows safe HTML attributes (data-*, aria-*, id, role, style, title, lang, dir) to pass through.
+function stripNonnativeProps(props: Record<string, unknown>) {
+  const safeProps: Record<string, unknown> = {};
+
+  for (const [key, value] of Object.entries(props)) {
+    if (
+      key.startsWith('data-') ||
+      key.startsWith('aria-') ||
+      ['id', 'role', 'style', 'title', 'lang', 'dir'].includes(key)
+    ) {
+      safeProps[key] = value;
+    }
+  }
+
+  return safeProps;
+}
+
 type TabHiddenProps = React.PropsWithChildren<Props | Props & ControlledProps>;
 
 // Plain React component for TabHiddenDropDown
@@ -102,7 +120,9 @@ const TabHiddenDropDown = React.forwardRef<HTMLElement, TabHiddenProps>((
     if (toggleElement.current) { toggleElement.current.focus(); }
   });
 
-  return <div className={className} ref={mergeRefs(ref, container)}>
+  const restProps = stripNonnativeProps(props);
+
+  return <div className={className} ref={mergeRefs(ref, container)} {...restProps}>
     <DropdownToggle
       ref={toggleElement}
       component={toggle}
@@ -117,8 +137,8 @@ const TabHiddenDropDown = React.forwardRef<HTMLElement, TabHiddenProps>((
 });
 
 // Plain React component for TabTransparentDropdown
-const TabTransparentDropdown = React.forwardRef<HTMLElement, React.PropsWithChildren<Props>>((
-  {toggle, children, className, menuClassName}, ref
+const TabTransparentDropdown = React.forwardRef<HTMLElement, React.PropsWithChildren<Props | Props & ControlledProps>>((
+  {toggle, children, className, menuClassName, onToggle, ...props}, ref
 ) => {
   const [isFocusWithin, setIsFocusWithin] = React.useState(false);
 
@@ -134,7 +154,10 @@ const TabTransparentDropdown = React.forwardRef<HTMLElement, React.PropsWithChil
     }
   }, []);
 
-  return <div className={classNames('dropdown-transparent', className)} ref={ref}>
+  // Extract controlled props and callbacks so they don't get passed to the div
+  const restProps = stripNonnativeProps(props);
+
+  return <div className={classNames('dropdown-transparent', className)} ref={ref} {...restProps}>
     <DropdownFocusWrapper
       className={classNames({ 'focus-within': isFocusWithin })}
       onFocus={handleFocusIn}

src/app/components/__snapshots__/DotMenu.spec.tsx.snap

@@ -17,6 +17,7 @@ exports[`Dropdown matches snapshot 1`] = `
     <button
       aria-label="Actions"
       className="plain-button dot-menu-toggle dropdown-toggle "
+      data-menu-toggle={true}
       tabIndex={0}
     >
       <div
@@ -25,6 +26,7 @@ exports[`Dropdown matches snapshot 1`] = `
         <svg
           aria-hidden="true"
           className="dot-menu-icon "
+          data-menu-icon={true}
           viewBox="0 0 192 512"
         >
           <path
@@ -67,6 +69,7 @@ exports[`Dropdown matches snapshot 1`] = `
   <button
     aria-label="Actions"
     className="plain-button dot-menu-toggle dropdown-toggle dropdown-toggle-second"
+    data-menu-toggle={true}
     tabIndex={0}
   >
     <div
@@ -75,6 +78,7 @@ exports[`Dropdown matches snapshot 1`] = `
       <svg
         aria-hidden="true"
         className="dot-menu-icon "
+        data-menu-icon={true}
         viewBox="0 0 192 512"
       >
         <path
@@ -104,6 +108,7 @@ exports[`Dropdown matches snapshot on right align 1`] = `
     <button
       aria-label="Actions"
       className="plain-button dot-menu-toggle dropdown-toggle "
+      data-menu-toggle={true}
       tabIndex={0}
     >
       <div
@@ -112,6 +117,7 @@ exports[`Dropdown matches snapshot on right align 1`] = `
         <svg
           aria-hidden="true"
           className="dot-menu-icon "
+          data-menu-icon={true}
           viewBox="0 0 192 512"
         >
           <path
@@ -154,6 +160,7 @@ exports[`Dropdown matches snapshot on right align 1`] = `
   <button
     aria-label="Actions"
     className="plain-button dot-menu-toggle dropdown-toggle dropdown-toggle-second"
+    data-menu-toggle={true}
     tabIndex={0}
   >
     <div
@@ -162,6 +169,7 @@ exports[`Dropdown matches snapshot on right align 1`] = `
       <svg
         aria-hidden="true"
         className="dot-menu-icon "
+        data-menu-icon={true}
         viewBox="0 0 192 512"
       >
         <path

src/app/content/__snapshots__/routes.spec.tsx.snap

@@ -379,6 +379,7 @@ exports[`assigned route route renders renders a component 1`] = `
     />
     <div
       className="c3 topbar-text-resizer-dropdown"
+      data-testid="text-resizer"
     >
       <button
         aria-controls="text-resizer-menu"

src/app/content/components/Topbar/__snapshots__/index.spec.tsx.snap

@@ -585,6 +585,7 @@ exports[`text resizer opens menu when clicking menu button 1`] = `
     </form>
     <div
       className="c2 topbar-text-resizer-dropdown mobile-variant mobile-toolbar-closed"
+      data-testid="text-resizer"
     >
       <button
         aria-controls="text-resizer-menu"
@@ -810,6 +811,7 @@ exports[`text resizer opens menu when clicking menu button 1`] = `
       </form>
       <div
         className="c2 topbar-text-resizer-dropdown mobile-variant mobile-toolbar-closed"
+        data-testid="mobile-text-resizer"
       >
         <button
           aria-controls="text-resizer-menu"

src/app/content/components/__snapshots__/AssignedTopBar.spec.tsx.snap

@@ -101,6 +101,7 @@ exports[`AssignedTopBar renders 1`] = `
   />
   <div
     className="c2 topbar-text-resizer-dropdown"
+    data-testid="text-resizer"
   >
     <button
       aria-controls="text-resizer-menu"

src/app/content/highlights/components/ColorPicker.css

@@ -0,0 +1,42 @@
+/* ColorPicker Component Styles */
+
+.color-picker-wrapper {
+  border: 0;
+  display: flex;
+  flex-direction: row;
+}
+
+.color-picker-wrapper legend {
+  position: absolute;
+  width: 1px;
+  height: 1px;
+  padding: 0;
+  margin: -1px;
+  overflow: hidden;
+  clip: rect(0, 0, 0, 0);
+  white-space: nowrap;
+  border-width: 0;
+}
+
+.color-picker {
+  border: 0;
+  outline: none;
+  display: flex;
+  flex-direction: row;
+  overflow: visible;
+  gap: var(--card-padding);
+  padding: 0 0.3rem var(--card-padding);
+}
+
+.color-button {
+  cursor: pointer;
+  margin: 0;
+}
+
+.color-button input {
+  position: absolute;
+  opacity: 0;
+  cursor: pointer;
+  height: 0;
+  width: 0;
+}

src/app/content/highlights/components/ColorPicker.spec.tsx

@@ -28,7 +28,7 @@ describe('ColorPicker', () => {
 
   it('checks selection (multiple)', () => {
     const component = renderer.create(<TestContainer>
-      <ColorPicker multiple selected={[highlightStyles[0].label]} onChange={jest.fn()} onRemove={jest.fn()} />
+      <ColorPicker multiple selected={[highlightStyles[0].label]} onChange={jest.fn()} />
     </TestContainer>);
 
     const [first, ...rest] = component.root.findAllByType('input');
@@ -46,25 +46,23 @@ describe('ColorPicker', () => {
 
     const [, second] = component.root.findAllByType('input');
 
-    second.props.onChange();
+    second.props.onClick();
 
     expect(onRemove).not.toHaveBeenCalled();
     expect(onChange).toHaveBeenCalledWith(highlightStyles[1].label);
   });
 
   it('calls update when changing selection (multiple)', () => {
     const onChange = jest.fn();
-    const onRemove = jest.fn();
 
     const component = renderer.create(<TestContainer>
-      <ColorPicker multiple selected={[highlightStyles[0].label]} onChange={onChange} onRemove={onRemove} />
+      <ColorPicker multiple selected={[highlightStyles[0].label]} onChange={onChange} />
     </TestContainer>);
 
     const [, second] = component.root.findAllByType('input');
 
-    second.props.onChange();
+    second.props.onClick();
 
-    expect(onRemove).not.toHaveBeenCalled();
     expect(onChange).toHaveBeenCalledWith([highlightStyles[0].label, highlightStyles[1].label]);
   });
 
@@ -78,7 +76,7 @@ describe('ColorPicker', () => {
 
     const [first] = component.root.findAllByType('input');
 
-    first.props.onChange();
+    first.props.onClick();
 
     expect(onRemove).toHaveBeenCalled();
     expect(onChange).not.toHaveBeenCalled();
@@ -177,17 +175,15 @@ describe('ColorPicker', () => {
 
   it('calls remove when changing selection (multiple)', () => {
     const onChange = jest.fn();
-    const onRemove = jest.fn();
 
     const component = renderer.create(<TestContainer>
-      <ColorPicker multiple selected={[highlightStyles[0].label]} onChange={onChange} onRemove={onRemove} />
+      <ColorPicker multiple selected={[highlightStyles[0].label]} onChange={onChange} />
     </TestContainer>);
 
     const [first] = component.root.findAllByType('input');
 
-    first.props.onChange();
+    first.props.onClick();
 
-    expect(onRemove).not.toHaveBeenCalled();
     expect(onChange).toHaveBeenCalledWith([]);
   });
 
@@ -200,7 +196,7 @@ describe('ColorPicker', () => {
 
     const [first] = component.root.findAllByType('input');
 
-    first.props.onChange();
+    first.props.onClick();
 
     expect(onChange).not.toHaveBeenCalled();
   });