Dialog "enter" animation doesn't work since new animate API

[Grsmto]

🐛 Bug report

Current behavior

The "enter" animation used on a Dialog component does not work with the new API.

Steps to reproduce the bug

Provide a repo or sandbox with the bug and describe the steps to reproduce it.

1. Open sandbox: https://codesandbox.io/s/reakit-enterleave-animation-jyti1
2. Click on open/close

Note: this can also be reproduced on the Reakit website.

Expected behavior

"Enter" doesn't get triggered in some scenarios:

• On Chrome: seems to be quite random. Sometimes it doesn't work on the very first click (after page load).
• On Firefox: works when opening/closing with the keyboard "enter" key, but not with a mouse click.

Environment

MacOS 10.15.2

Firefox 76.0.1
Chrome 81.0.40

Reakit 1.0.0

Note: It was working up until reakit 1.0.0-beta.16

[open-collective-bot]

Hey @Grsmto 👋,

Thank you for opening an issue. We'll get back to you as soon as we can.
Please, consider supporting us on Open Collective. We give a special attention to issues opened by backers.

If you use Reakit at work, you can also ask your company to sponsor us ❤️.

[diegohaz]

Thanks for reporting it, @Grsmto!

Do you know if it works on 1.0.0-rc.2?

[Grsmto]

Seems to be working fine on 1.0.0-rc.2! 😮

[diegohaz]

I'm not sure what's happening, but maybe we will have to use double requestAnimationFrame here: https://github.com/reakit/reakit/blob/a17f5115fde786e22681fdfa2368f8c91980e59c/packages/reakit/src/Disclosure/DisclosureContent.ts#L48-L57

Possibly related to https://stackoverflow.com/questions/44145740/how-does-double-requestanimationframe-work

I just don't know if we can cancel the nested animation frame in the effect cleanup callback.

[Grsmto]

Right. I would have guess that code to be enough to "wait for the next render" before adding the enter class...but seems like it's not. Not sure how, maybe renders get batched since RAF is being used?!

But maybe this would do the trick?:

React.useEffect(() => {
  if (!options.animated) return undefined;

  let raf;

  window.setTimeout(() => {
    raf = window.requestAnimationFrame(() => {
      if (options.visible) {
        setTransition("enter");
      } else if (animating) {
        setTransition("leave");
      } else {
        setTransition(null);
      }
    });
  }, 0);

  return () => window.cancelAnimationFrame(raf);
}, [options.animated, options.visible, animating]);

I'm not sure setTimeout needs cleaning since it's 0.

I think this would be the exact same thing as the double RAF trick.
I would say a setTimeout 0 is clearer as to "wait for the next frame" than a nested RAF, (and I don't think there is any perf impact, since the code being called ends up being in a RAF anyway) but idk maybe there are behaviour differences...