fix: Overflow update should run show/hide steps twice (microsoft#28628)

* fix: Overflow update should run show/hide steps twice

When new items are added to the overflow manager, they are always
visible by default. This might not actually be the 'correct' state
especially considering priorities.

Consider and item that is added whose priority is less than the top of
the invisible item queue - it should actually not be visible. This can
negatively affect the overflow calculation (which is batched)

In order to avoid these other solutions for their negative perf and
complexity:
* Add more logic to addItem to account for an 'invalid' state
* Call `forceUpdate` every time an item is added

This PR simply changes the logic to run the hide/show step twice. Which
will resolve any new items to their correct state first. In order to
improve performance a cache for the offset size was added. This improves
the performance even compared to the current version since `offsetWidth`
and `offsetHeight` can be called multiple times for a single element in
an overflow update.

* changefile

* fix lint

* revert story

change/@fluentui-priority-overflow-c98cbac7-b421-4898-a6b9-cd0cf7dfb500.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "fix: Overflow update should run show/hide steps twice",
+  "packageName": "@fluentui/priority-overflow",
+  "email": "lingfan.gao@microsoft.com",
+  "dependentChangeType": "patch"
+}

change/@fluentui-react-overflow-87d68562-a91a-45c0-bdd9-133e3a7d2616.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "fix: Overflow update should run show/hide steps twice",
+  "packageName": "@fluentui/react-overflow",
+  "email": "lingfan.gao@microsoft.com",
+  "dependentChangeType": "patch"
+}

packages/react-components/priority-overflow/src/overflowManager.ts

