fix(Toolbar): exclude spacer from overflow popover

packages/main/src/components/Toolbar/OverflowPopover.tsx

@@ -25,7 +25,6 @@ interface OverflowPopoverProps {
   children: ReactNode[];
   portalContainer: Element;
   overflowContentRef: Ref<HTMLDivElement>;
-  numberOfAlwaysVisibleItems?: number;
   overflowPopoverRef?: Ref<PopoverDomRef>;
   overflowButton?: ReactElement<ToggleButtonPropTypes> | ReactElement<ButtonPropTypes>;
   setIsMounted: Dispatch<SetStateAction<boolean>>;
@@ -41,7 +40,6 @@ export const OverflowPopover: FC<OverflowPopoverProps> = (props: OverflowPopover
     children,
     portalContainer,
     overflowContentRef,
-    numberOfAlwaysVisibleItems,
     overflowButton,
     overflowPopoverRef,
     setIsMounted,
@@ -118,43 +116,45 @@ export const OverflowPopover: FC<OverflowPopoverProps> = (props: OverflowPopover
   })();
 
   const OverflowPopoverContextProvider = getOverflowPopoverContext().Provider;
+  const visibleChildren = lastVisibleIndex === -1 ? children : children.slice(lastVisibleIndex + 1);
 
-  let startIndex = null;
-  const filteredChildrenArray = children
+  const filteredChildren = (visibleChildren as ReactElement[])
+    // @ts-expect-error: if type is not defined, it's not a spacer
+    .filter((child) => child.type?.displayName !== 'ToolbarSpacer' && isValidElement(child))
     .map((item, index, arr) => {
-      if (index > lastVisibleIndex && index > numberOfAlwaysVisibleItems - 1 && isValidElement(item)) {
-        if (startIndex === null) {
-          startIndex = index;
-        }
-        const labelProp = item?.props?.['data-accessible-name'] ? 'accessibleName' : 'aria-label';
-        let labelVal = i18nBundle.getText(X_OF_Y, index + 1 - startIndex, arr.length - startIndex);
-        if (item?.props?.[labelProp]) {
-          labelVal += ' ' + item.props[labelProp];
-        }
-        if (item?.props?.id) {
-          return cloneElement<HTMLAttributes<HTMLElement>>(item, {
-            id: `${item.props.id}-overflow`,
-            [labelProp]: labelVal
-          });
-        }
-        // @ts-expect-error: if type is not defined, it's not a spacer
-        if (item.type?.displayName === 'ToolbarSeparator') {
-          return cloneElement(item as ReactElement, {
-            style: {
-              height: '0.0625rem',
-              margin: '0.375rem 0.1875rem',
-              width: '100%'
-            },
-            'aria-label': labelVal
-          });
-        }
+      const labelProp = item?.props?.['data-accessible-name'] ? 'accessibleName' : 'aria-label';
+      let labelVal = i18nBundle.getText(X_OF_Y, index + 1, arr.length);
+      if (item?.props?.[labelProp]) {
+        labelVal += ' ' + item.props[labelProp];
+      }
+
+      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+      // @ts-ignore: React 19
+      if (item?.props?.id) {
         return cloneElement<HTMLAttributes<HTMLElement>>(item, {
+          // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+          // @ts-ignore: React 19
+          id: `${item.props.id}-overflow`,
           [labelProp]: labelVal
         });
       }
-      return null;
-    })
-    .filter(Boolean);
+      // @ts-expect-error: if type is not defined, it's not a separator
+      if (item.type?.displayName === 'ToolbarSeparator') {
+        return cloneElement(item, {
+          // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+          // @ts-ignore: React 19
+          style: {
+            height: '0.0625rem',
+            margin: '0.375rem 0.1875rem',
+            width: '100%'
+          },
+          'aria-label': labelVal
+        });
+      }
+      return cloneElement<HTMLAttributes<HTMLElement>>(item, {
+        [labelProp]: labelVal
+      });
+    });
 
   return (
     <OverflowPopoverContextProvider value={{ inPopover: true }}>
@@ -185,15 +185,15 @@ export const OverflowPopover: FC<OverflowPopoverProps> = (props: OverflowPopover
             onAfterOpen={handleAfterOpen}
             hideArrow
             accessibleRole={accessibleRole}
-            accessibleName={i18nBundle.getText(WITH_X_ITEMS, filteredChildrenArray.length)}
+            accessibleName={i18nBundle.getText(WITH_X_ITEMS, filteredChildren.length)}
           >
             <div
               className={classes.popoverContent}
               ref={overflowContentRef}
               role={a11yConfig?.overflowPopover?.contentRole}
               data-component-name="ToolbarOverflowPopoverContent"
             >
-              {filteredChildrenArray}
+              {filteredChildren}
             </div>
           </Popover>,
           portalContainer ?? document.body

packages/main/src/components/Toolbar/Toolbar.cy.tsx

@@ -79,6 +79,7 @@ const OverflowTestComponent = (props: PropTypes) => {
         <Text data-testid="toolbar-item" style={{ width: '200px' }}>
           Item1
         </Text>
+        {/*not rendered in overflow popover*/}
         <ToolbarSpacer data-testid="spacer1" />
         <Text data-testid="toolbar-item2" style={{ width: '200px' }}>
           Item2
@@ -143,7 +144,7 @@ describe('Toolbar', () => {
     cy.mount(<OverflowTestComponent onOverflowChange={onOverflowChange} />);
     cy.get('@overflowChangeSpy').should('have.been.calledOnce');
     cy.findByTestId('toolbarElements').should('have.text', 2);
-    cy.findByTestId('overflowElements').should('have.text', 4);
+    cy.findByTestId('overflowElements').should('have.text', 3);
     cy.findByText('Item1').should('be.visible');
     cy.get('[data-testid="toolbar-item2"]').should('not.be.visible');
     cy.get('[data-testid="toolbar-item3"]').should('not.be.visible');
@@ -156,22 +157,22 @@ describe('Toolbar', () => {
 
     cy.viewport(500, 500);
 
-    // fuzzy - remount component instead
+    // flaky - remount component instead
     // cy.get(`[ui5-toggle-button]`).click();
     cy.mount(<OverflowTestComponent onOverflowChange={onOverflowChange} />);
     cy.get('[ui5-popover]').should('not.have.attr', 'open');
 
     cy.get('@overflowChangeSpy').should('have.callCount', 2);
     cy.findByTestId('toolbarElements').should('have.text', 3);
-    cy.findByTestId('overflowElements').should('have.text', 3);
+    cy.findByTestId('overflowElements').should('have.text', 2);
 
     cy.findByTestId('input').shadow().find('input').type('100');
     cy.findByTestId('input').trigger('change');
     cy.findByTestId('input').shadow().find('input').clear({ force: true });
 
     cy.get('@overflowChangeSpy').should('have.callCount', 3);
     cy.findByTestId('toolbarElements').should('have.text', 0);
-    cy.findByTestId('overflowElements').should('have.text', 6);
+    cy.findByTestId('overflowElements').should('have.text', 4);
 
     cy.get('[data-testid="toolbar-item"]').should('not.be.visible');
     cy.get('[data-testid="toolbar-item2"]').should('not.be.visible');
@@ -193,13 +194,13 @@ describe('Toolbar', () => {
 
     cy.get('@overflowChangeSpy').should('have.callCount', 5);
     cy.findByTestId('toolbarElements').should('have.text', 3);
-    cy.findByTestId('overflowElements').should('have.text', 3);
+    cy.findByTestId('overflowElements').should('have.text', 2);
 
     cy.findByText('Add').click();
 
     cy.get('@overflowChangeSpy').should('have.callCount', 6);
     cy.findByTestId('toolbarElements').should('have.text', 3);
-    cy.findByTestId('overflowElements').should('have.text', 4);
+    cy.findByTestId('overflowElements').should('have.text', 3);
 
     cy.findByText('Add').click();
     cy.findByText('Add').click();
@@ -209,13 +210,13 @@ describe('Toolbar', () => {
 
     cy.get('@overflowChangeSpy').should('have.callCount', 11);
     cy.findByTestId('toolbarElements').should('have.text', 3);
-    cy.findByTestId('overflowElements').should('have.text', 9);
+    cy.findByTestId('overflowElements').should('have.text', 8);
 
     cy.findByText('Remove').click();
 
     cy.get('@overflowChangeSpy').should('have.callCount', 12);
     cy.findByTestId('toolbarElements').should('have.text', 3);
-    cy.findByTestId('overflowElements').should('have.text', 8);
+    cy.findByTestId('overflowElements').should('have.text', 7);
 
     cy.findByText('Remove').click();
     cy.findByText('Remove').click();
@@ -225,14 +226,14 @@ describe('Toolbar', () => {
 
     cy.get('@overflowChangeSpy').should('have.callCount', 17);
     cy.findByTestId('toolbarElements').should('have.text', 3);
-    cy.findByTestId('overflowElements').should('have.text', 3);
+    cy.findByTestId('overflowElements').should('have.text', 2);
 
     cy.get(`[ui5-toggle-button]`).click();
 
-    // ToolbarSpacers should not be visible in the popover
+    // ToolbarSpacers should not be rendered in the popover
     cy.get('[data-component-name="ToolbarOverflowPopover"]')
       .findByTestId('spacer2')
-      .should('not.be.visible', { timeout: 100 });
+      .should('not.exist', { timeout: 100 });
     cy.findByTestId('spacer1').should('exist');
 
     // ToolbarSeparator should be displayed with horizontal line

packages/main/src/components/Toolbar/index.tsx

@@ -396,11 +396,10 @@ const Toolbar = forwardRef<HTMLDivElement, ToolbarPropTypes>((props, ref) => {
         >
           <OverflowPopover
             overflowPopoverRef={overflowPopoverRef}
-            lastVisibleIndex={lastVisibleIndex}
+            lastVisibleIndex={Math.max(lastVisibleIndex, numberOfAlwaysVisibleItems - 1)}
             classes={classNames}
             portalContainer={portalContainer}
             overflowContentRef={overflowContentRef}
-            numberOfAlwaysVisibleItems={numberOfAlwaysVisibleItems}
             overflowButton={overflowButton}
             setIsMounted={setIsPopoverMounted}
             a11yConfig={a11yConfig}