[Popper] Upgrade floating-ui #2032

lesha1201 · 2023-03-19T17:30:33Z

Description

This PR upgrades major version of @floating-ui/react-dom in Popper component.

Noticeble release notes from Floating UI:

The PR does the following:

Upgrades @floating-ui/react-dom to 2.0.0.
Changes @floating-ui/react-dom from fixed version to minor version. Since Floating UI uses semantic versioning, it should be safe to use minor version and it will reduce a chance of duplicate packages for Floating UI for the end users.
Removes unnecessary logic/workarounds after the upgrade.
Removed filtering falsy values in middleware because now it's done inside @floating-ui/react-dom.

Affected issues:

Fixes Popover - invalid value for --radix-popper-available-width #2128

lesha1201 · 2023-03-19T17:31:08Z

packages/react/popper/src/Popper.tsx

-          transform: isPlaced
-            ? `translate3d(${Math.round(x)}px, ${Math.round(y)}px, 0)`
+          transform: isPositioned
+            ? `translate3d(${Math.round(x as number)}px, ${Math.round(y as number)}px, 0)`


Checking isPositioned makes sure that x and y are not null.

You can use the new floatingStyles object returned from the hook v2 instead of computing the styles here. It performs rounding based on DPR and should be more accurate.

This point: // keep off the page when measuring shouldn't matter because the browser doesn't paint until it's positioned anyway.

the browser doesn't paint until it's positioned anyway

@atomiks Why not? I tried to remove this and in heavily loaded story, the difference is noticable.

Desktop.2023.05.13.-.15.34.14.02.mp4

I see! Yeah, for SSR etc, then keeping it out of view (or using visibility: isPositioned ? undefined : 'hidden') is the correct approach.

@atomiks I kept the logic for hidding it when it's not positioned but changed to use computed value from floatingStyles when it's positioned.

I just noticed that there is translate in floatingStyles and translate3d in Radix. I will change it to translate to make it consistent.

Yeah, the object adds willChange: 'transform' to apply GPU acceleration, making translate act as translate3d. It's only applied to high PPI displays to reduce blurring issues on Windows.

It performs rounding based on DPR and should be more accurate.

@atomiks I'm guessing this is what we're seeing here caught up by Chromatic?

I'd assume so yes

lesha1201 · 2023-03-19T17:32:56Z

packages/react/popper/package.json

I didn't upgrade version for the package because I'm not sure what version it should be. My suggestion is to bump minor version since this component heavily relies on Floating UI but doesn't bring any breaking changes to the end users.

lesha1201 · 2023-04-26T09:38:45Z

@benoitgrelard @andy-hook Sorry for pinging. Can you please review? Does this PR is blocked by something? It would be nice to have this PR released because some projects might already use a new Floating UI version and installing Radix (Popper, Tooltip) results in duplicate packages.

atomiks · 2023-05-11T17:36:50Z

@lesha1201 in the latest version of @floating-ui/react-dom, there are quite a few changes that make a lot of this even simpler. I'll try to review them

atomiks · 2023-05-11T17:37:25Z

packages/react/popper/src/Popper.tsx

    // we need to re-compute the positioning once the parent has finally been placed.
    // https://github.com/floating-ui/floating-ui/issues/1531
    useLayoutEffect(() => {
-      if (isRoot && isPlaced) {
+      if (isRoot && isPositioned) {
        Array.from(positionUpdateFns)
          .reverse()
          .forEach((fn) => requestAnimationFrame(fn));
      }
-    }, [isRoot, isPlaced, positionUpdateFns]);
+    }, [isRoot, isPositioned, positionUpdateFns]);


This should no longer be necessary since @floating-ui/dom@1.2.5

atomiks · 2023-05-11T17:39:35Z

packages/react/popper/src/Popper.tsx

+        // default to `fixed` strategy so users don't have to pick and we also avoid focus scroll issues
+        strategy: 'fixed',


isPositioned lets you wait until the position is ready now and stick to absolute, but that's probably a more intensive change for just upgrading

@atomiks I agree, I would better leave it as it is for now.

isPositioned lets you wait until the position is ready now and stick to absolute, but that's probably a more intensive change for just upgrading

@atomiks, can you tell me more about this, specifically around the absolute strategy? Are you saying absolute should work for everything now? If so, why is there still a strategy option? This is something that we know we want to work out at some point because although fixed works consistently for us, we know it isn't the most performant when scrolling, resizing, etc.

It means that you can prevent scroll jumps using absolute, by doing this:

useLayoutEffect(() => { isPositioned && element.focus(); }, [isPositioned]);

fixed is useful in two cases:

The reference element is in a fixed container (or is fixed itself) and the floating element isn't inside the fixed container. This prevents jitters when scrolling.

"Breaking" the floating element out of a clipping container in many cases (when a containing block isn't being formed) to keep DOM context in tact. Not relevant if you're portalling.

atomiks · 2023-05-11T17:39:59Z

packages/react/popper/src/Popper.tsx

+              contentStyle.setProperty('--radix-popper-available-width', `${availableWidth}px`);
+              contentStyle.setProperty('--radix-popper-available-height', `${availableHeight}px`);
+              contentStyle.setProperty('--radix-popper-anchor-width', `${anchorWidth}px`);
+              contentStyle.setProperty('--radix-popper-anchor-height', `${anchorHeight}px`);


Bugs with this that people have been filing are fixed in the latest version of @floating-ui/dom

@atomiks Can you please give me the context? What bugs?

Oh, I found one issue. Now I get it.

Popover - invalid value for --radix-popper-available-width #2128

Added a story for Popover from the issue.

atomiks · 2023-05-11T17:45:18Z

packages/react/popper/src/Popper.tsx

    // assign the reference dynamically once `Content` has mounted so we can collocate the logic
    useLayoutEffect(() => {
      reference(context.anchor);
    }, [reference, context.anchor]);


You can remove this effect and pass the elements in directly in v2:

useFloating({ elements: {reference: context.anchor}, });

lesha1201 · 2023-05-13T08:00:27Z

@atomiks Thank you for the review! I will it handle it when I can (I hope this weekend).

benoitgrelard

Thanks for doing this both! I'll make a few more changes and test out our side.

benoitgrelard · 2023-05-17T12:26:09Z

packages/react/popover/src/Popover.stories.tsx

@@ -24,6 +24,46 @@ export const Styled = () => {
  );
 };

