tailwindlabs / headlessui

Completely unstyled, fully accessible UI components, designed to integrate beautifully with Tailwind CSS.
https://headlessui.com
MIT License
25.81k stars 1.07k forks source link

Regression in `@headlessui/react@2.1.x` when using `<TransitionChild>` with `<Dialog>` for the backdrop #3467

Closed breadadams closed 3 weeks ago

breadadams commented 3 weeks ago

What package within Headless UI are you using?

@headlessui/react

What version of that package are you using?

v2.1.5

What browser are you using?

Chrome

Reproduction URL

Describe your issue

We are using the <Transition> + <TransitionChild> approach for animating <Dialog> components. After updating to v2.1.5 the backdrop animation is no-longer working.

Due to how we have a custom theme structured, we don't use alpha channel colors, and therefore use opacity-X on the backdrop <div>. Prior to v2.1.x this worked correctly.

Steps to reproduce:

  1. Click the "Open Modal" button.
  2. See how the backdrop doesn't fade in, however <DialogPanel> animates in correctly.
  3. Close the modal via the "Got it, thanks!" button.
  4. See how the backdrop doesn't fade out, however <DialogPanel> animates out correctly.

In an attempt to locate the problem I've also added a 2nd button in the Codesandboxes, "Open Simple Overlay". The component this triggers doesn't include any <Dialog> related pieces, just a <Transition> + <TransitionChild>. Notice how the animation works correctly here.

Worth mentioning that in the v2.0.4 example the "Simple Overlay" only actually animates-in, the animate-out doesn't work. In v2.1.x it animates in and out correctly.

Ideally we'd move to the <DialogBackdrop> component, however this feels like a regression, since AFAIK the use of <Transition> & <TransitionChild> with <Dialog> isn't deprecated.

It also seems that <DialogBackdrop> doesn't have an equivalent to the appear prop from <Transition>, although maybe I'm missing that somewhere in the docs and/or release notes?

MNeverOff commented 3 weeks ago

Having a similar issue, also mentioned here: https://github.com/tailwindlabs/headlessui/issues/3437#issuecomment-2338989331

RobinMalfait commented 3 weeks ago

Hey!

Thanks for the bug report, we recently made a lot of improvements to the Transition component. Instead of relying on manual DOM mutation, we just use React to add additional classes (from the enterFrom, enterTo, … props) based on the current transition state, without changing any of the default classes that are already applied.

The only thing we have to focus on is this:

<TransitionChild
  enter="ease-out duration-300"
  enterFrom="opacity-0"
  enterTo="opacity-50"
  leave="ease-in duration-200"
  leaveFrom="opacity-50"
  leaveTo="opacity-0"
>
  <div className="fixed inset-0 bg-black opacity-50" />
</TransitionChild>

We won't touch the existing classes of the main element:

<div className="fixed inset-0 bg-black opacity-50" />

Which means that at some point, during a transition both the opacity-0 and opacity-50 will be present at the exact same time. Since the order of classes in the HTML doesn't matter it means that the same opacity will be applied at all times (aka, no transition is happening).

If I'm reading this correctly, my assumption is that you want to fade in the element, but it should only be at 50% black such that it's not fully black once the transition completes.

To solve this, you can remove the conflicting classes (opacity-50)

  <TransitionChild
    enter="ease-out duration-300"
    enterFrom="opacity-0"
    enterTo="opacity-50"
    leave="ease-in duration-200"
    leaveFrom="opacity-50"
    leaveTo="opacity-0"
  >
-   <div className="fixed inset-0 bg-black opacity-50" />
+   <div className="fixed inset-0 bg-black" />
  </TransitionChild>

This means that the transition happens from 0% to 50%, but once it's fully done, it will be at 100%.

That's not what you want of course, so the next thing you can do is make the background color always be at 50% background opacity:

  <TransitionChild
    enter="ease-out duration-300"
    enterFrom="opacity-0"
    enterTo="opacity-50"
    leave="ease-in duration-200"
    leaveFrom="opacity-50"
    leaveTo="opacity-0"
  >
-   <div className="fixed inset-0 bg-black" />
+   <div className="fixed inset-0 bg-black/50" />
  </TransitionChild>

This now ensures that the background will be at 50% background opacity at the end. But the bad part is that during the transition it would only be at 25% (because 50% opacity, 50% background color opacity)

To solve that, we can make the transition go from 0% to 100%:

  <TransitionChild
    enter="ease-out duration-300"
    enterFrom="opacity-0"
-   enterTo="opacity-50"
+   enterTo="opacity-100"
    leave="ease-in duration-200"
-   leaveFrom="opacity-50"
+   leaveTo="opacity-100"
    leaveTo="opacity-0"
  >
    <div className="fixed inset-0 bg-black/50" />
  </TransitionChild>

The final result looks like this:

<TransitionChild
  enter="ease-out duration-300"
  enterFrom="opacity-0"
  enterTo="opacity-100"
  leave="ease-in duration-200"
  leaveFrom="opacity-100"
  leaveTo="opacity-0"
>
  <div className="fixed inset-0 bg-black/50" />
</TransitionChild>

This now means that:

  1. The transition will happen because the opacity values will be different
  2. The transition happens from 0% -> 100% completely
  3. The background color will always be at 50% background opacity
  4. There are no conflicting classes anymore

The DialogBackdrop component doesn't have an appear prop, but if you apply it on the TransitionChild component it should behave as expected.

Alternatively, you can migrate to the transition prop instead of the Transition component. E.g.: https://headlessui.com/react/dialog#adding-transitions

But as you mentioned, the Transition and TransitionChild components are indeed not deprecated.

Hope this helps!

breadadams commented 3 weeks ago

Hey @RobinMalfait, thanks for the detailed reply. Unfortunately that won't work for us due to what I mentioned in the issue description:

Due to how we have a custom theme structured, we don't use alpha channel colors, and therefore use opacity-X on the backdrop <div>. Prior to v2.1.x this worked correctly.

So basically we can't do bg-overlay/50. Is there any alternative without needing alpha channels?


As for this part of your comment:

The DialogBackdrop component doesn't have an appear prop, but if you apply it on the TransitionChild component it should behave as expected.

Alternatively, you can migrate to the transition prop instead of the Transition component.

So what I meant was migrating to transition prop - not using DialogBackdrop with TransitionChild. However the transition prop doesn't support animating on initial render, or am I mistaken?

RobinMalfait commented 2 weeks ago

@breadadams

So basically we can't do bg-overlay/50. Is there any alternative without needing alpha channels?

I completely missed this part, my apologies. One solution is to use bg-[#000]/50 and not rely on colors from your theme. Another solution is using color-mix() (which we use in v4) which allows you to mix a color with a transparent color.

E.g.: bg-black/50 would map to this in v4:

/* Specificity: (0, 1, 0) */
.bg-black\/50 {
  background-color: color-mix(
    in srgb,
    var(--color-black, #000) 50%,
    transparent
  );
}

This might be too tricky to implement in a v3 codebase right now.

Another solution, while not pretty, is to make use of !opacity-0 so that those classes win over the opacity-50 that was already applied.

So what I meant was migrating to transition prop - not using DialogBackdrop with TransitionChild. However the transition prop doesn't support animating on initial render, or am I mistaken?

Ah yep that's correct, there is no appear equivalent. But typically you don't land on a page where the Dialog is open and you want a transition happening as well.

Either way, Transition and TransitionChild are not deprecated and should work 👍

breadadams commented 2 weeks ago

No worries, @RobinMalfait! Hmm I see, that's unfortunate, we don't want to not use a theme variable here, and using the !important modifier isn't pretty either. However I'll check with our team and see if there's something we can do here, although I'd guess there are more users with a similar approach as us that will be affected by this minor difference.

[...] there is no appear equivalent. But typically you don't land on a page where the Dialog is open and you want a transition happening as well.

I don't think that's entirely true. AFAIK it's a common approach to use a Dialog (as a modal or a slideover) for a route, i.e. with react-router-dom. That's the case we have, where for example visiting a route opens a slideover.

If we were to migrate to the transition prop (I'd really like to), there's no way to maintain this behaviour. Would it be possible to get an equivalent with the new transition prop?