fix: handle multiple classes in PortalCompatProvider (#29351)

microsoft · Oct 2, 2023 · 18ddec9 · 18ddec9
1 parent 1f6ebde
commit 18ddec9
Show file tree

Hide file tree

Showing 3 changed files with 149 additions and 81 deletions.
diff --git a/change/@fluentui-react-portal-compat-89771dd5-317e-4567-a861-92ddaecdbfdd.json b/change/@fluentui-react-portal-compat-89771dd5-317e-4567-a861-92ddaecdbfdd.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "fix: handle multiple classes in PortalCompatProvider",
+  "packageName": "@fluentui/react-portal-compat",
+  "email": "olfedias@microsoft.com",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/react-components/react-portal-compat/src/PortalCompatProvider.test.tsx b/packages/react-components/react-portal-compat/src/PortalCompatProvider.test.tsx
@@ -1,49 +1,66 @@
 import { ThemeClassNameProvider_unstable as ThemeClassNameProvider } from '@fluentui/react-shared-contexts';
 import { usePortalCompat } from '@fluentui/react-portal-compat-context';
-import { FluentProvider } from '@fluentui/react-provider';
+import { FluentProvider, useFluentProviderThemeStyleTag } from '@fluentui/react-provider';
 import { IdPrefixProvider, resetIdsForTests } from '@fluentui/react-utilities';
 import { renderHook } from '@testing-library/react-hooks';
 import * as React from 'react';
 
-import { PortalCompatProvider } from './PortalCompatProvider';
+import { PortalCompatProvider, useProviderThemeClasses } from './PortalCompatProvider';
 
 // eslint-disable-next-line @typescript-eslint/no-empty-function
 const noop = () => {};
 
-describe('PortalCompatProvider', () => {
-  afterEach(() => {
-    resetIdsForTests();
+const TestWrapperWithMultipleClasses: React.FC = props => {
+  // Creates a second className with CSS variables
+  const { styleTagId } = useFluentProviderThemeStyleTag({
+    theme: { borderRadiusCircular: '50px' },
+    targetDocument: document,
+    rendererAttributes: {},
   });
 
-  it('registers a function in a context', () => {
-    jest.spyOn(console, 'warn').mockImplementation(noop);
-
-    const { result } = renderHook(() => usePortalCompat(), { wrapper: PortalCompatProvider });
+  return (
+    <FluentProvider className={styleTagId} theme={{ colorNeutralBackground1: '#ccc' }}>
+      <PortalCompatProvider>{props.children}</PortalCompatProvider>
+    </FluentProvider>
+  );
+};
 
-    expect(result.current).toBeInstanceOf(Function);
+describe('useProviderThemeClasses', () => {
+  afterEach(() => {
+    resetIdsForTests();
   });
 
-  it('during register adds a className from "ThemeClassNameContext" context', () => {
-    const element = document.createElement('div');
-    const { result } = renderHook(() => usePortalCompat(), {
+  it('handles classes from FluentProvider', () => {
+    const { result } = renderHook(() => useProviderThemeClasses(), {
       wrapper: props => (
         <FluentProvider theme={{ colorNeutralBackground1: '#ccc' }}>
           <PortalCompatProvider>{props.children}</PortalCompatProvider>
         </FluentProvider>
       ),
     });
 
-    expect(result.current(element)).toBeInstanceOf(Function);
-    expect(element.classList).toMatchInlineSnapshot(`
-      DOMTokenList {
-        "0": "fui-FluentProvider1",
-      }
+    expect(result.current).toMatchInlineSnapshot(`
+      Array [
+        "fui-FluentProvider1",
+      ]
     `);
   });
 
-  it('during register adds a className from "ThemeClassNameContext" context with custom ID prefix', () => {
-    const element = document.createElement('div');
-    const { result } = renderHook(() => usePortalCompat(), {
+  it('handles multiple classes from FluentProvider', () => {
+    const { result } = renderHook(() => useProviderThemeClasses(), {
+      wrapper: TestWrapperWithMultipleClasses,
+    });
+
+    expect(result.current).toMatchInlineSnapshot(`
+      Array [
+        "fui-FluentProvider2",
+        "fui-FluentProvider1",
+      ]
+    `);
+  });
+
+  it('handles classes with custom ID prefix', () => {
+    const { result } = renderHook(() => useProviderThemeClasses(), {
       wrapper: props => (
         <IdPrefixProvider value="custom1-">
           <FluentProvider theme={{ colorNeutralBackground1: '#ccc' }}>
@@ -53,28 +70,95 @@ describe('PortalCompatProvider', () => {
       ),
     });
 
+    expect(result.current).toMatchInlineSnapshot(`
+      Array [
+        "custom1-fui-FluentProvider1",
+      ]
+    `);
+  });
+
+  it('handles classes with a React 18 compatible ID', () => {
+    const { result } = renderHook(() => useProviderThemeClasses(), {
+      wrapper: props => (
+        <ThemeClassNameProvider value="fui-FluentProviderR1a">
+          <PortalCompatProvider>{props.children}</PortalCompatProvider>
+        </ThemeClassNameProvider>
+      ),
+    });
+
+    expect(result.current).toMatchInlineSnapshot(`
+      Array [
+        "fui-FluentProviderR1a",
+      ]
+    `);
+  });
+
+  it('returns only proper classes', () => {
+    const { result } = renderHook(() => useProviderThemeClasses(), {
+      wrapper: props => (
+        <ThemeClassNameProvider value="foo bar baz">
+          <PortalCompatProvider>{props.children}</PortalCompatProvider>
+        </ThemeClassNameProvider>
+      ),
+    });
+
+    expect(result.current).toHaveLength(0);
+  });
+
+  it('logs a warning when does not have top level FluentProvider', () => {
+    const warn = jest.fn().mockImplementation(noop);
+    jest.spyOn(console, 'warn').mockImplementation(warn);
+
+    renderHook(() => useProviderThemeClasses(), { wrapper: PortalCompatProvider });
+
+    expect(warn).toHaveBeenCalledWith(
+      expect.stringContaining('PortalCompatProvider: "useThemeClassName()" hook returned an empty string'),
+    );
+  });
+});
+
+describe('PortalCompatProvider', () => {
+  afterEach(() => {
+    resetIdsForTests();
+  });
+
+  it('registers a function in a context', () => {
+    jest.spyOn(console, 'warn').mockImplementation(noop);
+
+    const { result } = renderHook(() => usePortalCompat(), { wrapper: PortalCompatProvider });
+
+    expect(result.current).toBeInstanceOf(Function);
+  });
+
+  it('during register adds a className from "ThemeClassNameContext" context', () => {
+    const element = document.createElement('div');
+    const { result } = renderHook(() => usePortalCompat(), {
+      wrapper: props => (
+        <FluentProvider theme={{ colorNeutralBackground1: '#ccc' }}>
+          <PortalCompatProvider>{props.children}</PortalCompatProvider>
+        </FluentProvider>
+      ),
+    });
+
     expect(result.current(element)).toBeInstanceOf(Function);
     expect(element.classList).toMatchInlineSnapshot(`
       DOMTokenList {
-        "0": "custom1-fui-FluentProvider1",
+        "0": "fui-FluentProvider1",
       }
     `);
   });
 
-  it('during register adds a className from "ThemeClassNameContext" context with a React 18 compatible ID', () => {
+  it('during register adds multiple classes from "ThemeClassNameContext" context if they exist', () => {
     const element = document.createElement('div');
     const { result } = renderHook(() => usePortalCompat(), {
-      wrapper: props => (
-        <ThemeClassNameProvider value="fui-FluentProviderR1a">
-          <PortalCompatProvider>{props.children}</PortalCompatProvider>
-        </ThemeClassNameProvider>
-      ),
+      wrapper: TestWrapperWithMultipleClasses,
     });
 
     expect(result.current(element)).toBeInstanceOf(Function);
     expect(element.classList).toMatchInlineSnapshot(`
       DOMTokenList {
-        "0": "fui-FluentProviderR1a",
+        "0": "fui-FluentProvider2",
+        "1": "fui-FluentProvider1",
       }
     `);
   });
@@ -101,29 +185,4 @@ describe('PortalCompatProvider', () => {
     expect(unregister()).toBeUndefined();
     expect(element.classList.length).toBe(0);
   });
-
-  it('during register adds only proper className', () => {
-    const element = document.createElement('div');
-    const { result } = renderHook(() => usePortalCompat(), {
-      wrapper: props => (
-        <ThemeClassNameProvider value="foo bar baz">
-          <PortalCompatProvider>{props.children}</PortalCompatProvider>
-        </ThemeClassNameProvider>
-      ),
-    });
-    result.current(element);
-
-    expect(element.classList.length).toBe(0);
-  });
-
-  it('logs a warning when does not have top level FluentProvider', () => {
-    const warn = jest.fn().mockImplementation(noop);
-    jest.spyOn(console, 'warn').mockImplementation(warn);
-
-    renderHook(() => usePortalCompat(), { wrapper: PortalCompatProvider });
-
-    expect(warn).toHaveBeenCalledWith(
-      expect.stringContaining('PortalCompatProvider: "useThemeClassName()" hook returned an empty string'),
-    );
-  });
 });
diff --git a/packages/react-components/react-portal-compat/src/PortalCompatProvider.tsx b/packages/react-components/react-portal-compat/src/PortalCompatProvider.tsx
@@ -5,39 +5,17 @@ import { applyFocusVisiblePolyfill } from '@fluentui/react-tabster';
 
 import type { RegisterPortalFn } from '@fluentui/react-portal-compat-context';
 
-const CLASS_NAME_REGEX = new RegExp(`([^\\s]*${fluentProviderClassNames.root}\\w+)`);
-
-export const PortalCompatProvider: React.FC<{ children?: React.ReactNode }> = props => {
-  const { children } = props;
+const CLASS_NAME_REGEX = new RegExp(`([^\\s]*${fluentProviderClassNames.root}\\w+)`, 'g');
 
+export function useProviderThemeClasses(): string[] {
   const themeClassName = useThemeClassName();
-  const cssVariablesClassName = React.useMemo<string | undefined>(
-    // "themeClassName" may contain multiple classes while we want to add only a class that hosts CSS variables
+  const cssVariablesClasses = React.useMemo<string[]>(
+    // "themeClassName" may contain multiple classes while we want to add only classes that host CSS variables
     // Keep in sync with "packages/react-provider/src/components/FluentProvider/useFluentProviderThemeStyleTag.ts"
-    () => themeClassName.match(CLASS_NAME_REGEX)?.[1],
+    () => themeClassName.match(CLASS_NAME_REGEX) ?? [],
     [themeClassName],
   );
 
-  const registerPortalEl = React.useCallback<RegisterPortalFn>(
-    element => {
-      let disposeFocusVisiblePolyfill: () => void = () => undefined;
-      if (cssVariablesClassName) {
-        element.classList.add(cssVariablesClassName);
-        if (element.ownerDocument.defaultView) {
-          disposeFocusVisiblePolyfill = applyFocusVisiblePolyfill(element, element.ownerDocument.defaultView);
-        }
-      }
-
-      return () => {
-        if (cssVariablesClassName) {
-          element.classList.remove(cssVariablesClassName);
-        }
-        disposeFocusVisiblePolyfill();
-      };
-    },
-    [cssVariablesClassName],
-  );
-
   if (process.env.NODE_ENV !== 'production') {
     // This if statement technically breaks the rules of hooks, but ENV variables never change during app lifecycle
     // eslint-disable-next-line react-hooks/rules-of-hooks
@@ -54,5 +32,29 @@ export const PortalCompatProvider: React.FC<{ children?: React.ReactNode }> = pr
     }, []);
   }
 
+  return cssVariablesClasses;
+}
+
+export const PortalCompatProvider: React.FC<{ children?: React.ReactNode }> = props => {
+  const { children } = props;
+  const cssVariablesClasses = useProviderThemeClasses();
+
+  const registerPortalEl = React.useCallback<RegisterPortalFn>(
+    element => {
+      let disposeFocusVisiblePolyfill: () => void = () => undefined;
+
+      element.classList.add(...cssVariablesClasses);
+      if (element.ownerDocument.defaultView) {
+        disposeFocusVisiblePolyfill = applyFocusVisiblePolyfill(element, element.ownerDocument.defaultView);
+      }
+
+      return () => {
+        element.classList.remove(...cssVariablesClasses);
+        disposeFocusVisiblePolyfill();
+      };
+    },
+    [cssVariablesClasses],
+  );
+
   return <PortalCompatContextProvider value={registerPortalEl}>{children}</PortalCompatContextProvider>;
 };