From eef4267a3075c4aac1601bbb40c1ed69b3beadee Mon Sep 17 00:00:00 2001 From: Ed Leeks Date: Fri, 22 Oct 2021 11:50:17 +0100 Subject: [PATCH] fix(tabs): memoize `tabRef` array so it regenerate when children change Updates implementation of `tabRefs` to `useMemo`, this means they will regenerate if the children change at all fix #4498 --- src/components/tabs/tabs.component.js | 42 +++++++------ src/components/tabs/tabs.spec.js | 91 +++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 19 deletions(-) diff --git a/src/components/tabs/tabs.component.js b/src/components/tabs/tabs.component.js index 6068be69d9..02da4e8740 100644 --- a/src/components/tabs/tabs.component.js +++ b/src/components/tabs/tabs.component.js @@ -3,8 +3,12 @@ import React, { useContext, useEffect, useLayoutEffect, + useMemo, useRef, useState, + createRef, + cloneElement, + Children, } from "react"; import PropTypes from "prop-types"; import styledSystemPropTypes from "@styled-system/prop-types"; @@ -37,7 +41,19 @@ const Tabs = ({ headerWidth, ...rest }) => { - const tabRefs = useRef([]); + /** The children nodes converted into an Array */ + const filteredChildren = useMemo( + () => Children.toArray(children).filter((child) => child), + [children] + ); + + /** Array of refs to the TabTitle nodes */ + const tabRefs = useMemo( + () => + Array.from({ length: filteredChildren.length }).map(() => createRef()), + [filteredChildren.length] + ); + const previousSelectedTabId = useRef(selectedTabId); const [selectedTabIdState, setSelectedTabIdState] = useState(); const { isInSidebar } = useContext(DrawerSidebarContext); @@ -47,7 +63,7 @@ const Tabs = ({ useLayoutEffect(() => { const selectedTab = - selectedTabId || React.Children.toArray(children)[0].props.tabId; + selectedTabId || Children.toArray(children)[0].props.tabId; setSelectedTabIdState(selectedTab); // eslint-disable-next-line react-hooks/exhaustive-deps @@ -106,13 +122,7 @@ const Tabs = ({ }; /** Focuses the tab for the reference specified */ - const focusTab = (ref) => ref.focus(); - - /** The children nodes converted into an Array */ - const filteredChildren = React.useMemo( - () => React.Children.toArray(children).filter((child) => child), - [children] - ); + const focusTab = (ref) => ref.current.focus(); /** Array of the tabIds for the child nodes */ const tabIds = () => { @@ -132,7 +142,7 @@ const Tabs = ({ newIndex = 0; } const nextTabId = ids[newIndex]; - const nextRef = tabRefs.current[newIndex]; + const nextRef = tabRefs[newIndex]; updateVisibleTab(nextTabId); focusTab(nextRef); }; @@ -156,12 +166,6 @@ const Tabs = ({ /** Returns true/false for if the given tab id is selected. */ const isTabSelected = (tabId) => tabId === selectedTabIdState; - const addRef = (ref) => { - if (ref && !tabRefs.current.includes(ref)) { - tabRefs.current.push(ref); - } - }; - /** Build the headers for the tab component */ const renderTabHeaders = () => { const tabTitles = filteredChildren.map((child, index) => { @@ -218,7 +222,7 @@ const Tabs = ({ key={tabId} onClick={handleTabClick} onKeyDown={handleKeyDown(index)} - ref={(node) => addRef(node)} + ref={tabRefs[index]} tabIndex={isTabSelected(tabId) ? "0" : "-1"} title={title} href={href} @@ -270,7 +274,7 @@ const Tabs = ({ }); return tab - ? React.cloneElement(tab, { + ? cloneElement(tab, { isTabSelected: isTabSelected(tab.props.tabId), }) : null; @@ -285,7 +289,7 @@ const Tabs = ({ } const tabs = filteredChildren.map((child) => { - return React.cloneElement(child, { + return cloneElement(child, { ...child.props, role: "tabpanel", position, diff --git a/src/components/tabs/tabs.spec.js b/src/components/tabs/tabs.spec.js index a18fb7a119..216867c065 100644 --- a/src/components/tabs/tabs.spec.js +++ b/src/components/tabs/tabs.spec.js @@ -9,6 +9,7 @@ import { TabContext } from "./tab/index"; import { rootTagTest } from "../../__internal__/utils/helpers/tags/tags-specs"; import StyledTabs from "./tabs.style"; import StyledTab from "./tab/tab.style"; +import { StyledTabTitle } from "./__internal__/tab-title/tab-title.style"; import { assertStyleMatch, simulate, @@ -734,6 +735,96 @@ describe("Tabs", () => { expect(tabTitle.at(1).props().info).toEqual(false); }); }); + + describe("Keyboard behaviour", () => { + let container; + let wrapper; + const tabTitles = ["tab-1", "tab-2", "tab-3"]; + + const ConditionalChildrenMock = () => { + const [showAllTabs, setShowAllTabs] = React.useState(true); + + const generateTab = (tabTitle) => ( + + {tabTitle} + + ); + + return ( + <> + + + {!showAllTabs && generateTab(tabTitles[0])} + {showAllTabs && + tabTitles.map((tabTitle) => generateTab(tabTitle))} + + + ); + }; + + beforeEach(() => { + container = document.createElement("div"); + container.id = "enzymeContainer"; + document.body.appendChild(container); + + wrapper = mount(, { + attachTo: document.getElementById("enzymeContainer"), + }); + }); + + afterEach(() => { + if (container?.parentNode) { + container.parentNode.removeChild(container); + } + + container = null; + }); + + const runFocusExpectations = (keyDown, array) => + array.forEach((index) => { + const child = wrapper.update().find(StyledTabTitle).at(index); + expect(child.getDOMNode()).toBeFocused(); + simulate.keydown[keyDown](child); + }); + + const toggleChildren = () => { + act(() => { + wrapper.find("#foo").prop("onClick")(); + }); + + expect(wrapper.update().find(StyledTabTitle).length).toEqual(1); + + act(() => { + wrapper.find("#foo").prop("onClick")(); + }); + + expect(wrapper.update().find(StyledTabTitle).length).toEqual(3); + }; + + it("is consistent when navigating with the arrow keys and the composition of the children changes", () => { + wrapper.find(StyledTabTitle).first().getDOMNode().focus(); + + runFocusExpectations("pressLeftArrow", [0, 2, 1, 0, 2]); + + toggleChildren(); + + wrapper.find(StyledTabTitle).first().getDOMNode().focus(); + + runFocusExpectations("pressLeftArrow", [0, 2, 1, 0, 2]); + + toggleChildren(); + + wrapper.find(StyledTabTitle).first().getDOMNode().focus(); + + runFocusExpectations("pressRightArrow", [0, 1, 2, 0]); + }); + }); }); describe("tags", () => {