Add radio role and aria checked state to color checkboxes (#2331)

* Add radio role and aria checked state to color checkboxes [DISCO-323] Leaving the type as checkbox retained the functionality. Adding the role takes care of the accessibility. * Add radiogroup role to fieldset * Add trash can icon to color picker * Only show trashcan when it will do something * Separate trash button; fix tests * Hide colorpicker legends; remove space instruction * No fieldset grouping color indicators and trash icon --------- Co-authored-by: staxly[bot] <35789409+staxly[bot]@users.noreply.github.com>
openstax · Feb 14, 2025 · 1d84445 · 1d84445
1 parent 8f4990a
commit 1d84445
Show file tree

Hide file tree

Showing 12 changed files with 764 additions and 648 deletions.
diff --git a/src/app/content/highlights/components/ColorIndicator.tsx b/src/app/content/highlights/components/ColorIndicator.tsx
@@ -4,6 +4,8 @@ import { Check } from 'styled-icons/fa-solid/Check';
 import { isDefined } from '../../../guards';
 import { highlightStyles } from '../../constants';
 import { defaultFocusOutline } from '../../../theme';
+import { useIntl } from 'react-intl';
+import trashIcon from '../../../../assets/trash-347.svg';
 
 interface StyleProps {
   style: typeof highlightStyles[number];
@@ -114,4 +116,31 @@ const ColorIndicator = styled(Hoc)`
   }
 `;
 
+function TB({
+  onClick,
+  className,
+}: {
+  onClick: React.MouseEventHandler<HTMLButtonElement> | undefined;
+  className: string;
+}) {
+  return (
+    <button
+      type='button'
+      className={className}
+      aria-label={useIntl().formatMessage({id: 'i18n:a11y:keyboard-shortcuts:deselect-highlight-color'})}
+      onClick={onClick}
+    >
+      <img src={trashIcon} alt='' />
+    </button>
+  );
+}
+
+// tslint:disable-next-line:variable-name
+export const TrashButton = styled(TB)`
+  img {
+    height: ${(props: Props) => indicatorSize(props) - 0.5}rem;
+    width: ${(props: Props) => indicatorSize(props) - 0.5}rem;
+  }
+`;
+
 export default ColorIndicator;
diff --git a/src/app/content/highlights/components/ColorPicker.spec.tsx b/src/app/content/highlights/components/ColorPicker.spec.tsx
@@ -4,7 +4,7 @@ import { renderToDom, dispatchKeyDownEvent } from '../../../../test/reactutils';
 import TestContainer from '../../../../test/TestContainer';
 import { highlightStyles } from '../../constants';
 import ColorPicker from './ColorPicker';
-import { HTMLElement, HTMLFieldSetElement, HTMLInputElement } from '@openstax/types/lib.dom';
+import { HTMLElement, HTMLFieldSetElement, HTMLInputElement, HTMLButtonElement } from '@openstax/types/lib.dom';
 import { assertDocument } from '../../../utils';
 
 describe('ColorPicker', () => {
@@ -84,6 +84,27 @@ describe('ColorPicker', () => {
     expect(onChange).not.toHaveBeenCalled();
   });
 
+  it('calls remove when trashcan is clicked', () => {
+    const onChange = jest.fn();
+    const onRemove = jest.fn();
+
+    const {root} = renderToDom(<TestContainer>
+      <ColorPicker color={highlightStyles[0].label} onChange={onChange} onRemove={onRemove} />
+    </TestContainer>);
+
+    const button = root.querySelector('button') as HTMLButtonElement;
+
+    // This should trigger the button, but does not in testing
+    dispatchKeyDownEvent({
+      element: button as HTMLElement,
+      key: 'Enter',
+    });
+
+    button.click();
+    expect(onRemove).toHaveBeenCalled();
+    expect(onChange).not.toHaveBeenCalled();
+  });
+
   it('operates as a radiogroup', () => {
     const onChange = jest.fn();
     const onRemove = jest.fn();

diff --git a/src/app/content/highlights/components/ColorPicker.tsx b/src/app/content/highlights/components/ColorPicker.tsx
@@ -1,11 +1,12 @@
 import { HighlightColorEnum } from '@openstax/highlighter/dist/api';
 import React from 'react';
 import { useIntl } from 'react-intl';
+import { hiddenButAccessible } from '../../../theme';
 import styled from 'styled-components/macro';
 import { match, not } from '../../../fpUtils';
 import { highlightStyles } from '../../constants';
 import { cardPadding } from '../constants';
-import ColorIndicator from './ColorIndicator';
+import ColorIndicator, { TrashButton } from './ColorIndicator';
 import { HTMLDivElement, HTMLInputElement } from '@openstax/types/lib.dom';
 
 interface SingleSelectProps {
@@ -48,7 +49,7 @@ const ColorButton = styled(({className, size, style, ...props}: ColorButtonProps
     component={<label />}
     className={className}
   >
-    <input type='checkbox' {...props} />
+    <input type='radio' aria-checked={props.checked} {...props} />
   </ColorIndicator>;
 })`
   cursor: pointer;
@@ -82,6 +83,17 @@ function nextIdx(idx: number, itemCount: number, key: NavKeys) {
   return idx;
 }
 
+// tslint:disable-next-line:variable-name
+const FSWrapper = styled.div`
+  border: 0;
+  display: flex;
+  flex-direction: row;
+
+  legend {
+    ${hiddenButAccessible}
+  }
+`;
+
 // tslint:disable-next-line:variable-name
 const ColorPicker = ({className, ...props}: Props) => {
   const ref = React.useRef<HTMLDivElement>(null);
@@ -113,33 +125,43 @@ const ColorPicker = ({className, ...props}: Props) => {
     },
     [color]
   );
+  const hasOnRemove = 'onRemove' in props && props.onRemove;
 
   React.useEffect(focusOnSelected, [focusOnSelected]);
 
   return (
-    <fieldset
-      className={className}
-      tabIndex={0}
-      ref={ref}
-      onKeyDown={handleKeyNavigation}
-      onFocus={focusOnSelected}
-    >
-      <legend>Choose highlight color</legend>
-      {highlightStyles.map((style) => <ColorButton key={style.label}
-        name={style.label}
-        checked={props.multiple ? props.selected.includes(style.label) : props.color === style.label}
-        style={style}
-        size={props.size}
-        tabIndex={-1}
-        onChange={() => props.multiple
-          ? props.selected.includes(style.label)
-            ? props.onChange(props.selected.filter(not(match(style.label))))
-            : props.onChange([...props.selected, style.label])
-          : props.color === style.label
-            ? props.onRemove ? props.onRemove() : null
-            : props.onChange(style.label)}
-      />)}
-    </fieldset>
+    <FSWrapper>
+      <fieldset
+        className={className}
+        tabIndex={0}
+        ref={ref}
+        onKeyDown={handleKeyNavigation}
+        onFocus={focusOnSelected}
+        role='radiogroup'
+      >
+        <legend>Choose highlight color</legend>
+        {highlightStyles.map((style) => <ColorButton key={style.label}
+          name={style.label}
+          checked={props.multiple ? props.selected.includes(style.label) : props.color === style.label}
+          style={style}
+          size={props.size}
+          tabIndex={-1}
+          onChange={() => props.multiple
+            ? props.selected.includes(style.label)
+              ? props.onChange(props.selected.filter(not(match(style.label))))
+              : props.onChange([...props.selected, style.label])
+            : props.color === style.label
+              ? props.onRemove ? props.onRemove() : null
+              : props.onChange(style.label)}
+        />)}
+      </fieldset>
+      { (!hasOnRemove || props.size === 'small') ? null :
+        <TrashButton
+          size={props.size}
+          onClick={props.onRemove}
+        />
+      }
+    </FSWrapper>
   );
 };
 
@@ -151,12 +173,4 @@ export default styled(ColorPicker)`
   overflow: visible;
   gap: ${cardPadding}rem;
   padding: ${cardPadding}rem 0.3rem;
-
-  legend {
-    position: absolute;
-    height: 1px;
-    width: 1px;
-    overflow: hidden;
-    clip: rect(1px, 1px, 1px, 1px);
-  }
 `;
diff --git a/src/app/content/highlights/components/EditCard.spec.tsx b/src/app/content/highlights/components/EditCard.spec.tsx
@@ -120,8 +120,9 @@ describe('EditCard', () => {
     const data = {
       ...highlightData,
       annotation: '',
+      color: 'red',
     };
-    const component = renderer.create(
+    const {root} = renderer.create(
       <TestContainer services={services} store={store}>
         <EditCard
           {...editCardProps}
@@ -131,7 +132,8 @@ describe('EditCard', () => {
       </TestContainer>
     );
 
-    const picker = component.root.findByType(ColorPicker);
+    const picker = root.findByType(ColorPicker);
+
     renderer.act(() => {
       picker.props.onRemove();
     });
@@ -154,14 +156,25 @@ describe('EditCard', () => {
     );
 
     const picker = component.root.findByType(ColorPicker);
-    renderer.act(() => {
-      picker.props.onRemove();
-    });
 
-    expect(editCardProps.onRemove).not.toHaveBeenCalled();
+    expect(picker.props.onRemove).toBeNull();
     mockSpyUser.mockClear();
   });
 
+  it('doesn\'t chain ColorPicker onRemove if there is no data', () => {
+    const mockSpyUser = jest.spyOn(selectAuth, 'user')
+      .mockReturnValue(formatUser(testAccountsUser));
+    const component = renderer.create(
+      <TestContainer services={services} store={store}>
+            <EditCard {...editCardProps} isActive={true} />
+      </TestContainer>
+    );
+
+    const picker = component.root.findByType(ColorPicker);
+
+    expect(picker.props.onRemove).toBeNull();
+    mockSpyUser.mockClear();
+  });
   it('doesn\'t chain ColorPicker onRemove if there is a pending note', () => {
     highlight.getStyle.mockReturnValue('red');
     const mockSpyUser = jest.spyOn(selectAuth, 'user')
@@ -186,11 +199,8 @@ describe('EditCard', () => {
     });
 
     const picker = component.root.findByType(ColorPicker);
-    renderer.act(() => {
-      picker.props.onRemove();
-    });
 
-    expect(editCardProps.onRemove).not.toHaveBeenCalled();
+    expect(picker.props.onRemove).toBeNull();
     mockSpyUser.mockClear();
   });
 

diff --git a/src/app/content/highlights/components/EditCard.tsx b/src/app/content/highlights/components/EditCard.tsx
@@ -251,13 +251,17 @@ function ActiveEditCard({
 function useOnRemove(props: EditCardProps, pendingAnnotation: string) {
   const onRemove = props.onRemove;
   const trackDeleteHighlight = useAnalyticsEvent('deleteHighlight');
+  const removeAndTrack = React.useCallback(
+    () => {
+      const data = assertDefined(props.data, 'props.data must be defined');
 
-  return React.useCallback(() => {
-    if (props.data && !props.data.annotation && !pendingAnnotation) {
       onRemove();
-      trackDeleteHighlight(props.data.color);
-    }
-  }, [onRemove, pendingAnnotation, props.data, trackDeleteHighlight]);
+      trackDeleteHighlight(data.color);
+    },
+    [onRemove, props.data, trackDeleteHighlight]
+  );
+
+  return props.data && !props.data.annotation && !pendingAnnotation && props.data.color ? removeAndTrack : null;
 }
 
 function AnnotationEditor({