Fix/dropdown accessibility (react-bootstrap#4977) (react-bootstrap#4978)

* Allow id and aria-controls props to pass through AbstractNavItem if navContext doesn't provide a value.

* Fix documentation typo

* Add aria-controls to NavDropdown and always render the dropdown menu.

* Add tests for the dropdown id and aria-controls changes

* Add aria-controls to DropdownToggle in DropdownButton and SplitButton

* Add tests for aria-controls attribute on DropdownButton and SplitButton

* Fix test name typo

* Add tests for aria-controls attribute on DropdownButton and SplitButton

* Add prop to render dropdown menus in the DOM before being shown

* Display a warning if user's id is ignored in AbstractNavItem.

Also pass the user provided id through to Tab children so the inner AbstractNavItem knows the user's id is being overwritten.

Author: tragessere

src/AbstractNavItem.js

@@ -3,10 +3,12 @@ import PropTypes from 'prop-types';
 import React, { useContext } from 'react';
 import useEventCallback from '@restart/hooks/useEventCallback';
 
+import warning from 'warning';
 import NavContext from './NavContext';
 import SelectableContext, { makeEventKey } from './SelectableContext';
 
 const propTypes = {
+  id: PropTypes.string,
   active: PropTypes.bool,
   role: PropTypes.string,
 
@@ -18,6 +20,8 @@ const propTypes = {
   as: PropTypes.any,
   onClick: PropTypes.func,
   onSelect: PropTypes.func,
+
+  'aria-controls': PropTypes.string,
 };
 
 const defaultProps = {
@@ -46,9 +50,21 @@ const AbstractNavItem = React.forwardRef(
     if (navContext) {
       if (!props.role && navContext.role === 'tablist') props.role = 'tab';
 
+      const contextControllerId = navContext.getControllerId(navKey);
+      const contextControlledId = navContext.getControlledId(navKey);
+
+      warning(
+        !contextControllerId || !props.id,
+        `[react-bootstrap] The provided id '${props.id}' was overwritten by the current navContext with '${contextControllerId}'.`,
+      );
+      warning(
+        !contextControlledId || !props['aria-controls'],
+        `[react-bootstrap] The provided aria-controls value '${props['aria-controls']}' was overwritten by the current navContext with '${contextControlledId}'.`,
+      );
+
       props['data-rb-event-key'] = navKey;
-      props.id = navContext.getControllerId(navKey);
-      props['aria-controls'] = navContext.getControlledId(navKey);
+      props.id = contextControllerId || props.id;
+      props['aria-controls'] = contextControlledId || props['aria-controls'];
 
       isActive =
         active == null && navKey != null

src/DropdownButton.js

@@ -26,6 +26,9 @@ const propTypes = {
   /** An ARIA accessible role applied to the Menu component. When set to 'menu', The dropdown */
   menuRole: PropTypes.string,
 
+  /** Whether to render the dropdown menu in the DOM before the first time it is shown */
+  renderMenuOnMount: PropTypes.bool,
+
   /**
    *  Which event when fired outside the component will cause it to be closed.
    *
@@ -59,6 +62,7 @@ const DropdownButton = React.forwardRef(
       variant,
       size,
       menuRole,
+      renderMenuOnMount,
       disabled,
       href,
       id,
@@ -77,7 +81,11 @@ const DropdownButton = React.forwardRef(
       >
         {title}
       </Dropdown.Toggle>
-      <Dropdown.Menu role={menuRole} rootCloseEvent={rootCloseEvent}>
+      <Dropdown.Menu
+        role={menuRole}
+        renderOnMount={renderMenuOnMount}
+        rootCloseEvent={rootCloseEvent}
+      >
         {children}
       </Dropdown.Menu>
     </Dropdown>

src/DropdownMenu.js

@@ -16,6 +16,9 @@ const propTypes = {
   /** Controls the visibility of the Dropdown menu  */
   show: PropTypes.bool,
 
+  /** Whether to render the dropdown menu in the DOM before the first time it is shown */
+  renderOnMount: PropTypes.bool,
+
   /** Have the dropdown switch to it's opposite placement when necessary to stay on screen. */
   flip: PropTypes.bool,
 
@@ -63,6 +66,7 @@ const DropdownMenu = React.forwardRef(
       flip,
       popperConfig,
       show: showProps,
+      renderOnMount,
       // Need to define the default "as" during prop destructuring to be compatible with styled-components github.com/react-bootstrap/react-bootstrap/issues/3595
       as: Component = 'div',
       ...props
@@ -92,7 +96,7 @@ const DropdownMenu = React.forwardRef(
       useWrappedRefWithWarning(ref, 'DropdownMenu'),
     );
 
-    if (!hasShown) return null;
+    if (!hasShown && !renderOnMount) return null;
 
     // For custom components provide additional, non-DOM, props;
     if (typeof Component !== 'string') {

src/NavDropdown.js

@@ -28,6 +28,9 @@ const propTypes = {
   /** An ARIA accessible role applied to the Menu component. When set to 'menu', The dropdown */
   menuRole: PropTypes.string,
 
+  /** Whether to render the dropdown menu in the DOM before the first time it is shown */
+  renderMenuOnMount: PropTypes.bool,
+
   /**
    *  Which event when fired outside the component will cause it to be closed.
    *
@@ -50,6 +53,7 @@ const NavDropdown = React.forwardRef(
       menuRole,
       disabled,
       active,
+      renderMenuOnMount,
       ...props
     },
     ref,
@@ -66,7 +70,11 @@ const NavDropdown = React.forwardRef(
         {title}
       </Dropdown.Toggle>
 
-      <Dropdown.Menu role={menuRole} rootCloseEvent={rootCloseEvent}>
+      <Dropdown.Menu
+        role={menuRole}
+        renderOnMount={renderMenuOnMount}
+        rootCloseEvent={rootCloseEvent}
+      >
         {children}
       </Dropdown.Menu>
     </Dropdown>

src/SplitButton.js

@@ -38,6 +38,10 @@ const propTypes = {
 
   /** An ARIA accessible role applied to the Menu component. When set to 'menu', The dropdown */
   menuRole: PropTypes.string,
+
+  /** Whether to render the dropdown menu in the DOM before the first time it is shown */
+  renderMenuOnMount: PropTypes.bool,
+
   /**
    *  Which event when fired outside the component will cause it to be closed.
    *
@@ -73,6 +77,7 @@ const SplitButton = React.forwardRef(
       href,
       target,
       menuRole,
+      renderMenuOnMount,
       rootCloseEvent,
       ...props
     },
@@ -102,7 +107,11 @@ const SplitButton = React.forwardRef(
         <span className="sr-only">{toggleLabel}</span>
       </Dropdown.Toggle>
 
-      <Dropdown.Menu role={menuRole} rootCloseEvent={rootCloseEvent}>
+      <Dropdown.Menu
+        role={menuRole}
+        renderOnMount={renderMenuOnMount}
+        rootCloseEvent={rootCloseEvent}
+      >
         {children}
       </Dropdown.Menu>
     </Dropdown>

src/Tabs.js

@@ -94,7 +94,7 @@ function getDefaultActiveKey(children) {
 }
 
 function renderTab(child) {
-  const { title, eventKey, disabled, tabClassName } = child.props;
+  const { title, eventKey, disabled, tabClassName, id } = child.props;
   if (title == null) {
     return null;
   }
@@ -104,6 +104,7 @@ function renderTab(child) {
       as={NavLink}
       eventKey={eventKey}
       disabled={disabled}
+      id={id}
       className={tabClassName}
     >
       {title}

test/DropdownMenuSpec.js

@@ -33,6 +33,14 @@ describe('<Dropdown.Menu>', () => {
     ).assertSingle('.dropdown-menu-right');
   });
 
+  it('renders on mount with prop', () => {
+    mount(
+      <DropdownMenu renderOnMount>
+        <DropdownItem>Item</DropdownItem>
+      </DropdownMenu>,
+    ).assertSingle('div.dropdown-menu');
+  });
+
   // it.only('warns about bad refs', () => {
   //   class Parent extends React.Component {
   //     componentDidCatch() {}

test/NavDropdownSpec.js

@@ -49,4 +49,14 @@ describe('<NavDropdown>', () => {
       .text()
       .should.equal('DropdownItem 2 content');
   });
+
+  it('should pass the id to the NavLink element', () => {
+    const wrapper = mount(
+      <NavDropdown id="test-id" title="title">
+        <DropdownItem eventKey="1">DropdownItem 1 content</DropdownItem>
+      </NavDropdown>,
+    );
+
+    wrapper.assertSingle('a#test-id');
+  });
 });

types/components/DropdownButton.d.ts

@@ -16,6 +16,7 @@ export interface DropdownButtonProps extends PropsFromToggle {
   id: string;
   title: React.ReactNode;
   menuRole?: string;
+  renderMenuOnMount?: boolean;
   rootCloseEvent?: 'click' | 'mousedown';
   bsPrefix?: string;
 }

types/components/DropdownMenu.d.ts

@@ -4,6 +4,7 @@ import { BsPrefixComponent, SelectCallback } from './helpers';
 
 export interface DropdownMenuProps {
   show?: boolean;
+  renderOnMount?: boolean;
   flip?: boolean;
   alignRight?: boolean;
   onSelect?: SelectCallback;

types/components/NavDropdown.d.ts

@@ -10,6 +10,7 @@ export interface NavDropdownProps {
   disabled?: boolean;
   active?: boolean;
   menuRole?: string;
+  renderMenuOnMount?: boolean;
   rootCloseEvent?: 'click' | 'mousedown';
   bsPrefix?: string;
 }

types/components/SplitButton.d.ts

@@ -20,6 +20,7 @@ export interface SplitButtonProps extends PropsFromToggle {
   onClick?: React.MouseEventHandler<this>;
   title: React.ReactNode;
   menuRole?: string;
+  renderMenuOnMount?: boolean;
   rootCloseEvent?: 'click' | 'mousedown';
   bsPrefix?: string;
 }

www/src/pages/components/navs.mdx

@@ -116,7 +116,7 @@ component
 
 <CodeBlock codeText={NavDropdownImpl} />
 
-The above demostrates how flexible the component model can be. But if
+The above demonstrates how flexible the component model can be. But if
 you didn't want to roll your own versions we've included a
 straight-forward `<NavDropdown>` that works for most cases.