search

bvaughn / react-devtools-experimental

Experimental rewrite of the React DevTools extension
https://react-devtools-experimental.now.sh
MIT License
965 stars 55 forks source link

Doesn't scroll down to the component when inspecting #369

Closed Daniel15 closed 5 years ago

Daniel15 commented 5 years ago

Go to https://our.internmc.facebook.com/intern/wiki/ [fb-only] Right-click notification jewel and select "Inspect" Go to "Components" tab Notice that it doesn't scroll down to the component, however, the correct component IS selected (as the props and context in the right pane are correct)

bvaughn commented 5 years ago

This is interesting. I can't repro it in a test app created to try to trigger it: https://tj8xl.csb.app/

All ways of selecting one of the rows (the selector tool, right click + "inspect", scroll and click in Elements tab) successfully sync up with the DevTools Components tab.

bvaughn commented 5 years ago

I can if I add a second React root. I think that might be the cause somehow.

Update: I could for a little while and now I can't again :grin:

bvaughn commented 5 years ago

The flow of code for this feature is...

  1. DevTools backend listens for a new selection in the browser's Elements tab and sets a flag (needsToSyncElementSelection): https://github.com/bvaughn/react-devtools-experimental/blob/8001b6432c203ee46d8bbcb4099bef79856107d7/shells/browser/shared/src/main.js#L213-L216

  2. Right before the Components tab is shown again, if this flag has been marked, it notifies the UI to update the selection: https://github.com/bvaughn/react-devtools-experimental/blob/8001b6432c203ee46d8bbcb4099bef79856107d7/shells/browser/shared/src/main.js#L221-L226

  3. The Agent in the middle listens for this: https://github.com/bvaughn/react-devtools-experimental/blob/8001b6432c203ee46d8bbcb4099bef79856107d7/src/backend/agent.js#L345-L351

  4. And forwards it on to the frontend: https://github.com/bvaughn/react-devtools-experimental/blob/8001b6432c203ee46d8bbcb4099bef79856107d7/src/backend/agent.js#L319-L324

  5. The frontend listens for this and updates its state: https://github.com/bvaughn/react-devtools-experimental/blob/8001b6432c203ee46d8bbcb4099bef79856107d7/src/devtools/views/Components/TreeContext.js#L710-L716

  6. Lastly the Tree UI listens for a change in state, and triggers the scroll to: https://github.com/bvaughn/react-devtools-experimental/blob/8001b6432c203ee46d8bbcb4099bef79856107d7/src/devtools/views/Components/Tree.js#L66-L76

Looks like all of these things are working as expected (in that the selection is being updated) except for the scroll operation. It works some (most?) of the time but not always.

Maybe this could indicate that, in some cases, the DevTools panel (and the tree/list inside of it) has an initial height of 0 or something?

bvaughn commented 5 years ago

If I force a height of 0, (by opening the bottom pop-out console so it completely covers DevTools), I can force this to happen.

bvaughn commented 5 years ago

Maybe this could indicate that, in some cases, the DevTools panel (and the tree/list inside of it) has an initial height of 0 or something?

Yeah, I think for sure this is what's happening. AutoSizer bails out rendering its children when it has a height of 0: https://github.com/bvaughn/react-virtualized-auto-sizer/blob/ffcba2dd39b89111ed4b42d64431f35ce7c1c23a/src/index.js#L121-L150

So what's happening is that the effect that should trigger the scroll gets run: https://github.com/bvaughn/react-devtools-experimental/blob/8001b6432c203ee46d8bbcb4099bef79856107d7/src/devtools/views/Components/Tree.js#L66-L76

But at that time, the list ref is null. Once it's later set, the effect doesn't get re-run, because it's a ref.

bvaughn commented 5 years ago

I think the fix to this would just be to set a min-height for the wrapper component to ensure that the AutoSizer bailout never happens in this case.

Alternately I guess I could go with a callback ref for this. Maybe that's a less hacky way to do this.

bvaughn commented 5 years ago

Based on my own testing, with a more contrived scenario, this fixes the issue. Hopefully it will fix it for you too but if not we'll revisit. For now I'll land a fix and see if you can confirm :)

bvaughn commented 5 years ago

Fixed via 8fa608d~

Daniel15 commented 5 years ago

Thanks for looking into it!! :D

Will our internal FB version auto-update or do I need to manually install the update?

bvaughn commented 5 years ago

Will our internal FB version auto-update

Yes, assuming you've got the Chef-installed version (by being a member in the v4 Workplace group). If you've manually installed though, you'll need to manually update