Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transition resolves instantly sometimes #3126

Closed
twiddler opened this issue Dec 6, 2022 · 14 comments · Fixed by #5873
Closed

Transition resolves instantly sometimes #3126

twiddler opened this issue Dec 6, 2022 · 14 comments · Fixed by #5873
Labels
Fixed patch Completed issues that will be published with next patch (1.0.X)

Comments

@twiddler
Copy link
Contributor

twiddler commented Dec 6, 2022

What package has an issue

@mantine/core

Describe the bug

Transition sometimes shows its content instantly. I encountered this error in my app and was able to reproduce it on Mantine's documentation page on Transitions:

Screencast.from.2022-12-06.13-08-32.webm

In the video, the dropdown is shown instantly when opening it the 4th and the 7th time.

I cannot reproduce this bug every time. It seems to happen randomly.

I am running Fedora 37. I encounter this bug in Mozilla Firefox 107.0, Chromium 105.0.5195.125 and Chrome 110.0.5449.0. I did not try it in other browsers or versions.

What version of @mantine/hooks page do you have in package.json?

5.9.0

If possible, please include a link to a codesandbox with the reproduced problem

https://mantine.dev/core/transition/

Do you know how to fix the issue

No.

Are you willing to participate in fixing this issue and create a pull request with the fix

I don't believe I can, so no.

@rtivital
Copy link
Member

rtivital commented Dec 6, 2022

I cannot reproduce it in Chrome, Safari and Firefox on macOS.

@rtivital rtivital added help wanted Contributions from community are welcome Linux Issues that are reproducible only on Linux and removed review required labels Dec 6, 2022
@crstffr

This comment was marked as resolved.

@rtivital
Copy link
Member

rtivital commented Dec 7, 2022

Your issue is not related to this issue, see https://mantine.dev/theming/theme-object/#respectreducemotion

@crstffr
Copy link

crstffr commented Dec 7, 2022

You are right. It does look to be the reduced motion settings. I'm sorry for the confusion.

@cyantree
Copy link
Contributor

I can reproduce it on Win 10 with Chrome.

It might be related to the following timeout:
https://github.com/mantinedev/mantine/blob/master/src/mantine-core/src/Transition/use-transition.ts#L57

My theory:
Sometimes this timeout resolves within the same animation frame. This leads to the browser rendering the pre-entering animation state (e. g. opacity: 0) and entering animation state (e. g. opacity: 1) in the same frame. Therefore the transition gets skipped entirely (because there is nothing to animate).
The current timeout of 10ms might be relevant too because it's less than 1/60th of a second (or approx. 16.67ms) (typical refresh rate of displays).
So maybe it doesn't occur on displays with higher refresh rates like 120 hz (or approx. 8.33ms).

If I reduce the timeout from 10 to 1 I can reproduce it quite reliably.

      const preStateTimeout = window.requestAnimationFrame(() => {
        window.requestAnimationFrame(() => {
          typeof preHandler === 'function' && preHandler();
          setStatus(shouldMount ? 'entering' : 'exiting');
        });
      });

On the other hand increasing the timeout from 10 to 50 seems to eliminate it for my case.

So besides choosing an arbitrary timeout another solution might be switching to requestAnimationFrame() as this should ensure that there's a frame switch happening.

However in my tests I needed to wait for two frames to get it working.

      const preStateTimeout = window.requestAnimationFrame(() => {
        window.requestAnimationFrame(() => {
          typeof preHandler === 'function' && preHandler();
          setStatus(shouldMount ? 'entering' : 'exiting');
        });
      });

I don't know why this is. Maybe because the first requestAnimationFrame() still resolves in the same frame?

@cyantree
Copy link
Contributor

I've checked this issue with the v6 and can confirm that it's still an issue there.
However I also realized that my proposal doesn't work realiably.

After some more digging I think I found the real problem:
It's related to update batching of React. v18 introduces state update batching for calls like setTimeout(). This means that in these cases the call setStatus(shouldMount ? 'entering' : 'exiting'); within the timeout will be batched with setStatus(shouldMount ? 'pre-entering' : 'pre-exiting');. Therefore the state pre-entering won't get processed at all.

A solution for this would be forcing the state update with ReactDOM.flushSync():

    flushSync(() => {
      setStatus(shouldMount ? 'pre-entering' : 'pre-exiting');
    });

More details on this feature:
https://reactjs.org/blog/2022/03/29/react-v18.html#new-feature-automatic-batching
And some technical details including the way to opt out of batching:
reactwg/react-18#21

Another solution could be using effect hooks that make sure that pre-entering really has been set before continuing with with setting entering.

@runthis
Copy link

runthis commented Jul 6, 2023

Hey there @rtivital just wanted to provide another replicated example that I think shows the issue more clear than the original video. I've noticed it for some time so I have avoided using it for anything interactive but I regularly check back on the component from time to time to see if it's better 😀.

chrome, mantine 6, react 18, standard usage of component.

In the video I am clicking pretty fast but it can happen after a long wait also. I hope this helps adjust this awesome component.

2023-07-06.11-17-46.mp4

@rtivital rtivital added the Fix Unknown It is not clear whether the can be resolved label Sep 24, 2023
@rtivital rtivital removed help wanted Contributions from community are welcome Fix Unknown It is not clear whether the can be resolved labels Jan 5, 2024
@elibroftw
Copy link

This occurs on Windows 11 with Firefox as well. Text shows up before rather than along with the transition.

@cyantree
Copy link
Contributor

I've revisited this issue and tried the requestAnimationFrame() solution again. Now it seems to work which I've tested for the demo in the docs and the demo from #5193.

Sandbox which uses a patched package:
https://codesandbox.io/p/devbox/mantine-transition-test-next-forked-f65whv?file=%2Fpackage.json%3A12%2C23&workspaceId=8bab33a5-6cc6-4fd2-9a92-7ae3f118fdf8

And here are the changes:
https://github.com/mantinedev/mantine/compare/master...cyantree:mantine:3126_fix-transition-handling?expand=1

If this seems fine I'll prepare a PR for it.

@rtivital
Copy link
Member

Yeah, sure, you are welcome to submit a PR

@rtivital
Copy link
Member

@cyantree I've copied and tested your changes, they work well. I've integrated them, so there is no need for a PR. Thanks for the research!

@rtivital rtivital added Fixed patch Completed issues that will be published with next patch (1.0.X) and removed Linux Issues that are reproducible only on Linux labels Feb 26, 2024
@cyantree
Copy link
Contributor

Thanks @rtivital. I wanted to wait for some feedback from the issuers but if you already checked it out that's great! Happy to help out.

@rtivital
Copy link
Member

Fixed in 7.6.1

@rilrom
Copy link
Contributor

rilrom commented Mar 2, 2024

I am still able to reproduce this in 7.6.1, though less often than before.

It seems more prevalent when the example provided above is moved into storybook instead of codesandbox, and even more prevalent in Firefox compared to chromium based browsers.

Please note I'm only checking the Toggle Transition button for this issue in the example above. The Toggle Transition Interval button seems to be getting out of sync due to the use of setInterval, making it unlikely to be related to the Mantine package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed patch Completed issues that will be published with next patch (1.0.X)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants