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`.
<rdar://problem/46886427>
Created attachment 357970 [details] Patch
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
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.
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.
(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.
Comment on attachment 357970 [details] Patch r=me
Comment on attachment 357970 [details] Patch Clearing flags on attachment: 357970 Committed r239517: <https://trac.webkit.org/changeset/239517>
All reviewed patches have been landed. Closing bug.