fix: small nits and added aria-labelledby

dequelabs · Aug 29, 2023 · 92d725d · 92d725d
1 parent 265766b
commit 92d725d
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 21 deletions.
diff --git a/docs/pages/components/Popover.mdx b/docs/pages/components/Popover.mdx
@@ -160,5 +160,15 @@ function BigTooltipExample() {
       description: 'Label for button, which is used to close popover in Alert variant of the component',
       defaultValue: 'Close'
     },
+    {
+      name: 'aria-label',
+      type: 'string',
+      description: 'A label for tabs is required. This means you must provide either an aria-label or aria-labelledby prop. Please note that for alert variant we use infoText to get labelled by'
+    },
+    {
+      name: 'aria-labelledby',
+      type: 'string',
+      description: 'A label for tabs is required. This means you must provide either an aria-label or aria-labelledby prop. Please note that for alert variant we use infoText to get labelled by'
+    },
   ]}
 />
diff --git a/packages/react/__tests__/src/components/Popover/index.js b/packages/react/__tests__/src/components/Popover/index.js
@@ -331,3 +331,17 @@ test('deactivates aria isolate on hide', async () => {
 
   expect(deactivateFn).toBeCalled();
 });
+
+test('aria-labelledby is set correctly for alert variant', async () => {
+  const wrapper = mount(<WrapperAlert />);
+  await update(wrapper);
+
+  const id = wrapper.find('.Popover').props().id;
+
+  expect(`${id}-label`).toEqual(
+    wrapper
+      .find('.Popover')
+      .getDOMNode()
+      .getAttribute('aria-labelledby')
+  );
+});
diff --git a/packages/react/src/components/Popover/index.tsx b/packages/react/src/components/Popover/index.tsx
@@ -1,24 +1,18 @@
-import React, {
-  useState,
-  useEffect,
-  ReactNode,
-  forwardRef,
-  Ref,
-  useRef,
-  MutableRefObject
-} from 'react';
+import React, { useState, useEffect, ReactNode, forwardRef, Ref } from 'react';
 import { createPortal } from 'react-dom';
 import { useId } from 'react-id-generator';
 import { Placement } from '@popperjs/core';
 import { usePopper } from 'react-popper';
 import { isBrowser } from '../../utils/is-browser';
+import { Cauldron } from '../../types';
 
 import classnames from 'classnames';
 import ClickOutsideListener from '../ClickOutsideListener';
 import Button from '../Button';
 import FocusTrap from 'focus-trap-react';
 import focusableSelector from '../../utils/focusable-selector';
 import AriaIsolate from '../../utils/aria-isolate';
+import useSharedRef from '../../utils/useSharedRef';
 
 export type PopoverVariant = 'alert' | 'custom';
 
@@ -38,7 +32,7 @@ type CustomProps = BaseProps & {
   closeButtonText?: string;
   infoText?: string;
   chidlren: ReactNode;
-};
+} & Cauldron.LabelProps;
 
 type AlertProps = BaseProps & {
   variant: 'alert';
@@ -56,14 +50,15 @@ const AlertPopoverContent = ({
   applyButtonText = 'Apply',
   onApply,
   closeButtonText = 'Close',
-  infoText
+  infoText,
+  infoTextId
 }: Pick<
   PopoverProps,
   'onClose' | 'applyButtonText' | 'onApply' | 'closeButtonText' | 'infoText'
->) => {
+> & { infoTextId: string }) => {
   return (
     <>
-      {infoText}
+      <span id={infoTextId}>{infoText}</span>
       <Button className="Popover__apply" onClick={onApply} thin>
         {applyButtonText}
       </Button>
@@ -79,7 +74,7 @@ const AlertPopoverContent = ({
   );
 };
 
-const Popover = forwardRef<HTMLElement, PopoverProps>(
+const Popover = forwardRef<HTMLDivElement, PopoverProps>(
   (
     {
       id: propId,
@@ -97,7 +92,7 @@ const Popover = forwardRef<HTMLElement, PopoverProps>(
       onApply,
       ...props
     }: PopoverProps,
-    ref: Ref<HTMLElement>
+    ref: Ref<HTMLDivElement>
   ): JSX.Element | null => {
     const [id] = propId ? [propId] : useId(1, 'popover');
 
@@ -107,10 +102,7 @@ const Popover = forwardRef<HTMLElement, PopoverProps>(
 
     const [isolator, setIsolator] = useState<AriaIsolate | null>(null);
 
-    const popoverElement = useRef<HTMLDivElement | null>(null);
-
-    const popoverRef =
-      (ref as MutableRefObject<HTMLDivElement>) || popoverElement;
+    const popoverRef = useSharedRef<HTMLDivElement>(ref);
 
     const [arrowElement, setArrowElement] = useState<HTMLElement | null>(null);
 
@@ -133,6 +125,9 @@ const Popover = forwardRef<HTMLElement, PopoverProps>(
         (attributes.popper['data-popper-placement'] as Placement)) ||
       initialPlacement;
 
+    const additionalProps =
+      variant === 'alert' ? { 'aria-labelledby': `${id}-label` } : {};
+
     // Keep targetElement in sync with target prop
     useEffect(() => {
       const targetElement =
@@ -149,8 +144,15 @@ const Popover = forwardRef<HTMLElement, PopoverProps>(
     useEffect(() => {
       if (!isolator) return;
 
-      if (show) isolator.activate();
-      else isolator.deactivate();
+      if (show) {
+        isolator.activate();
+      } else {
+        isolator.deactivate();
+      }
+
+      return () => {
+        isolator?.deactivate();
+      };
     }, [show, isolator]);
 
     useEffect(() => {
@@ -252,6 +254,7 @@ const Popover = forwardRef<HTMLElement, PopoverProps>(
             style={styles.popper}
             {...attributes.popper}
             {...props}
+            {...additionalProps}
           >
             <div
               className="Popover__popoverArrow"
@@ -266,6 +269,7 @@ const Popover = forwardRef<HTMLElement, PopoverProps>(
                 closeButtonText={closeButtonText}
                 infoText={infoText || ''}
                 onClose={handleClosePopover}
+                infoTextId={`${id}-label`}
               />
             ) : (
               children