Skip to content

Conversation

@EmilNordling
Copy link

@EmilNordling EmilNordling commented Nov 23, 2025

Possible fix for: #3308

Writing possible because:

  1. The test might be valid, if it's actually is running Safari during testing.
  2. Moving the timing might not be the correct approach of solving the bug. But it does end up in the expected behavior, see the video below:
Screen.Recording.2025-11-23.at.18.46.51.mov
  1. There seems to be some fishy memory leak? Because if getCssDimensions is invoked before sizeFrame1.request, it'll get into the buggy state again.

    Here's an recording that shows how it's behaving on master, then on this PR's change and lastly adding a getCssDimensions call before sizeFrame1.request executes:
Screen.Recording.2025-11-23.at.18.51.22.mov

This is really surprised me. Something with invoking getCssDimensions causes it to behave differently.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 23, 2025

vite-css-base-ui-example

pnpm add https://pkg.pr.new/mui/base-ui/@base-ui-components/react@3309
pnpm add https://pkg.pr.new/mui/base-ui/@base-ui-components/utils@3309

commit: ad68b69

@mui-bot
Copy link

mui-bot commented Nov 23, 2025

Bundle size report

Bundle Parsed size Gzip size
@base-ui-components/react 🔺+47B(+0.01%) 🔺+14B(+0.01%)

Details of bundle changes


Check out the code infra dashboard for more information about this PR.

@netlify
Copy link

netlify bot commented Nov 23, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit ad68b69
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/69246a6c28aa9a0008c49271
😎 Deploy Preview https://deploy-preview-3309--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@atomiks
Copy link
Contributor

atomiks commented Nov 24, 2025

For some reason, I can't reproduce the original issue on Safari at all (26.1)

Moving the timing might not be the correct approach of solving the bug

It ends up breaking the transitions of the size of the popup, so doesn't seem so.

Popover.Viewport/Tooltip.Viewport have a different technique of solving size transitions, it's possible they don't have the bug, but I can't reproduce it it to begin with


It's possible handleOpenEvent needs reworking. From what I see locally it always "self-corrects" via the layout effect where handleValueChange(0, 0) is called (it seems not for you?), but handleOpenEvent gets called with 0 width/height initially.

Falling back may help:

  const handleValueChange = useStableCallback((currentWidth: number, currentHeight: number) => {
    if (!popupElement || !positionerElement) {
      return;
    }

    popupElement.style.removeProperty(NavigationMenuPopupCssVars.popupWidth);
    popupElement.style.removeProperty(NavigationMenuPopupCssVars.popupHeight);
    positionerElement.style.removeProperty(NavigationMenuPositionerCssVars.positionerWidth);
    positionerElement.style.removeProperty(NavigationMenuPositionerCssVars.positionerHeight);

    const { width, height } = getCssDimensions(popupElement);
    const measuredWidth = width || prevSizeRef.current.width;
    const measuredHeight = height || prevSizeRef.current.height;

    if (currentHeight === 0 || currentWidth === 0) {
      currentWidth = measuredWidth;
      currentHeight = measuredHeight;
    }

    popupElement.style.setProperty(NavigationMenuPopupCssVars.popupWidth, `${currentWidth}px`);
    popupElement.style.setProperty(NavigationMenuPopupCssVars.popupHeight, `${currentHeight}px`);
    positionerElement.style.setProperty(
      NavigationMenuPositionerCssVars.positionerWidth,
      `${measuredWidth}px`,
    );
    positionerElement.style.setProperty(
      NavigationMenuPositionerCssVars.positionerHeight,
      `${measuredHeight}px`,
    );

    sizeFrame1.request(() => {
      popupElement.style.setProperty(NavigationMenuPopupCssVars.popupWidth, `${measuredWidth}px`);
      popupElement.style.setProperty(
        NavigationMenuPopupCssVars.popupHeight,
        `${measuredHeight}px`,
      );

      sizeFrame2.request(() => {
        animationAbortControllerRef.current = new AbortController();
        runOnceAnimationsFinish(setAutoSizes, animationAbortControllerRef.current.signal);
      });
    });
  });

Or figuring out why the self-correction via the layout effect doesn't occur in your case

@EmilNordling
Copy link
Author

For some reason, I can't reproduce the original issue on Safari at all (26.1)

Sorry I forgot to add which version I'm experiencing the behavior at. I'm running the version before macOS 26. Seems to only be an issue on ~18.1.1 (20619.2.8.11.12) in that case!


Falling back may help

Tried to apply the above, it'll not fallback to anything with a size unfortunately. In the scope the width, currentWidth and prevSizeRef.current.width will all be set to zero in this Safari version.

Though, and if it makes sense to change, it'll self-correct if it wouldn't reset the prevSizeRef to DEFAULT_SIZE.

React.useEffect(() => {
    if (!open) {
      stickIfOpenTimeout.clear();
      sizeFrame1.cancel();
      sizeFrame2.cancel();
-     prevSizeRef.current = DEFAULT_SIZE;
    }
  }, [stickIfOpenTimeout, open, sizeFrame1, sizeFrame2]);
Screen.Recording.2025-11-24.at.09.21.52.mov

Seems like figuring out why popupElement yields a faulty width value in the version ~18 of Safari is wanted. I'm going to see if I can fish something out!

@atomiks
Copy link
Contributor

atomiks commented Nov 24, 2025

Try changing prevSizeRef to be set to the default on mounted=false instead of open=false? If it fixes it, that seems fine.

@zannager zannager added the component: navigation menu Changes related to the navigation menu component. label Nov 24, 2025
@EmilNordling
Copy link
Author

... to be set to the default on mounted=false

That does work!

Screen.Recording.2025-11-24.at.15.25.48.mov

@atomiks
Copy link
Contributor

atomiks commented Nov 24, 2025

Alright, nice! The other side effects still need to be done with open=false though, so an independent effect seems necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: navigation menu Changes related to the navigation menu component.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants