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

Random wrong DOM elements order #2849

Open
jedlikowski opened this issue Nov 26, 2020 · 14 comments
Open

Random wrong DOM elements order #2849

jedlikowski opened this issue Nov 26, 2020 · 14 comments

Comments

@jedlikowski
Copy link

jedlikowski commented Nov 26, 2020

Hi there,

I'm seeing incorrect ordering of DOM elements in Firefox on Mac when rendering the below component. It's being rendered inside a Suspense and a fragment if this helps.

const CookieWidget = ({
  type = "banner",
  location = "bottom",
  onAccept = noop,
  backdrop = false,
  ...props
}) => {
  const [Widget, widgetProps] = useCookieWidget(type, location);

  return (
    <>
      {!!backdrop && <Backdrop show />}
      <Widget {...props} {...widgetProps} onAccept={onAccept} />
    </>
  );
};

This is the screenshot from rendered DOM elements:
image

They should be rendered the other way around, the current order makes the backdrop cover the widget.
Any help? Maybe I should provide more code parts? The weird thing is that sometimes it renders in the correct order, usually the first time after a hard refresh. Consecutive soft refreshing causes wrong order.

@JoviDeCroock
Copy link
Member

Hey,

I think a codesandbox example would help us solve this quicker

@xtroncode
Copy link

We are also facing a similar issue while migrating to preact (random reorders) hard to reproduce.

@olliekav
Copy link

olliekav commented Jan 4, 2021

I'm seeing this suddenly after a Netlify deploy. I can't replicate locally and now I can't get it to work properly when building.

You can see it on https://dj.olliekav.com, the player is stuck to the top when it should be at the bottom.

The Github repo is https://github.com/olliekav/dj.olliekav.com. I'm using a context to wrap the player, and the children are being rendered in the wrong order...

https://github.com/olliekav/dj.olliekav.com/blob/8566c9be25dc722ef67ab414f580df6400bef48f/src/contexts/player-context.js#L117

return (
    <PlayerContext.Provider value={{
      player,
      wavesurfer,
      changeVolume: changeVolume,
      playTrackAtIndex: playTrackAtIndex,
      playPause: playPause,
      setTimers: setTimers
    }}>
      <div class="wrapper loaded">
        {props.children}
        <Player
          waveformChildRef={waveformRef}
        />
      </div>
    </PlayerContext.Provider>
  );

@jedlikowski
Copy link
Author

In my case it turned out to be the app code loaded twice. The app I'm building is a widget people add to their sites and someone added it twice. Maybe this will help someone?

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Jan 4, 2021

@olliekav That's awesome, thank you so much for the detailed repro. Took a bit, but I was able to narrow it down to a ref function being side-effectful. It triggers a render and that job should only be done by any of the state setters. The faulty ref is here:

https://github.com/olliekav/dj.olliekav.com/blob/8566c9be25dc722ef67ab414f580df6400bef48f/src/contexts/player-context.js#L28-L32

const waveformRef = useCallback(node => {
  if (node !== null) {
    initWaveSurfer(node);
  }
}, []);

// Later in WaveformProgress (there are some components between here and above)
<div ref={waveformRef} />

Refs are meant to only mutate variables, and never trigger an update so the fix is to switch to useEffect for that

The fix looks like this:

const waveformRef = useRef();
useEffect(() => {
  if (waveformRef.current !== null) {
    initWaveSurfer(waveformRef.current);
  }
}, []);

// Later in WaveformProgress (there are some components between here and above)
<div ref={waveformRef} />

The code might become easier if that portion is moved into WaveformProgress directly. In the end all it does is fire up waveform and call a function passed through context and that can happen from anywhere:

// Pseudo code example for WaveformProgress
function WaveformProgress() {
  // Some sort of setter function that triggers an update
  // in the provider component.
  const { setWaveform } = useContext(PlayerContext)

  const ref = useRef();
  useEffect(() => {
    if (ref.current) {
      setWaveform(ref.current);
    }
  }, []);
  
  return <div ref={ref} />
}

@olliekav
Copy link

olliekav commented Jan 4, 2021

@marvinhagemeister wow, I really appreciate you looking at this so quickly, and giving me a code review 🙏.

I'd just moved all my class components to hooks + functional components so getting my head around the re-structuring. I just wondered why it suddenly started happening after one deploy as was fine earlier this week and I couldn't replicate it locally.

I was actually doing that to make sure the Dom node has loaded, which is in the React docs... https://reactjs.org/docs/hooks-faq.html#how-can-i-measure-a-dom-node

And yes, I was battling with forwarding refs down the component tree but I could definitely do some work on moving that inside the waveform component.

Thank you!

@olliekav
Copy link

olliekav commented Jan 5, 2021

@marvinhagemeister So I just made these updates, but I'm seeing the same outcome on https://dj.olliekav.com.

It's very random between hard & soft refreshes as @jedlikowski mentioned above.

@olliekav
Copy link

olliekav commented Jan 5, 2021

So the only way I could get the order to reliably work in my case, was to wrap the provider {props.children} in an extra container.

return (
    <PlayerContext.Provider value={{
      player,
      wavesurfer,
      changeVolume: changeVolume,
      playTrackAtIndex: playTrackAtIndex,
      playPause: playPause,
      setTimers: setTimers,
      initWavesurfer: initWavesurfer
    }}>
      <div class="wrapper loaded">
        <main class="main">
          {props.children}
        </main>
        <Player />
      </div>
    </PlayerContext.Provider>
  );

I'll put together a codepen this week and see if I can replicate with a minimal use case 👍

@JoviDeCroock
Copy link
Member

Are you using createPortal for these components by any chance?

@olliekav
Copy link

olliekav commented Jan 5, 2021

Are you using createPortal for these components by any chance?

Nope.

@JoviDeCroock
Copy link
Member

React-modal seems to be, that's where the issue presents itself

@olliekav
Copy link

olliekav commented Jan 5, 2021

React-modal seems to be, that's where the issue presents itself

Ah, interesting. I'll do some tests shortly with that 👍

@Zhanadil1509
Copy link

Zhanadil1509 commented Mar 12, 2021

i'm writing in webpack.config:
resolve: {
alias: {
"react": "preact/compat",
"react-dom": "preact/compat"
}
}

and random wrong DOM elements order. For example:

before:
--header--
--main--
--footer--

after:
--main--
--footer--
--header--

How to fix?

@mikekasprzak
Copy link
Contributor

mikekasprzak commented Sep 17, 2022

I'm upgrading a Preact 8 codebase to Preact X, and I also ran into this unusual DOM order behavior. I'm sharing how I worked around the issue, though it's not clear to me why the old code didn't work.

After rewriting/fixing my blog feed code, additional posts that were loaded (appended) to my feed array would be rendered in a strange order: one would get added to the end of the output, and the rest would get added to the front. If I was to console.log or view it in the Preact Dev Tools, the order would be correct. It's only the DOM that was wrong.

Anyway, like @olliekav above I solved my problem by wrapping my output in another tag (in my case, a Fragment).

render( props, state ) {
  const {feed} = state; // feed is an array of blog posts to be drawn in order. the array grows over time
  let content = [];  // an array of outputs we build below

  // Before
  //content = [...content, ...feed.map(this.makeFeedItem)];

  // After
  content.push(<Fragment>{feed.map(this.makeFeedItem)}</Fragment>);

  // This button disappears after you click it, and returns after the feed finishes loading
  if (state.isLoading) {
    content.push(<MoreButton />);
  }

  return <Fragment>{content}</Fragment>;
}

If anyone's interested, here's a written example of the unusual output.

Before clicking the MORE button, my output would look something like this.

  • item 1
  • item 2
  • item 3
  • item 4
  • MoreButton

After clicking the MORE button, the MORE button disappears (replaced with a loading animation). After loading is complete, the button returns, but output looks like this.

  • item 6
  • item 7
  • item 8
  • MoreButton
  • item 1
  • item 2
  • item 3
  • item 4
  • item 5

To me it looks to me like the last item (MoreButton) was replaced by item 5, then everything else got pushed on to the front. I did try adding keys in case that was related, but it was not.

If I put a console.log inside my makeFeedItem function, OR view the VDOM in the Preact Dev Tools, the output is what you'd expect:

  • item 1
  • item 2
  • item 3
  • item 4
  • item 5
  • item 6
  • item 7
  • item 8
  • MoreButton

Firefox and Preact 10.11.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants