DevTools: Scheduling profiler: Add vertical scroll bar

[bvaughn]

Adds a vertical scrollbar to right side when sum of view heights is larger than the canvas.

Remaining items to be done:

• Shift+wheel should scroll the outer window down if hovering over a non-scrollable sub-region
• Resize track and thumb should appear without extra mouse move delay

This is a pretty big change. May help to look at individual commits?

outer-scroll-bar.mp4

[mrkev]

Wow it's (finally) happening! Let's gooooo!

[bvaughn]

@taneliang I have an idea to add support for interactions to bubble (like DOM events) and for views to stop an interaction from bubbling further if they handle it, then VerticalScrollOverflowView can compose VerticalScrollView to enable drag and wheel scrolling. I will push a commit shortly :)

composed-scroll-view.mp4

[taneliang]

@bvaughn That makes sense! I had an initial bubbling implementation but deleted it, probably just because we could do everything we needed without it. Here’s the old code anyway in case there are ideas we can steal! MLH-Fellowship/scheduling-profiler-prototype@46b28d0

[taneliang]

Something I noticed the old code did: I looped through the list of subviews in reverse because later subviews can draw over earlier ones, so we’d want later subviews to handle events first.

https://github.com/MLH-Fellowship/scheduling-profiler-prototype/blob/46997525744263b1614c2b617167ddcb33588ce7/src/layout/StaticLayoutView.js#L83-L85

[bvaughn]

Yeah! I could have sworn my bubbling code did the same but I guess that was an earlier iteration that I didn't end up committing. I'll change it to do this though :)

[bvaughn]

• Resize track and thumb should appear without extra mouse move delay

Regarding this issue, I see what the cause is but I'm not sure yet of how to fix it.

For example, the code in ResizableView that expands the collapsed view to full height when you click on the collapsed label– it asks the subview (e.g. the FlamechartView) for its desired size:

const subviewDesiredSize = this._subview.desiredSize();

Then we count on the withVerticalScrollbarLayout to show the scrollbar based on the new view height, but at the time it's called, the desired size of the content (VerticalScrollView) is still small:

const desiredContentSize = contentLayoutInfo.view.desiredSize();

It's not until the next interaction that the content size has increased– even if I explicitly also call setFrame on the content (VerticalScrollView) in the "click" handler.

_updateSubviewFrames() doesn't run until after withVerticalScrollbarLayout has run, at which point the new height info is available.

If I manually trigger surface.displayIfNeeded() (via a hack) even if I do it synchronously, at the end of the "click" handler, the layout is re-run and the scrollbar appears.

[bvaughn]

Okay, I have an idea for an approach that only sucks a little. In CanvasPage:

    const surface = surfaceRef.current;
    surface.handleInteraction(interaction);

    // Flush any display work that got queued up as part of the previous interaction.
    // Typically there should be no work, but certain interactions may need a second pass.
    // For example, the ResizableView may collapse/expand its contents,
    // which requires a second layout pass for an ancestor VerticalScrollOverflowView.
    surface.displayIfNeeded();

[bvaughn]

