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

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

Closed
Daniel15 opened this issue Aug 9, 2019 · 10 comments

Comments

@Daniel15
Copy link

commented Aug 9, 2019

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 bvaughn added the 😭 bug label Aug 9, 2019

@bvaughn

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

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 😁

@bvaughn

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

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):

    setReactSelectionFromBrowser();
    chrome.devtools.panels.elements.onSelectionChanged.addListener(() => {
    setReactSelectionFromBrowser();
    });

  2. Right before the Components tab is shown again, if this flag has been marked, it notifies the UI to update the selection:

    chrome.devtools.panels.create('⚛ Components', '', 'panel.html', panel => {
    panel.onShown.addListener(panel => {
    if (needsToSyncElementSelection) {
    needsToSyncElementSelection = false;
    bridge.send('syncSelectionFromNativeElementsPanel');
    }

  3. The Agent in the middle listens for this:

    syncSelectionFromNativeElementsPanel = () => {
    const target = window.__REACT_DEVTOOLS_GLOBAL_HOOK__.$0;
    if (target == null) {
    return;
    }
    this.selectNode(target);
    };

  4. And forwards it on to the frontend:

    selectNode(target: Object): void {
    const id = this.getIDForNode(target);
    if (id !== null) {
    this._bridge.send('selectFiber', id);
    }
    }

  5. The frontend listens for this and updates its state:

    // Listen for host element selections.
    useEffect(() => {
    const handleSelectFiber = (id: number) =>
    dispatchWrapper({ type: 'SELECT_ELEMENT_BY_ID', payload: id });
    bridge.addListener('selectFiber', handleSelectFiber);
    return () => bridge.removeListener('selectFiber', handleSelectFiber);
    }, [bridge, dispatchWrapper]);

  6. Lastly the Tree UI listens for a change in state, and triggers the scroll to:

    // Make sure a newly selected element is visible in the list.
    // This is helpful for things like the owners list and search.
    useLayoutEffect(() => {
    if (selectedElementIndex !== null && listRef.current != null) {
    listRef.current.scrollToItem(selectedElementIndex, 'smart');
    // Note this autoscroll only works for rows.
    // There's another autoscroll inside the elements
    // that ensures the component name is visible horizontally.
    // It's too early to do it now because the row might not exist yet.
    }
    }, [listRef, selectedElementIndex]);

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

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

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:

// Make sure a newly selected element is visible in the list.
// This is helpful for things like the owners list and search.
useLayoutEffect(() => {
if (selectedElementIndex !== null && listRef.current != null) {
listRef.current.scrollToItem(selectedElementIndex, 'smart');
// Note this autoscroll only works for rows.
// There's another autoscroll inside the elements
// that ensures the component name is visible horizontally.
// It's too early to do it now because the row might not exist yet.
}
}, [listRef, selectedElementIndex]);

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 bvaughn self-assigned this Aug 12, 2019

@bvaughn

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2019

Fixed via 8fa608d~

@bvaughn bvaughn closed this Aug 12, 2019

@Daniel15

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

Thanks for looking into it!! :D

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

@bvaughn

This comment has been minimized.

Copy link
Owner

commented Aug 13, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.