@@ -14,6 +14,9 @@ import type {
  * @returns overflow manager instance
  */
 export function createOverflowManager(): OverflowManager {
+  // calls to `offsetWidth or offsetHeight` can happen multiple times in an update
+  // Use a cache to avoid causing too many recalcs and avoid scripting time to meausure sizes
+  const sizeCache = new Map<HTMLElement, number>();
   let container: HTMLElement | undefined;
   let overflowMenu: HTMLElement | undefined;
   // Set as true when resize observer is observing
@@ -136,7 +139,13 @@ export function createOverflowManager(): OverflowManager {
   });
 
   const getOffsetSize = (el: HTMLElement) => {
-    return options.overflowAxis === 'horizontal' ? el.offsetWidth : el.offsetHeight;
+    if (sizeCache.has(el)) {
+      return sizeCache.get(el)!;
+    }
+
+    const offsetSize = options.overflowAxis === 'horizontal' ? el.offsetWidth : el.offsetHeight;
+    sizeCache.set(el, offsetSize);
+    return offsetSize;
   };
 
   function computeSizeChange(entry: OverflowItemEntry) {
@@ -193,6 +202,8 @@ export function createOverflowManager(): OverflowManager {
     if (!container) {
       return false;
     }
+    sizeCache.clear();
+
     const totalDividersWidth = Object.values(overflowDividers)
       .map(dvdr => (dvdr.groupId ? getOffsetSize(dvdr.element) : 0))
       .reduce((prev, current) => prev + current, 0);
@@ -207,20 +218,24 @@ export function createOverflowManager(): OverflowManager {
     const visibleTop = visibleItemQueue.peek();
     const invisibleTop = invisibleItemQueue.peek();
 
-    let currentWidth = visibleItemQueue
+    let currentSize = visibleItemQueue
       .all()
       .map(id => overflowItems[id].element)
       .map(getOffsetSize)
       .reduce((prev, current) => prev + current, 0);
 
-    // Add items until available width is filled - can result in overflow
-    while (currentWidth + overflowMenuSize() < availableSize && invisibleItemQueue.size() > 0) {
-      currentWidth += showItem();
-    }
+    // Run the show/hide step twice - the first step might not be correct if
+    // it was triggered by a new item being added - new items are always visible by default.
+    for (let i = 0; i < 2; i++) {
+      // Add items until available width is filled - can result in overflow
+      while (currentSize + overflowMenuSize() < availableSize && invisibleItemQueue.size() > 0) {
+        currentSize += showItem();
+      }
 
-    // Remove items until there's no more overflow
-    while (currentWidth + overflowMenuSize() > availableSize && visibleItemQueue.size() > options.minimumVisible) {
-      currentWidth -= hideItem();
+      // Remove items until there's no more overflow
+      while (currentSize + overflowMenuSize() > availableSize && visibleItemQueue.size() > options.minimumVisible) {
+        currentSize -= hideItem();
+      }
     }
 
     // only update when the state of visible/invisible items has changed
@@ -247,6 +262,7 @@ export function createOverflowManager(): OverflowManager {
 
   const disconnect: OverflowManager['disconnect'] = () => {
     observing = false;
+    sizeCache.clear();
     resizeObserver.disconnect();
   };
 
@@ -316,6 +332,7 @@ export function createOverflowManager(): OverflowManager {
       item.element.removeAttribute(DATA_OVERFLOW_GROUP);
     }
 
+    sizeCache.delete(item.element);
     delete overflowItems[itemId];
     update();
   };

packages/react-components/react-overflow/src/Overflow.cy.tsx

@@ -9,7 +9,6 @@ import {
   useIsOverflowGroupVisible,
   useOverflowMenu,
   useOverflowContext,
-  useIsOverflowItemVisible,
 } from '@fluentui/react-overflow';
 import { Portal } from '@fluentui/react-portal';
 
@@ -641,34 +640,50 @@ describe('Overflow', () => {
     cy.contains('Update priority').click().get('#foo-visibility').should('have.text', 'visible');
   });
 
-  it('Should have correct initial visibility state', () => {
-    const mapHelper = new Array(10).fill(0).map((_, i) => i);
-    const Assert = () => {
-      const isVisible = mapHelper.map(i => {
-        // eslint-disable-next-line react-hooks/rules-of-hooks
-        return useIsOverflowItemVisible(i.toString());
-      });
-
-      if (isVisible.every(x => x)) {
-        return <span data-passed="true" />;
-      }
-
-      return null;
+  it('Should switch priorities and use all available space', () => {
+    const Example = () => {
+      const [selected, setSelelected] = React.useState(0);
+      return (
+        <>
+          <Container>
+            {mapHelper.map(i => (
+              <Item key={i} id={i.toString()} priority={selected === i ? 1000 : 0} width={i === 9 ? 100 : undefined}>
+                <span onClick={() => setSelelected(i)} style={{ color: selected === i ? 'red' : 'black' }}>
+                  {i}
+                </span>
+              </Item>
+            ))}
+            <Menu />
+          </Container>
+          <div>
+            <button id="select-9" onClick={() => setSelelected(9)}>
+              Select 9
+            </button>
+            <button id="select-0" onClick={() => setSelelected(0)}>
+              Select 0
+            </button>
+          </div>
+        </>
+      );
     };
 
-    mount(
-      <Container minimumVisible={5}>
-        {mapHelper.map(i => (
-          <Item key={i} id={i.toString()}>
-            {i}
-          </Item>
-        ))}
-        <Menu />
-        <Assert />
-      </Container>,
-    );
+    const mapHelper = new Array(10).fill(0).map((_, i) => i);
+    mount(<Example />);
 
-    setContainerSize(500);
-    cy.get('[data-passed="true"]').should('exist');
+    cy.get(`[${selectors.item}="3"]`).click();
+    setContainerSize(250);
+    cy.get('#select-9').click();
+    cy.get(`[${selectors.item}="9"]`).should('be.visible');
+    cy.get(`[${selectors.item}="0"]`).should('be.visible');
+    cy.get(`[${selectors.item}="1"]`).should('be.visible');
+    cy.get(`[${selectors.item}="2"]`).should('not.be.visible');
+    cy.get(`[${selectors.item}="3"]`).should('not.be.visible');
+
+    cy.get('#select-0').click();
+    cy.get(`[${selectors.item}="9"]`).should('not.be.visible');
+    cy.get(`[${selectors.item}="0"]`).should('be.visible');
+    cy.get(`[${selectors.item}="1"]`).should('be.visible');
+    cy.get(`[${selectors.item}="2"]`).should('be.visible');
+    cy.get(`[${selectors.item}="3"]`).should('be.visible');
   });
 });