• Scroll state should be moved up into context (see Scheduling Profiler: Misc UX and performance improvements #22043)

Unfortunately there's another bit of a race case with this item. The outer VerticalScrollView can remember scroll state between tab changes, but it's immediately blown away by one of the layouter functions.

Adding an async delay before restoring it "works" but is super clunky.

I think the interactions between the layout helpers may warrant another pass. Seems like it's awkward to code around them. Maybe I should make them stateful? I don't know. I'm feeling burnt out on this for now. I may merge this as-is and leave those last two items for follow up.

[taneliang]

but at the time it's called, the desired size of the content (VerticalScrollView) is still small

Oh interesting! Did you tried calling this._updateSubviewFrames(); at the start of ResizableView's desiredSize method? I suspect that'll make its subview desire its new height 😂

Calling surface.displayIfNeeded(); in the event handler may not be a great idea – I recall seeing perf issues because we redrew the canvas multiple times in a single animation frame, which could happen if multiple events (e.g. mouse moves) are dispatched within the single animation frame.

Scroll state should be moved up into context

Hmm, this is an unorthodox solution, but would it be a good idea to just not unmount the scheduling profiler when switching tabs? That'll allow us to avoid all of these state restoration issues.

I had a similar issue at work where we had a complex component that was extremely expensive to mount, so just keeping it mounted but frozen (with a React.memo hack similar to https://codesandbox.io/s/subtree-freeze-geu6f?file=/src/App.js:925-990) + tucked away somewhere offscreen let us avoid expensive mounts when we tabbed between these components.

I think we could do something similar here to preserve component + view state, especially if we'll be facing many state restoration issues/complexity. Just an idea!

[bvaughn]

Did you tried calling this._updateSubviewFrames(); at the start of ResizableView's desiredSize method? I suspect that'll make its subview desire its new height 😂

Calling _updateSubviewFrames doesn't change the subview's desired size. I assume this is what you were suggesting?

diff --git a/packages/react-devtools-scheduling-profiler/src/CanvasPage.js b/packages/react-devtools-scheduling-profiler/src/CanvasPage.js
index 0caf20a4e3..8c84eac1b1 100644
--- a/packages/react-devtools-scheduling-profiler/src/CanvasPage.js
+++ b/packages/react-devtools-scheduling-profiler/src/CanvasPage.js
@@ -402,12 +402,6 @@ function AutoSizedCanvas({
     const surface = surfaceRef.current;
     surface.handleInteraction(interaction);
 
-    // Flush any display work that got queued up as part of the previous interaction.
-    // Typically there should be no work, but certain interactions may need a second pass.
-    // For example, the ResizableView may collapse/expand its contents,
-    // which requires a second layout pass for an ancestor VerticalScrollOverflowView.
-    surface.displayIfNeeded();
-
     canvas.style.cursor = surface.getCurrentCursor() || 'default';
 
     // Defer drawing to canvas until React's commit phase, to avoid drawing
diff --git a/packages/react-devtools-scheduling-profiler/src/view-base/resizable/ResizableView.js b/packages/react-devtools-scheduling-profiler/src/view-base/resizable/ResizableView.js
index b56422f715..fb7a92979d 100644
--- a/packages/react-devtools-scheduling-profiler/src/view-base/resizable/ResizableView.js
+++ b/packages/react-devtools-scheduling-profiler/src/view-base/resizable/ResizableView.js
@@ -79,6 +79,8 @@ export class ResizableView extends View {
   }
 
   desiredSize() {
+    this._updateSubviewFrames();
+
     const subviewDesiredSize = this._subview.desiredSize();
 
     if (this._shouldRenderResizeBar()) {

The easiest way to repro the (broken) behavior locally (with minimal noise from other views) is to remove all subviews from the root except for the flamechartViewWrapper, then just load a profile and click on the collapsed "flamechart" accordion tab.

The overall flow is:

1. ResizableView desiredSize() runs and has an updated content height (e.g. 322)
2. withVerticalScrollbarLayout() runs but with the old content height still (e.g. 16)
3. Eventually another interaction runs
4. withVerticalScrollbarLayout() runs again, now with the new content height (e.g. 330)

which could happen if multiple events (e.g. mouse moves) are dispatched within the single animation frame.

Oooh, as a side note, we should probably avoid dispatching multiple mouse-move events within an animation frame?

Calling surface.displayIfNeeded(); in the event handler may not be a great idea – I recall seeing perf issues because we redrew the canvas multiple times in a single animation frame, which could happen if multiple events (e.g. mouse moves) are dispatched within the single animation frame.

Good call. I'd love to be able to remove the hack if we can find a better way to get the nested layout issue resolved.

[jstejada]

i think there's lots i need to wrap my head around to give a useful review here, but overall lgtm. added some questions, some seemingly unintended changes are still showing up for me, but accepting to unblock

[bvaughn]

Since this has been approved by Luna and Juan, I'm going to go ahead and merge it so we can share with the v18 Working Group and start gathering user feedback.

@taneliang Let's continue the discussion thread above with a follow up PR?

[taneliang]

Calling _updateSubviewFrames doesn't change the subview's desired size. I assume this is what you were suggesting?

Unfortunately yes, that was what I was suggesting. Interesting that the layouter in step 2 is still using the old size even though ResizableView is returning the new size in step 1? This seems surprisingly difficult.

It's possible that the desiredSize implementation also needs a closer look – it has always felt a little messy to me, with different views having various ways of figuring out their sizes, and I vaguely recall some weird cases where the desired size only works when certain views are used with each other.

share with the v18 Working Group and start gathering user feedback.

Excited for this! Let's continue this thread in a future PR :)