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

Issue: useEffect executed before browser can paint DOM update #20863

Closed
limekiln opened this issue Feb 23, 2021 · 34 comments
Closed

Issue: useEffect executed before browser can paint DOM update #20863

limekiln opened this issue Feb 23, 2021 · 34 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@limekiln
Copy link

limekiln commented Feb 23, 2021

To my understanding, useEffect should always run after the browser acutally did paint any updates. This way, I tried to ensure that a loading indicator is rendered after a user submits data but before a blocking function execution (e.g. a fetch operation where the data is waited on) is executed. The code looks somewhat like the following:

Wrapper component to which a loading indicator can be passed:

export const LoadingWrapper = ({ callback, children}) => {
  useEffect(() => {
      if (typeof callback === "function") callback();
  }, [])

  return <React.Fragment> {children} </React.Fragment>
}

Usage:

const [loading, setLoading] = useState(false);

const blockingFunction = () => {
    // some functionaliy which blocks painting updates
}

return (
    // some kind of input elements which change the `loading` state on submit
    
    { loading &&
        <LoadingWrapper callback={blockingFunction}>
            <div> Loading! </div>
        </LoadingWrapper>
)

I would expect from the documentation that the callback function is executed after the "Loading" div is rendered on the screen, but that is actually not the case. The new div appears after the blocking call is completely resolved.

To get it working, I acutally have to use a local state variable in addition to trigger the effect:

export const LoadingWrapper = ({ callback, children}) => {
    const [isMounted, setIsMounted] = useState(false)

    useEffect(() => {
        setIsMounted(true);
    }

    useEffect(() => {
        if (isMounted && typeof callback === "function") callback();
    }, [isMounted]

    return <React.Fragment> {children} </React.Fragment>
}

Using this, I also observed some strange browser dependent behaviour. If input changes (which lead loading being set to true) are submitted onBlur of the input, it works fine in Chrome as well as in Firefox. Submitting these changes onKeyPress (while checking if the pressed key was the Enter key), it works in Firefox, but not in Chrome.

Does anyone have an idea what exactly is going on here or do I miss something important?

UPDATE:
I added a code sandbox: https://codesandbox.io/s/effectrendering-umtwo

The behaviour is quite strange:
In Chrome, triggering onBlur works (almost) always, sometimes it seems to bug out. Triggering onKeyPress will never trigger the spinner, except when onBlur was triggered before with the exact same input string (even though this string should have no influence at all here).
In Firefox, neither onBlur nor onKeyPress will trigger the spinner for me.

Tested versions:
Chrome 88.0.4324.150 (64-Bit)
Firefox 85.0.2 (64-Bit)

@PolybiusPro
Copy link

https://codesandbox.io/s/effectrendering-forked-svcqr?file=/src/App.js

Hopefully I fixed your issue. The useCallback hooks required dependencies and are unnecessary. When loading is true then the value of loading and setLoading are passed to the wrapper and used there. useEffect will now only be called when the value of loading is changed.

@limekiln
Copy link
Author

limekiln commented Mar 1, 2021

@PolybiusPro You fixed the issue by actually removing the blocking sleep function. Your example works since setTimeout will behave nicely, but I added the sleep function the way it is to simulate a hard block of the render thread. If I add it back to your example and use it instead of the timeout, the same issue occurs again.

Also, you are of course right that the useCallback is not necessary in this case, but it is just a simplified example of a more complex piece of code which I cannot post here directly, but I still wanted to use the same logic to present the behaviour. Can you elaborate why they should be an issue here? In my oppinion, they should not need any dependencies since I do not want them to be updated anyway, the functions do not really depend on anything here.

@Kais3rP
Copy link

Kais3rP commented Mar 2, 2021

I noticed the same behavior here:
https://stackblitz.com/edit/react-long-processing-function-with-disabled-button-ser4dc?file=Child.tsx
The log obviously shows the correct LIFO behavior of the Event Loop, but the weird behavior happens on DOM, if you see the button gets greyed out as if it was disabled after it is clicked, because isLoading is set to true, but if you click two times or more, even when it's greyed out, it acts as if it wasn't disabled, as if DOM did not update correctly.

@bobsmits
Copy link

I noticed the same thing on react-native. I am able to update view position with a call to the native side in use effect before the changes are drawn to the screen.

@limekiln
Copy link
Author

Any updates regarding this issue? I am currently working around this by having a timeout so that changes can be drawn to the screen, but that is no really satisfying solution for obvious reasons.

@eps1lon eps1lon added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Mar 26, 2021
@Kais3rP
Copy link

Kais3rP commented Mar 28, 2021

Any updates regarding this issue? I am currently working around this by having a timeout so that changes can be drawn to the screen, but that is no really satisfying solution for obvious reasons.

Time Slice addresses this maybe ?

@bgirard
Copy link
Contributor

bgirard commented Mar 28, 2021

To my understanding, useEffect should always run after the browser acutally did paint any updates

This isn't true AIUI. If you change the state during any unrelated useLayoutEffect then previous useEffect will be flush synchronously before synchronously reconciling the modified state. Stick a console.trace() in your effect and what check out the stack. If you see a stack like this:


  | r | @ | backend.js:8032
-- | -- | -- | --
  | eval | @ | App.js? [sm]:22
  | eval | @ | LoadingWrapper.js? [sm]:7
  | invokePassiveEffectCreate | @ | react-dom.development.js:15797
  | callCallback | @ | react-dom.development.js:2746
  | invokeGuardedCallbackDev | @ | react-dom.development.js:2770
  | invokeGuardedCallback | @ | react-dom.development.js:2804
  | flushPassiveEffectsImpl | @ | react-dom.development.js:15859
  | unstable_runWithPriority | @ | scheduler.development.js:646
  | runWithPriority$1 | @ | react-dom.development.js:7417
  | flushPassiveEffects | @ | react-dom.development.js:15763
  | performSyncWorkOnRoot | @ | react-dom.development.js:14996
  | eval | @ | react-dom.development.js:7457
  | unstable_runWithPriority | @ | scheduler.development.js:646
  | runWithPriority$1 | @ | react-dom.development.js:7417
  | flushSyncCallbackQueueImpl | @ | react-dom.development.js:7453
  | flushSyncCallbackQueue | @ | react-dom.development.js:7443
  | discreteUpdates$1 | @ | react-dom.development.js:15094
  | discreteUpdates | @ | react-dom.development.js:2636
  | dispatchDiscreteEvent | @ | react-dom.development.js:3987

You can read this bottom up as: React caught the event, is doing a sync render, and then it flushes the passive effect (because not seen on the stack, the change has changed and its preparing to re-render).

This way, I tried to ensure that a loading indicator is rendered after a user submits data but before a blocking function execution (e.g. a fetch operation where the data is waited on) is executed.

You shouldn't run blocking code from a useEffect, particularly due what I mentioned above. If you must, then at least run it outside of the effect to avoid blocking the paint.

@Kais3rP
Copy link

Kais3rP commented Mar 28, 2021

To my understanding, useEffect should always run after the browser acutally did paint any updates

This isn't true AIUI. If you change the state during any unrelated useLayoutEffect then previous useEffect will be flush synchronously before synchronously reconciling the modified state. Stick a console.trace() in your effect and what check out the stack. If you see a stack like this:


  | r | @ | backend.js:8032
-- | -- | -- | --
  | eval | @ | App.js? [sm]:22
  | eval | @ | LoadingWrapper.js? [sm]:7
  | invokePassiveEffectCreate | @ | react-dom.development.js:15797
  | callCallback | @ | react-dom.development.js:2746
  | invokeGuardedCallbackDev | @ | react-dom.development.js:2770
  | invokeGuardedCallback | @ | react-dom.development.js:2804
  | flushPassiveEffectsImpl | @ | react-dom.development.js:15859
  | unstable_runWithPriority | @ | scheduler.development.js:646
  | runWithPriority$1 | @ | react-dom.development.js:7417
  | flushPassiveEffects | @ | react-dom.development.js:15763
  | performSyncWorkOnRoot | @ | react-dom.development.js:14996
  | eval | @ | react-dom.development.js:7457
  | unstable_runWithPriority | @ | scheduler.development.js:646
  | runWithPriority$1 | @ | react-dom.development.js:7417
  | flushSyncCallbackQueueImpl | @ | react-dom.development.js:7453
  | flushSyncCallbackQueue | @ | react-dom.development.js:7443
  | discreteUpdates$1 | @ | react-dom.development.js:15094
  | discreteUpdates | @ | react-dom.development.js:2636
  | dispatchDiscreteEvent | @ | react-dom.development.js:3987

You can read this bottom up as: React caught the event, is doing a sync render, and then it flushes the passive effect (because not seen on the stack, the change has changed and its preparing to re-render).

This way, I tried to ensure that a loading indicator is rendered after a user submits data but before a blocking function execution (e.g. a fetch operation where the data is waited on) is executed.

You shouldn't run blocking code from a useEffect, particularly due what I mentioned above. If you must, then at least run it outside of the effect to avoid blocking the paint.

Could you check this example: https://stackblitz.com/edit/react-3jsceq ?

It doesn't look to block the paint to me, since the button gets greyed out after the first click, it keeps getting the click events though . What's the explanation of this behavior ?

@bgirard
Copy link
Contributor

bgirard commented Mar 29, 2021

In this case this works as expected right? Your event handler triggers a render, which has a passive effect and nothing special causes that effect to run before the paint. This work as you intent it.

Now if you have something else in your application that causes the effect to flush before the paint then suddenly it doesn't work as you expected. Like this: https://stackblitz.com/edit/react-8zxitw

Note that it's line 17 that's responsible for useEffect running before the paint.

@pervezalam777
Copy link

This is an alternative to setTimeout you can use 'requestAnimationFrame'

`export const LoadingWrapper = ({ callback, children }) => {
useEffect(() => {
if (typeof callback === "function") {
window.requestAnimationFrame(() => {
window.requestAnimationFrame(() => {
callback();
});
});
}
}, [callback]);

return <React.Fragment> {children} </React.Fragment>;
};`

Why I have used requestAnimationFrame twice, I have seen it in a video presented by google chrome team.

@bgirard
Copy link
Contributor

bgirard commented Mar 29, 2021

If you're in an event handler, the first requestAnimationFrame is still blocking the frame for that event handler, the second one will run after the fact in some situation, but it's certainly not a guarantee or a robust solution.

@limekiln
Copy link
Author

limekiln commented Mar 29, 2021

Time Slice addresses this maybe ?

I thought so too, but if I am not mistaken here this is still experimental, is it not?

To my understanding, useEffect should always run after the browser acutally did paint any updates

This isn't true AIUI

Then I find the documentation quite misleading, since it states:

React guarantees the DOM has been updated by the time it runs the effects.

So this does not mean the updates should be visible within the browser? Also, in the given example, there is neither a useLayoutEffect nor any other unrelated state updates, so I do not understand what whould introduce this behaviour there.

You shouldn't run blocking code from a useEffect, particularly due what I mentioned above. If you must, then at least run it outside of the effect to avoid blocking the paint.

In this case, what would be a good practice / pattern for this usecase (rendering a loading indicator before executing a blocking function call) without using effects or experimental features?

@marko-knoebl
Copy link

To my understanding, useEffect should always run after the browser acutally did paint any updates

I was expecting that too, but apparently it's not the case. I was trying to use it for CSS animations which ended up not working as expected in this way.

As far as I understand it, the situation is like this:

  • useLayoutEffect will run before a repaint is triggered in the browser
  • useEffect will run after a repaint is triggered (but not necessarily completed)

From my research, a (incomplete) hook that always triggers after a repaint has been completed could look like this:

function useAfterPaintEffect(effect, dependencies) {
  useEffect(() => {
    requestAnimationFrame(() => {
      setTimeout(() => {
        effect();
      }, 0);
    });
  }, dependencies);
}

Explanation (source: archived page "MozAfterPaint" on MDN):

Web pages that want to take an action after a repaint of the page can use requestAnimationFrame with a callback that sets a timeout of zero to then call the code that takes the desired post-repaint action.

I think nesting either requestAnimationFrame + setTimout or 2x requestAnimationFrame as per the comment of @pervezalam777 fits our use cases. I've also seen this in libraries, e.g. in react-animate-mount

I agree with @limekiln that the documentation is wrong - the documentation states that an effect runs after the browser has painted

@marko-knoebl
Copy link

Here's another small examle that demos the actual behavior (a fade-in animation):

https://codesandbox.io/s/react-effect-vs-paint-qkt1c?file=/src/App.tsx

As I said, I think the actual behavior and the documentation don't match.

@maricabertarini
Copy link

I could reproduce the issue of having useEffect code executed before the content is actually painted and DOM actually updated. It's enough to set a breakpoint in the first line of the useEffect param function to observe that.

For example, this means that we cannot use useEffect if we need to read the size of some elements after rendering, because rendering did not complete yet when useEffect code starts to run.

The solution by @marko-knoebl with requestAnimationFrame and the setTimeout worked in my case and allows querying the DOM after paint completes.

@marko-knoebl
Copy link

It's enough to set a breakpoint in the first line of the useEffect param function to observe that.

Hm, I can't confirm this observation. Setting a breakpoint will actually change / "fix" the behavior in the codesandbox that I posted. If I set a breakpoint on 34 that code will start working- it will render the transparent div in that instant.

@marko-knoebl
Copy link

marko-knoebl commented Oct 31, 2021

Found an iteresting comment here:

https://stackoverflow.com/questions/56727477/react-how-does-react-make-sure-that-useeffect-is-called-after-the-browser-has-h

According to this comment, React should be using some combination of postMessage and requestAnimationFrame to accomplish running the effect after a repaint

Edit: The answer points to some old code, the exact file apparently doesn't exist anymore.

@ltkn
Copy link

ltkn commented Nov 8, 2021

function useAfterPaintEffect(effect, dependencies) {
  useEffect(() => {
    requestAnimationFrame(() => {
      setTimeout(() => {
        effect();
      }, 0);
    });
  }, dependencies);
}

thanks @marko-knoebl, I've used this technique to fix my scroll restoration issues (previously had a less elegant solution with a short setTimout)

@abacaj
Copy link

abacaj commented Nov 16, 2021

Here's another small examle that demos the actual behavior (a fade-in animation):

https://codesandbox.io/s/react-effect-vs-paint-qkt1c?file=/src/App.tsx

As I said, I think the actual behavior and the documentation don't match.

I think you are confusing how useEffect works, here is the correct flow for hiding and showing a fade element (it will animate in and out and remove from DOM once done):

https://codesandbox.io/s/react-effect-vs-paint-forked-4ie9z?file=/src/App.tsx
(note: you don't need a container it was just cleaner - you could have it in one component: https://codesandbox.io/s/react-effect-vs-paint-forked-gc066?file=/src/App.tsx)

From my experience, useEffect does run correctly as stated in docs:

By using this Hook, you tell React that your component needs to do something after render. React will remember the function you passed (we’ll refer to it as our “effect”), and call it later after performing the DOM updates

If this was not true, my example above would not work as intended. If we study your example on codesandbox, you can see that the order of operations is correct, but the opacity is always 1 because you return a empty node when the opacity is 0 - this means that transitioning from 0 to 1 is impossible because the node that uses opacity is never in the DOM when the opacity is 0.

The reason it works with a request animation frame or set timeout is very simply because it is being executed after the effect in the call stack.

@marko-knoebl
Copy link

@abacaj thanks for taking the time to respond - very much appreciated.

You're saying about my code: "you return a empty node when the opacity is 0" - I think you meant "when phase is 'unmounted' ". It's true that in that phase I'm returning an empty node - but then I'm also rendering something in phase "beforeEnter".

Thanks for your code - I'll take a deeper look into it and would use it as a reference in the future. But I still think it's true that an effect can run before the paint - depending on how the component is coded.

@marko-knoebl
Copy link

https://thoughtspile.github.io/2021/11/15/unintentional-layout-effect/

This is an interesting article that explains a specific case where "useEffect" fires before paint.

Again, I think this is a documentation issue.

@bvaughn
Copy link
Contributor

bvaughn commented Nov 23, 2021

tl;dr for people only reading the last few comments:

Any state updates scheduled from useLayoutEffect get processed synchronously. Before React starts those updates, it will flush any previous/remaining useEffect so that things stay in a predictable order overall (render -> layout effects -> passive effects -> next render).

I agree that the documentation could be better/clearer in this regard. (cc @gaearon, @rachelnabors).

My advice:

  • Things that affect layout (like tooltip position or size) belong in layout effects (useLayoutEffect). This ensures they can tweak the UI before the user sees anything.
  • Things that aren't specific to layout (like event handlers or any number of other things) belong in passive effects (useEffect) but avoid writing code that expects these effects to be fired at a specific time. (It's safe to assume they'll always be fired after layout effects, but try to avoid assumptions about before/after paint.)

@abacaj
Copy link

abacaj commented Nov 23, 2021

@abacaj thanks for taking the time to respond - very much appreciated.

You're saying about my code: "you return a empty node when the opacity is 0" - I think you meant "when phase is 'unmounted' ". It's true that in that phase I'm returning an empty node - but then I'm also rendering something in phase "beforeEnter".

Thanks for your code - I'll take a deeper look into it and would use it as a reference in the future. But I still think it's true that an effect can run before the paint - depending on how the component is coded.

I was trying to point out that your example did not make use of useLayoutEffect and it was just incorrectly rendering an empty node.

The reason that there is an issue with useEffect as @bvaughn pointed out is due to the combination of useLayoutEffect forcing a flush of previous effects.

It was also pointed out by @thoughtspile: reactjs/react.dev#4107 (comment)

https://thoughtspile.github.io/2021/11/15/unintentional-layout-effect/
This is an interesting article that explains a specific case where "useEffect" fires before paint.
Again, I think this is a documentation issue.

Agreed, it is not well documented and I imagine could cause unwanted behavior like blocking the painting of the browser.

@gaearon
Copy link
Collaborator

gaearon commented Mar 30, 2022

Behavior in React 18 should be more consistent. Updates caused by discrete user input events (like clicks or typing) should make useEffect run synchronously. Otherwise, it should be asynchronous. Would somebody like to check whether the original repro has become more consistent in 18? Thanks!

@Kais3rP
Copy link

Kais3rP commented Apr 20, 2022

Behavior in React 18 should be more consistent. Updates caused by discrete user input events (like clicks or typing) should make useEffect run synchronously. Otherwise, it should be asynchronous. Would somebody like to check whether the original repro has become more consistent in 18? Thanks!

Yes, the behaviour looks fixed now with react@18 stackblitz . The button click gets disabled immediately now, even though it doesn't get visually greyed out.

There's also another discussion on SO, with the weird effect showed here: codesanbox. Not sure if related though, but react@18 does not seem to change anything in the behaviour ( tested with both @17 and @18 ).

@gaearon
Copy link
Collaborator

gaearon commented Apr 20, 2022

There's also another discussion on SO, with the weird effect showed here: codesanbox. Not sure if related though, but react@18 does not seem to change anything in the behaviour ( tested with both @17 and @18 ).

What is the "weird effect" here, could you please clarify?

@gaearon
Copy link
Collaborator

gaearon commented Apr 20, 2022

I think we can close this because 18 behavior is consistent. Unless I'm missing something.

@gaearon gaearon closed this as completed Apr 20, 2022
@Kais3rP
Copy link

Kais3rP commented Apr 20, 2022

There's also another discussion on SO, with the weird effect showed here: codesanbox. Not sure if related though, but react@18 does not seem to change anything in the behaviour ( tested with both @17 and @18 ).

What is the "weird effect" here, could you please clarify?

The CSS transition looks to work only if the setState that toggles the opacity inside the useEffect is placed in a setTimeout.

@gaearon
Copy link
Collaborator

gaearon commented Apr 20, 2022

Is that different from the browser behavior when the change is synchronous? Meaning, is there anything React-specific about this question?

@Kais3rP
Copy link

Kais3rP commented Apr 20, 2022

Is that different from the browser behavior when the change is synchronous? Meaning, is there anything React-specific about this question?

I tried to change the opacity imperatively inside the ref callback and it has the same issue with transition, so I think you are right, this is not React related.

@anasmost
Copy link

anasmost commented Jun 16, 2022

I indeed encoutered the same problem: useEffect delaying the painting. This problem occured after a click event meant to trigger some state update. I did not try to observe useEffect behavior for other use cases yet.
While searching I found my answer in the docs here Timing Of Effects, as quoted below:

Additionally, starting in React 18, the function passed to useEffect will fire synchronously before layout and paint when it’s the result of a discrete user input such as a click, or when it’s the result of an update wrapped in flushSync. This behavior allows the result of the effect to be observed by the event system, or by the caller of flushSync.

@anasmost
Copy link

anasmost commented Jun 17, 2022

After expressing my best regards @gaearon , I'm bringing to your attention that I've modified the sandbox you referred to earlier, by adding a delay of 2 seconds (sleep) when the mounted state is set to true, inside the 2nd useEffect. Actually, the screen is not painted until the delay has finished.
This would demonstrate that useState callback runs before the screen is painted, at least for this use case (upon button click event).
Btw, I've also deactivated the infite loop protection feature to allow the delay loop to execute.

@gaearon
Copy link
Collaborator

gaearon commented Jun 17, 2022

Sorry I don't understand which sandbox you're referring to. Can you rephrase what the question is?

@anasmost
Copy link

anasmost commented Jun 17, 2022

Sorry I don't understand which sandbox you're referring to. Can you rephrase what the question is?

Sorry, here is the sandbox link
As part of this issue (useEffect callback executed before painting) I commented the following:

I indeed encoutered the same problem: useEffect delaying the painting. The problem for my specific case occured after a click event meant to trigger some state update. I did not try to observe useEffect behavior for other use cases yet.
While searching I found my answer in the docs here Timing Of Effects, as quoted below:

Additionally, starting in React 18, the function passed to useEffect will fire synchronously before layout and paint when it’s the result of a discrete user input such as a click, or when it’s the result of an update wrapped in flushSync. This behavior allows the result of the effect to be observed by the event system, or by the caller of flushSync.

And to let you know that useEffect callback is indeed executed before painting, I addressed to you the following comment letting you know that I modified a bit the sandbox you referred to earlier - sandbox link - to expose this issue:

After expressing my best regards @gaearon , I'm bringing to your attention that I've modified the sandbox you referred to earlier, by adding a delay of 2 seconds (sleep) when the mounted state is set to true, inside the 2nd useEffect.
We could see that, actually, the screen is not painted until the delay in the useEffect callback has finished.
This would demonstrate that useState callback runs before the screen is painted, at least for this use case (upon button click event).
Btw, I've also deactivated the infite loop protection feature to allow the delay loop to execute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests