fix(tabs): Scroll tabs correctly in RTL mode

This also handled some tech-debt of updating the Tab components to use the new keyboard movement API. Closes #1356
mlaursen · Apr 2, 2022 · a23d708 · a23d708
1 parent 71d1343
commit a23d708
Show file tree

Hide file tree

Showing 12 changed files with 653 additions and 371 deletions.
diff --git a/packages/tabs/src/Tab.tsx b/packages/tabs/src/Tab.tsx
@@ -4,10 +4,9 @@ import cn from "classnames";
 import { TextIconSpacing } from "@react-md/icon";
 import type { InteractionStatesOptions } from "@react-md/states";
 import { useInteractionStates } from "@react-md/states";
-import { bem, useResizeObserver } from "@react-md/utils";
+import { bem, useKeyboardFocusableElement } from "@react-md/utils";
 
 import type { TabConfig } from "./types";
-import { useUpdateIndicatorStyles } from "./useTabIndicatorStyle";
 
 export interface TabProps
   extends TabConfig,
@@ -65,7 +64,7 @@ export const Tab = forwardRef<HTMLButtonElement, TabProps>(function Tab(
     enablePressedAndRipple,
     ...props
   },
-  propRef
+  ref
 ) {
   const { ripples, className, handlers } = useInteractionStates({
     handlers: props,
@@ -79,19 +78,13 @@ export const Tab = forwardRef<HTMLButtonElement, TabProps>(function Tab(
     rippleContainerClassName,
     enablePressedAndRipple,
   });
-  // TODO: Look into removing this resize observer. This is only required if
-  // someone manually updates the width of the tab (dev utils) or if the width
-  // was not changed due to the tabs container element resizing (iffy)
-  const updateIndicatorStyles = useUpdateIndicatorStyles();
-  const [, refHandler] = useResizeObserver(updateIndicatorStyles, {
-    ref: propRef,
-  });
+  const refCallback = useKeyboardFocusableElement(ref);
 
   return (
     <button
       {...props}
       {...handlers}
-      ref={active ? refHandler : propRef}
+      ref={refCallback}
       aria-selected={active}
       aria-controls={panelId}
       type="button"

diff --git a/packages/tabs/src/TabPanels.tsx b/packages/tabs/src/TabPanels.tsx
@@ -19,13 +19,17 @@ export interface TabPanelsProps extends HTMLAttributes<HTMLDivElement> {
   /**
    * Boolean if this component should no longer automatically reset the scrolling
    * to the top when the panel changes.
+   *
+   * @defaultValue `false`
    */
   disableScrollFix?: boolean;
 
   /**
    * Boolean if the swiping transition should be disabled. If you want to add
    * a custom transition, you'll need to wrap the `TabPanel`'s children in a
    * custom component that does appear and exit animations.
+   *
+   * @defaultValue `false`
    */
   disableTransition?: boolean;
 
@@ -35,6 +39,8 @@ export interface TabPanelsProps extends HTMLAttributes<HTMLDivElement> {
    * instead of mounting and unmounting when their active state changes. The
    * panels will also be updated to ensure that inactive panels can not be
    * tab focusable.
+   *
+   * @defaultValue `false`
    */
   persistent?: boolean;
 }

diff --git a/packages/tabs/src/Tabs.tsx b/packages/tabs/src/Tabs.tsx
@@ -1,4 +1,5 @@
 import { forwardRef } from "react";
+import { KeyboardMovementProvider } from "@react-md/utils";
 
 import { Tab } from "./Tab";
 import type { TabsListProps } from "./TabsList";
@@ -19,17 +20,29 @@ export const Tabs = forwardRef<HTMLDivElement, TabsProps>(function Tabs(
   ref
 ) {
   const { tabsId, tabs, activeIndex, onActiveIndexChange } = useTabs();
+  const horizontal = props.orientation !== "vertical";
+
   return (
-    <TabsList
-      {...props}
-      id={tabsId}
-      ref={ref}
-      activeIndex={activeIndex}
-      onActiveIndexChange={onActiveIndexChange}
-    >
-      {tabs.map(({ id, ...config }, index) => (
-        <Tab {...config} id={id} key={id} active={activeIndex === index} />
-      ))}
-    </TabsList>
+    <KeyboardMovementProvider loopable horizontal={horizontal}>
+      <TabsList
+        {...props}
+        id={tabsId}
+        ref={ref}
+        activeIndex={activeIndex}
+        onActiveIndexChange={onActiveIndexChange}
+      >
+        {tabs.map(({ id, ...config }, index) => (
+          <Tab
+            {...config}
+            id={id}
+            key={id}
+            active={activeIndex === index}
+            onClick={() => {
+              onActiveIndexChange(index);
+            }}
+          />
+        ))}
+      </TabsList>
+    </KeyboardMovementProvider>
   );
 });
diff --git a/packages/tabs/src/TabsList.tsx b/packages/tabs/src/TabsList.tsx
@@ -1,22 +1,14 @@
-/* eslint-disable jsx-a11y/interactive-supports-focus */
-import type { HTMLAttributes, Ref } from "react";
+import type { HTMLAttributes } from "react";
+import { forwardRef } from "react";
 import {
-  Children,
-  cloneElement,
-  forwardRef,
-  isValidElement,
-  useEffect,
-  useRef,
-} from "react";
+  bem,
+  useIsUserInteractionMode,
+  useKeyboardFocus,
+} from "@react-md/utils";
 import cn from "classnames";
-import { applyRef, bem, useIsUserInteractionMode } from "@react-md/utils";
 
 import type { TabsConfig } from "./types";
-import {
-  UpdateIndicatorStylesProvider,
-  useTabIndicatorStyle,
-} from "./useTabIndicatorStyle";
-import { useTabsMovement } from "./useTabsMovement";
+import { useTabIndicatorStyles } from "./useTabIndicatorStyles";
 
 export interface TabsListProps
   extends HTMLAttributes<HTMLDivElement>,
@@ -36,11 +28,13 @@ export interface TabsListProps
   /**
    * Boolean if the indicator transition should be disabled while the active tab
    * index changes.
+   *
+   * @defaultValue `false`
    */
   disableTransition?: boolean;
 }
 
-const block = bem("rmd-tabs");
+const styles = bem("rmd-tabs");
 
 /**
  * The `TabsList` component is the container for all the individual `Tab`s that
@@ -57,7 +51,7 @@ export const TabsList = forwardRef<HTMLDivElement, TabsListProps>(
     {
       style,
       className,
-      onClick,
+      onFocus,
       onKeyDown,
       children,
       activeIndex,
@@ -69,91 +63,46 @@ export const TabsList = forwardRef<HTMLDivElement, TabsListProps>(
       disableTransition = false,
       ...props
     },
-    forwardedRef
+    ref
   ) {
     const horizontal = orientation === "horizontal";
-    const { tabs, itemRefs, handleClick, handleKeyDown } = useTabsMovement({
-      onClick,
+    const isKeyboard = useIsUserInteractionMode("keyboard");
+    const { focusIndex: _focusIndex, ...eventHandlers } = useKeyboardFocus({
+      onFocus,
       onKeyDown,
-      children,
-      horizontal,
-      activeIndex,
-      onActiveIndexChange,
-      automatic,
+      onFocusChange(element, focusIndex) {
+        element.focus();
+        if (automatic) {
+          onActiveIndexChange(focusIndex);
+        }
+      },
     });
-    const [mergedStyle, tabsRefHandler, tabsRef, updateIndicatorStyles] =
-      useTabIndicatorStyle({
-        style,
-        ref: forwardedRef,
-        align,
-        itemRefs,
-        totalTabs: tabs.length,
-        activeIndex,
-      });
-    const isKeyboard = useIsUserInteractionMode("keyboard");
 
-    const prevActiveIndex = useRef(activeIndex);
-    useEffect(() => {
-      const tabs = tabsRef.current;
-      const tabRef = itemRefs[activeIndex] && itemRefs[activeIndex].current;
-      const incrementing = prevActiveIndex.current < activeIndex;
-      prevActiveIndex.current = activeIndex;
-      if (!tabs || !tabRef) {
-        return;
-      }
-
-      const currentX = tabs.scrollLeft + tabs.offsetWidth;
-      const tabLeft = tabRef.offsetLeft;
-      const tabWidth = tabRef.offsetWidth;
-      if (incrementing && currentX < tabLeft + tabWidth) {
-        tabs.scrollLeft = tabLeft - tabWidth;
-      } else if (!incrementing && tabs.scrollLeft > tabLeft) {
-        tabs.scrollLeft = tabLeft;
-      }
-
-      // don't want this to trigger on itemRefs or tabsRef changes since those
-      // have a chance of updating each render.
-      // eslint-disable-next-line react-hooks/exhaustive-deps
-    }, [activeIndex]);
+    const { refCallback, indicatorStyles } = useTabIndicatorStyles({
+      ref,
+      activeIndex,
+    });
 
     return (
-      <UpdateIndicatorStylesProvider value={updateIndicatorStyles}>
-        <div
-          {...props}
-          aria-orientation={orientation}
-          style={mergedStyle}
-          role="tablist"
-          className={cn(
-            block({
-              [align]: true,
-              padded,
-              vertical: !horizontal,
-              animate: !disableTransition && (!automatic || !isKeyboard),
-            }),
-            className
-          )}
-          ref={tabsRefHandler}
-          onClick={handleClick}
-          onKeyDown={handleKeyDown}
-        >
-          {Children.map(tabs, (child, i) => {
-            if (!isValidElement(child)) {
-              return child;
-            }
-
-            const tab = Children.only(child);
-            let ref: Ref<HTMLElement> = itemRefs[i];
-            if (tab.props.ref) {
-              ref = (instance: HTMLElement | null) => {
-                itemRefs[i].current = instance;
-                applyRef(instance, tab.props.ref);
-              };
-            }
-
-            return cloneElement(tab, { ref });
-          })}
-        </div>
-      </UpdateIndicatorStylesProvider>
+      <div
+        {...props}
+        aria-orientation={orientation}
+        style={{ ...style, ...indicatorStyles }}
+        role="tablist"
+        ref={refCallback}
+        className={cn(
+          styles({
+            [align]: true,
+            padded,
+            vertical: !horizontal,
+            animate: !disableTransition && (!automatic || !isKeyboard),
+          }),
+          className
+        )}
+        {...eventHandlers}
+      >
+        {children}
+      </div>
     );
   }
 );
diff --git a/packages/tabs/src/TabsManager.tsx b/packages/tabs/src/TabsManager.tsx
@@ -39,7 +39,7 @@ export interface TabsManagerContext {
    * A function to call when the `activeIndex` should change due to keyboard
    * movement or clicking on a tab.
    */
-  onActiveIndexChange: (activeIndex: number) => void;
+  onActiveIndexChange(activeIndex: number): void;
 
   /**
    * The list of tabs that should be controlled by the tabs manager.
@@ -76,6 +76,8 @@ export interface TabsManagerProps
   /**
    * The index of the tab that should be active by default. This is ignored if
    * the `activeIndex` prop is defined.
+   *
+   * @defaultValue `0`
    */
   defaultActiveIndex?: number;
 
@@ -114,6 +116,8 @@ export interface TabsManagerProps
    * it for each tab in the `tabs` list and if a `tab` in the `tabs` list has
    * the `stacked` attribute enabled defined, it will be used instead of this
    * value.
+   *
+   * @defaultValue `false`
    */
   stacked?: boolean;
 
@@ -126,6 +130,8 @@ export interface TabsManagerProps
    * it for each tab in the `tabs` list and if a `tab` in the `tabs` list has
    * the `stacked` attribute enabled defined, it will be used instead of this
    * value.
+   *
+   * @defaultValue `false`
    */
   iconAfter?: boolean;
 }

diff --git a/packages/tabs/src/__tests__/Tab.tsx b/packages/tabs/src/__tests__/Tab.tsx
@@ -1,11 +1,14 @@
+import { KeyboardMovementProvider } from "@react-md/utils";
 import { render } from "@testing-library/react";
 
 import { Tab } from "../Tab";
 
 describe("Tab", () => {
   it("should render correctly", () => {
     const props = { id: "tab", active: false, children: "Tab" };
-    const { container, rerender } = render(<Tab {...props} />);
+    const { container, rerender } = render(<Tab {...props} />, {
+      wrapper: KeyboardMovementProvider,
+    });
 
     expect(container).toMatchSnapshot();