+// Original issue: https://github.com/radix-ui/primitives/issues/2128
+export const Boundary = () => {


benoitgrelard · 2023-05-17T12:38:03Z

packages/react/popper/src/Popper.tsx

-    // when nested contents are rendered in portals, they are appended out of order causing
-    // children to be positioned incorrectly if initially open.
-    // we need to re-compute the positioning once the parent has finally been placed.
-    // https://github.com/floating-ui/floating-ui/issues/1531


Looks like this issue has been fixed, hence why you removed all this. There's a bit more that can be removed, I'll take care of it.

benoitgrelard · 2023-05-17T18:46:07Z

hmm, weird, some more differences in the latest chromatic run, only in some of the tooltip stories:

Timing?
That's worrying though… 🤔

atomiks · 2023-05-17T19:49:37Z

@benoitgrelard Timing is my guess yes.

Looking locally, all the tooltips paint at the same time (including with 6x slowdown), so I'm not sure how that screenshot is happening.

My guess is that multiple screenshots are being taken, then being stitched together, and some are being taken too early, before the JS measurement logic has run when loading the route. This only coincidentally didn't appear before as maybe it was initializing more quickly.

lesha1201 · 2023-05-17T19:51:46Z

@benoitgrelard I also think that this is timing because the performance is about the same on my machine as well. Maybe we should set delay for that Story to make it more reliable https://www.chromatic.com/docs/delay?

benoitgrelard · 2023-05-17T19:56:53Z

Yeah we might have to introduce some delay to some of those stories. We do this in places already.
Otherwise, I fear we will keep getting some false negatives like this.

Btw, @atomiks, have you seen this my comment here? #2018 (comment)

atomiks · 2023-05-17T19:57:51Z

Yeah I forgot, feel free to open an issue!

benoitgrelard · 2023-05-17T20:09:03Z

Yeah I forgot, feel free to open an issue!

floating-ui/floating-ui#2333

benoitgrelard · 2023-05-19T14:12:49Z

I haven't see anymore false positive like this one, so won't introduce delays yet. If we notice them, we can add them then.

benoitgrelard

Thanks @lesha1201 and @atomiks for this, I've tested my side, and it all looks great. Merging now 🎉

lesha1201 requested review from benoitgrelard and andy-hook as code owners March 19, 2023 17:30

lesha1201 commented Mar 19, 2023

View reviewed changes

atomiks reviewed May 11, 2023

View reviewed changes

lesha1201 force-pushed the chore/upgrade-floating-ui branch from 41cc3bb to 0d99cd1 Compare May 13, 2023 12:57

lesha1201 mentioned this pull request May 13, 2023

Popover - invalid value for --radix-popper-available-width #2128

Closed

benoitgrelard reviewed May 17, 2023

View reviewed changes

lesha1201 and others added 6 commits May 18, 2023 13:20

Upgrade floating-ui

6b3be94

Remove filtering of middlewares in Popper

bd78041

Upgrade floating-ui and handle review comments

bbd4cdc

Use translate instead of translate3d

8896640

Add version file

b288afc

Removed unused context

461afb2

benoitgrelard force-pushed the chore/upgrade-floating-ui branch from c7a6c59 to 461afb2 Compare May 18, 2023 12:30

benoitgrelard approved these changes May 19, 2023

View reviewed changes

benoitgrelard merged commit bf3ad31 into radix-ui:main May 19, 2023
5 checks passed

This was referenced May 30, 2023

[Popover] Content width CSS Variable is not responsive #2019

Closed

Floating UI's Virtual Elements not respected in collisionBoundary #1937

Closed

		// default to `fixed` strategy so users don't have to pick and we also avoid focus scroll issues
		strategy: 'fixed',

[Popper] Upgrade floating-ui #2032

[Popper] Upgrade floating-ui #2032

Conversation

lesha1201 commented Mar 19, 2023 • edited by benoitgrelard

Description

lesha1201 Mar 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atomiks May 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesha1201 commented Apr 26, 2023 • edited

atomiks commented May 11, 2023

atomiks May 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesha1201 May 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesha1201 commented May 13, 2023

benoitgrelard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitgrelard commented May 17, 2023

atomiks commented May 17, 2023

lesha1201 commented May 17, 2023

benoitgrelard commented May 17, 2023

atomiks commented May 17, 2023

benoitgrelard commented May 17, 2023

benoitgrelard commented May 19, 2023

benoitgrelard left a comment

Choose a reason for hiding this comment

lesha1201 commented Mar 19, 2023 •

edited by benoitgrelard

lesha1201 Mar 19, 2023 •

edited

atomiks May 13, 2023 •

edited

lesha1201 commented Apr 26, 2023 •

edited

atomiks May 11, 2023 •

edited

lesha1201 May 13, 2023 •

edited