Bug 192992 – Web Inspector: update scroll position when revealing a virtualized WI.DataGridNode

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

When calling `reveal()` on a `WI.DataGridNode`, if that node isn't currently visible (e.g. it's scrolled offscreen), then it most likely isn't part of the DOM tree, meaning that `scrollIntoViewIfNeeded` won't work.  Since `WI.DataGrid` uses virtualization to keep the DOM tree more lightweight, when we call `WI.DataGridNode.prototype.reveal`, we need to recalculate what nodes should be in the DOM tree.

This is the same logic that is done by a virtualized `WI.TreeOutline`.

[Devin Rousso]

<rdar://problem/46886427>

[Devin Rousso]

Created attachment 357970 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 357970 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357970&action=review

> Source/WebInspectorUI/ChangeLog:11
> +        When `reveal`ing a `WI.DataGridNode`, if it is not currently in the DOM tree (e.g. it's been
> +        virtualized by it's owner `WI.DataGrid`), we need to scroll to it's position so that it gets
> +        added to the DOM tree before it can be revealed/selected.

Those backticks tho

[Joseph Pecoraro]

Comment on attachment 357970 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357970&action=review

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1107
> +                this._scrollContainerElement.scrollTop = this._cachedScrollTop = (focusedIndex * rowHeight) - (this._cachedScrollableOffsetHeight / 2) + (rowHeight / 2);

The new scrollTop is equal to:

    (the top of the focused row) - (half the scrollable area) + (half a row)

I'm not sure I follow this. Everything else seems good.

[Devin Rousso]

Comment on attachment 357970 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357970&action=review

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1107
>> +                this._scrollContainerElement.scrollTop = this._cachedScrollTop = (focusedIndex * rowHeight) - (this._cachedScrollableOffsetHeight / 2) + (rowHeight / 2);
> 
> The new scrollTop is equal to:
> 
>     (the top of the focused row) - (half the scrollable area) + (half a row)
> 
> I'm not sure I follow this. Everything else seems good.

This just centers the node in the view.  Scrolling to `(focusedIndex * rowHeight)` (this is the `scrollTop` of the focused `WI.DataGridNode`) would make the node visible as the first row.  Subtracting `(this._cachedScrollableOffsetHeight / 2)` moves the node to the middle, specifically the `scrollTop` of the node is now in the middle (meaning that the `WI.DataGridNode` is below the middle).  Finally, adding `(rowHeight / 2)` ensures that the middle of the focused node is in the middle of the scrollable area.

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #5)
> Comment on attachment 357970 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357970&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1107
> >> +                this._scrollContainerElement.scrollTop = this._cachedScrollTop = (focusedIndex * rowHeight) - (this._cachedScrollableOffsetHeight / 2) + (rowHeight / 2);
> > 
> > The new scrollTop is equal to:
> > 
> >     (the top of the focused row) - (half the scrollable area) + (half a row)
> > 
> > I'm not sure I follow this. Everything else seems good.
> 
> This just centers the node in the view.  Scrolling to `(focusedIndex *
> rowHeight)` (this is the `scrollTop` of the focused `WI.DataGridNode`) would
> make the node visible as the first row.  Subtracting
> `(this._cachedScrollableOffsetHeight / 2)` moves the node to the middle,
> specifically the `scrollTop` of the node is now in the middle (meaning that
> the `WI.DataGridNode` is below the middle).  Finally, adding `(rowHeight /
> 2)` ensures that the middle of the focused node is in the middle of the
> scrollable area.

Okay that is what I was expecting and I somehow convinced myself that it wasn't doing that. I've now come to my senses. We may decide to configure scroll to middle or make it the top/bottom row depending on scroll direction. But middle is reasonable for starters.

[Joseph Pecoraro]

Comment on attachment 357970 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 357970 [details]
Patch

Clearing flags on attachment: 357970

Committed r239517: <https://trac.webkit.org/changeset/239517>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.