feat(react-menu, react-positioning): update maxSize to prevent double…

… scrollbar for Menu in small viewport (#28622) * fix * conditional overflowy * upd * chg * api * use overflow scroll in test * f * scroll->auto
microsoft · Jul 25, 2023 · e59b4b3 · e59b4b3
1 parent c0c62f9
commit e59b4b3
Show file tree

Hide file tree

Showing 10 changed files with 137 additions and 21 deletions.
diff --git a/apps/vr-tests-react-components/src/stories/Menu/ScrollableMenuSmallViewport.stories.tsx b/apps/vr-tests-react-components/src/stories/Menu/ScrollableMenuSmallViewport.stories.tsx
@@ -0,0 +1,85 @@
+import * as React from 'react';
+
+import { Menu, MenuTrigger, MenuList, MenuItem, MenuPopover, MenuGroup, MenuDivider } from '@fluentui/react-menu';
+import { makeStyles, shorthands } from '@griffel/react';
+import { PositioningProps } from '@fluentui/react-positioning';
+import { Steps, StoryWright } from 'storywright';
+const useStyles = makeStyles({
+  wrapper: { display: 'flex' },
+  shortContainer: {
+    width: '200px',
+    height: '220px',
+    ...shorthands.border('2px', 'dashed', 'red'),
+    ...shorthands.padding('10px'),
+  },
+  longContainer: {
+    width: '200px',
+    height: '400px',
+    ...shorthands.border('2px', 'dashed', 'green'),
+    ...shorthands.padding('10px'),
+  },
+  scrollableMenuGroup: {
+    maxHeight: '150px',
+    overflowY: 'auto',
+  },
+  menuPopover: {
+    overflowX: 'hidden',
+  },
+});
+
+const ScrollableMenu = ({ overflowBoundary }: Pick<PositioningProps, 'overflowBoundary'>) => {
+  const styles = useStyles();
+  return (
+    <Menu open positioning={{ overflowBoundary, flipBoundary: overflowBoundary, autoSize: true }}>
+      <MenuTrigger disableButtonEnhancement>
+        <button>Menu</button>
+      </MenuTrigger>
+
+      <MenuPopover className={styles.menuPopover}>
+        <MenuList>
+          <MenuGroup className={styles.scrollableMenuGroup}>
+            <MenuItem>Cut</MenuItem>
+            <MenuItem>Paste</MenuItem>
+            <MenuItem>Edit</MenuItem>
+            <MenuItem>Undo</MenuItem>
+            <MenuItem>Redo</MenuItem>
+            <MenuItem disabled>Open File</MenuItem>
+            <MenuItem>Open Folder</MenuItem>
+          </MenuGroup>
+          <MenuDivider />
+          <MenuItem>New </MenuItem>
+          <MenuItem>New Window</MenuItem>
+        </MenuList>
+      </MenuPopover>
+    </Menu>
+  );
+};
+
+const Example = () => {
+  const styles = useStyles();
+  const [shortOverflowBoundary, setShortOverflowBoundary] = React.useState<HTMLElement | null>(null);
+  const [longOverflowBoundary, setLongOverflowBoundary] = React.useState<HTMLElement | null>(null);
+
+  return (
+    <StoryWright steps={steps}>
+      <div className={styles.wrapper}>
+        <div className={styles.shortContainer} ref={setShortOverflowBoundary}>
+          <div>Short viewport:</div>
+          <ScrollableMenu overflowBoundary={shortOverflowBoundary} />
+        </div>
+
+        <div className={styles.longContainer} ref={setLongOverflowBoundary}>
+          <div>Long viewport:</div>
+          <ScrollableMenu overflowBoundary={longOverflowBoundary} />
+        </div>
+      </div>
+    </StoryWright>
+  );
+};
+
+const steps = new Steps().snapshot('default').end();
+export const ScrollableMenuSmallViewport = () => (
+  <StoryWright steps={steps}>
+    <Example />
+  </StoryWright>
+);
diff --git a/apps/vr-tests-react-components/src/stories/Menu/index.stories.tsx b/apps/vr-tests-react-components/src/stories/Menu/index.stories.tsx
@@ -3,6 +3,7 @@ import { ComponentMeta } from '@storybook/react';
 
 export { NestedSubmenusSmallViewportStacked } from './NestedMenuSmallViewportStacked.stories';
 export { NestedSubmenusSmallViewportFlipped } from './NestedMenuSmallViewportFlipped.stories';
+export { ScrollableMenuSmallViewport } from './ScrollableMenuSmallViewport.stories';
 
 export default {
   title: 'Menu',

diff --git a/change/@fluentui-react-menu-03f8268f-d187-4f13-ac82-48548dfa3c97.json b/change/@fluentui-react-menu-03f8268f-d187-4f13-ac82-48548dfa3c97.json
@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "feat: update style to prevent double scrollbar",
+  "packageName": "@fluentui/react-menu",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/change/@fluentui-react-positioning-f6515d84-5f8b-4649-90dc-7d0cf179d176.json b/change/@fluentui-react-positioning-f6515d84-5f8b-4649-90dc-7d0cf179d176.json
@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "feat: update maxSize middleware to apply height/width when overflow",
+  "packageName": "@fluentui/react-positioning",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/react-components/react-menu/etc/react-menu.api.md b/packages/react-components/react-menu/etc/react-menu.api.md
@@ -241,6 +241,7 @@ export type MenuListState = ComponentState<MenuListSlots> & Required<Pick<MenuLi
     selectRadio: SelectableHandler;
     setFocusByFirstCharacter: NonNullable<MenuListContextValue['setFocusByFirstCharacter']>;
     toggleCheckbox: SelectableHandler;
+    hasMenuContext?: boolean;
 };
 
 // @public

diff --git a/packages/react-components/react-menu/src/components/MenuItem/useMenuItemStyles.styles.ts b/packages/react-components/react-menu/src/components/MenuItem/useMenuItemStyles.styles.ts
@@ -27,6 +27,7 @@ const useStyles = makeStyles({
     paddingRight: '6px',
     paddingLeft: '6px',
     height: '32px',
+    minHeight: '32px',
     display: 'flex',
     alignItems: 'center',
     fontSize: tokens.fontSizeBase300,

diff --git a/packages/react-components/react-menu/src/components/MenuList/MenuList.types.ts b/packages/react-components/react-menu/src/components/MenuList/MenuList.types.ts
@@ -63,6 +63,11 @@ export type MenuListState = ComponentState<MenuListSlots> &
      * Toggles the state of a checkbox item
      */
     toggleCheckbox: SelectableHandler;
+
+    /**
+     * States if the MenuList is inside MenuContext
+     */
+    hasMenuContext?: boolean;
   };
 
 export type MenuListContextValues = {

diff --git a/packages/react-components/react-menu/src/components/MenuList/useMenuList.ts b/packages/react-components/react-menu/src/components/MenuList/useMenuList.ts
@@ -118,6 +118,7 @@ export const useMenuList_unstable = (props: MenuListProps, ref: React.Ref<HTMLEl
     hasIcons: menuContext.hasIcons || false,
     hasCheckmarks: menuContext.hasCheckmarks || false,
     checkedValues,
+    hasMenuContext,
     setFocusByFirstCharacter,
     selectRadio,
     toggleCheckbox,

diff --git a/packages/react-components/react-menu/src/components/MenuList/useMenuListStyles.styles.ts b/packages/react-components/react-menu/src/components/MenuList/useMenuListStyles.styles.ts
@@ -12,13 +12,21 @@ const useStyles = makeStyles({
     flexDirection: 'column',
     ...shorthands.gap('2px'),
   },
+  hasMenuContext: {
+    height: '100%',
+  },
 });
 
 /**
  * Apply styling to the Menu slots based on the state
  */
 export const useMenuListStyles_unstable = (state: MenuListState): MenuListState => {
   const styles = useStyles();
-  state.root.className = mergeClasses(menuListClassNames.root, styles.root, state.root.className);
+  state.root.className = mergeClasses(
+    menuListClassNames.root,
+    styles.root,
+    state.hasMenuContext && styles.hasMenuContext,
+    state.root.className,
+  );
   return state;
 };
diff --git a/packages/react-components/react-positioning/src/middleware/maxSize.ts b/packages/react-components/react-positioning/src/middleware/maxSize.ts
@@ -11,30 +11,30 @@ export function maxSize(autoSize: PositioningOptions['autoSize'], options: MaxSi
   return size({
     ...(overflowBoundary && { altBoundary: true, boundary: getBoundary(container, overflowBoundary) }),
     apply({ availableHeight, availableWidth, elements, rects }) {
-      const applyMaxWidth =
-        autoSize === 'always' ||
-        autoSize === 'width-always' ||
-        (rects.floating.width > availableWidth && (autoSize === true || autoSize === 'width'));
+      if (autoSize) {
+        elements.floating.style.setProperty('box-sizing', 'border-box');
+      }
+
+      const applyMaxWidth = autoSize === 'always' || autoSize === 'width-always';
+      const widthOverflow = rects.floating.width > availableWidth && (autoSize === true || autoSize === 'width');
 
-      const applyMaxHeight =
-        autoSize === 'always' ||
-        autoSize === 'height-always' ||
-        (rects.floating.height > availableHeight && (autoSize === true || autoSize === 'height'));
+      const applyMaxHeight = autoSize === 'always' || autoSize === 'height-always';
+      const heightOverflow = rects.floating.height > availableHeight && (autoSize === true || autoSize === 'height');
 
-      if (applyMaxHeight) {
-        Object.assign<CSSStyleDeclaration, Partial<CSSStyleDeclaration>>(elements.floating.style, {
-          maxHeight: `${availableHeight}px`,
-          boxSizing: 'border-box',
-          overflowY: 'auto',
-        });
+      if (applyMaxHeight || heightOverflow) {
+        elements.floating.style.setProperty('max-height', `${availableHeight}px`);
+      }
+      if (heightOverflow) {
+        elements.floating.style.setProperty('height', `${availableHeight}px`);
+        elements.floating.style.setProperty('overflow-y', 'auto');
       }
 
-      if (applyMaxWidth) {
-        Object.assign<CSSStyleDeclaration, Partial<CSSStyleDeclaration>>(elements.floating.style, {
-          maxWidth: `${availableWidth}px`,
-          boxSizing: 'border-box',
-          overflowX: 'auto',
-        });
+      if (applyMaxWidth || widthOverflow) {
+        elements.floating.style.setProperty('max-width', `${availableWidth}px`);
+      }
+      if (widthOverflow) {
+        elements.floating.style.setProperty('width', `${availableWidth}px`);
+        elements.floating.style.setProperty('overflow-x', 'auto');
       }
     },
   });