192992 2018-12-21 12:54:33 -0800 Web Inspector: update scroll position when revealing a virtualized WI.DataGridNode 2018-12-21 14:28:03 -0800 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 hi hi commit-queue hi inspector-bugzilla-changes joepeck simon.fraser webkit-bug-importer oldest_to_newest 1490444 0 hi 2018-12-21 12:54:33 -0800 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`. 1490445 1 hi 2018-12-21 12:54:48 -0800 <rdar://problem/46886427> 1490449 2 357970 hi 2018-12-21 13:01:44 -0800 Created attachment 357970 Patch 1490453 3 357970 simon.fraser 2018-12-21 13:12:47 -0800 Comment on attachment 357970 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 1490460 4 357970 joepeck 2018-12-21 13:34:28 -0800 Comment on attachment 357970 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. 1490461 5 357970 hi 2018-12-21 13:37:03 -0800 Comment on attachment 357970 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. 1490467 6 joepeck 2018-12-21 14:00:37 -0800 (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. 1490468 7 357970 joepeck 2018-12-21 14:00:44 -0800 Comment on attachment 357970 Patch r=me 1490485 8 357970 commit-queue 2018-12-21 14:28:01 -0800 Comment on attachment 357970 Patch Clearing flags on attachment: 357970 Committed r239517: <https://trac.webkit.org/changeset/239517> 1490486 9 commit-queue 2018-12-21 14:28:03 -0800 All reviewed patches have been landed. Closing bug. 357970 2018-12-21 13:01:44 -0800 2018-12-21 14:28:01 -0800 Patch bug-192992-20181221140143.patch text/plain 5968 hi ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDBlNDY4NDQ1ODdkNWEzNjM2Mzk3YWFkMWM5ZDEz YzFkY2M4YzhkYjcuLmI5ZGFmM2RmZTllNTczNzEzOWU0YTg3ODIzYWU1OTYzZWYwNmRhZTkgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dl Ykluc3BlY3RvclVJL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDE4LTEyLTIxICBEZXZp biBSb3Vzc28gIDxkcm91c3NvQGFwcGxlLmNvbT4KKworICAgICAgICBXZWIgSW5zcGVjdG9yOiB1 cGRhdGUgc2Nyb2xsIHBvc2l0aW9uIHdoZW4gcmV2ZWFsaW5nIGEgdmlydHVhbGl6ZWQgV0kuRGF0 YUdyaWROb2RlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xOTI5OTIKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzQ2ODg2NDI3PgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdoZW4gYHJldmVhbGBpbmcgYSBg V0kuRGF0YUdyaWROb2RlYCwgaWYgaXQgaXMgbm90IGN1cnJlbnRseSBpbiB0aGUgRE9NIHRyZWUg KGUuZy4gaXQncyBiZWVuCisgICAgICAgIHZpcnR1YWxpemVkIGJ5IGl0J3Mgb3duZXIgYFdJLkRh dGFHcmlkYCksIHdlIG5lZWQgdG8gc2Nyb2xsIHRvIGl0J3MgcG9zaXRpb24gc28gdGhhdCBpdCBn ZXRzCisgICAgICAgIGFkZGVkIHRvIHRoZSBET00gdHJlZSBiZWZvcmUgaXQgY2FuIGJlIHJldmVh bGVkL3NlbGVjdGVkLgorCisgICAgICAgICogVXNlckludGVyZmFjZS9WaWV3cy9EYXRhR3JpZC5q czoKKyAgICAgICAgKFdJLkRhdGFHcmlkLnByb3RvdHlwZS5sYXlvdXQpOgorICAgICAgICAoV0ku RGF0YUdyaWQucHJvdG90eXBlLnVwZGF0ZVZpc2libGVSb3dzKToKKyAgICAgICAgKFdJLkRhdGFH cmlkLnByb3RvdHlwZS5fdXBkYXRlVmlzaWJsZVJvd3MpOiBEZWxldGVkLgorCisgICAgICAgICog VXNlckludGVyZmFjZS9WaWV3cy9EYXRhR3JpZE5vZGUuanM6CisgICAgICAgIChXSS5EYXRhR3Jp ZE5vZGUucHJvdG90eXBlLnJldmVhbCk6CisKIDIwMTgtMTItMjAgIE5pa2l0YSBWYXNpbHlldiAg PG52YXNpbHlldkBhcHBsZS5jb20+CiAKICAgICAgICAgV2ViIEluc3BlY3RvcjogRGFyayBNb2Rl OiBUeXBlIHByb2ZpbGVyIHBvcG92ZXJzIGhhdmUgYmxhY2sgdGV4dCBvbiBkYXJrIGJhY2tncm91 bmQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdz L0RhdGFHcmlkLmpzIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3Mv RGF0YUdyaWQuanMKaW5kZXggMTM1NWVkMWYzYTFiZjdlNDU0ZTkzYWFiYjMzM2M1OGU2NzRmZjVk MC4uMWMwZmZiNGJjODhjYTdhNjhkZGRkY2U2YTVmNDExZDkxN2Y5OWNkNyAxMDA2NDQKLS0tIGEv U291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvRGF0YUdyaWQuanMKKysr IGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvRGF0YUdyaWQuanMK QEAgLTg3Niw3ICs4NzYsNyBAQCBXSS5EYXRhR3JpZCA9IGNsYXNzIERhdGFHcmlkIGV4dGVuZHMg V0kuVmlldwogICAgICAgICAgICAgdGhpcy5fdXBkYXRlSGVhZGVyQW5kU2Nyb2xsYmFyKCk7CiAg ICAgICAgIH0KIAotICAgICAgICB0aGlzLl91cGRhdGVWaXNpYmxlUm93cygpOworICAgICAgICB0 aGlzLnVwZGF0ZVZpc2libGVSb3dzKCk7CiAgICAgfQogCiAgICAgc2l6ZURpZENoYW5nZSgpCkBA IC0xMDQ3LDcgKzEwNDcsNyBAQCBXSS5EYXRhR3JpZCA9IGNsYXNzIERhdGFHcmlkIGV4dGVuZHMg V0kuVmlldwogICAgICAgICB0aGlzLm5lZWRzTGF5b3V0KCk7CiAgICAgfQogCi0gICAgX3VwZGF0 ZVZpc2libGVSb3dzKCkKKyAgICB1cGRhdGVWaXNpYmxlUm93cyhmb2N1c2VkRGF0YUdyaWROb2Rl KQogICAgIHsKICAgICAgICAgaWYgKHRoaXMuX2lubGluZSB8fCB0aGlzLl92YXJpYWJsZUhlaWdo dFJvd3MpIHsKICAgICAgICAgICAgIC8vIElubGluZSBEYXRhR3JpZHMgYWx3YXlzIHNob3cgYWxs IHRoZWlyIHJvd3MsIHNvIHdlIGNhbid0IHZpcnR1YWxpemUgdGhlbS4KQEAgLTEwNjYsNiArMTA2 Niw5IEBAIFdJLkRhdGFHcmlkID0gY2xhc3MgRGF0YUdyaWQgZXh0ZW5kcyBXSS5WaWV3CiAgICAg ICAgICAgICAgICAgbmV4dEVsZW1lbnQgPSByb3dFbGVtZW50OwogICAgICAgICAgICAgfQogCisg ICAgICAgICAgICBpZiAoZm9jdXNlZERhdGFHcmlkTm9kZSkKKyAgICAgICAgICAgICAgICBmb2N1 c2VkRGF0YUdyaWROb2RlLmVsZW1lbnQuc2Nyb2xsSW50b1ZpZXdJZk5lZWRlZChmYWxzZSk7CisK ICAgICAgICAgICAgIHJldHVybjsKICAgICAgICAgfQogCkBAIC0xMDc5LDI2ICsxMDgyLDMyIEBA IFdJLkRhdGFHcmlkID0gY2xhc3MgRGF0YUdyaWQgZXh0ZW5kcyBXSS5WaWV3CiAgICAgICAgIGlm IChpc05hTih0aGlzLl9jYWNoZWRTY3JvbGxhYmxlT2Zmc2V0SGVpZ2h0KSkKICAgICAgICAgICAg IHRoaXMuX2NhY2hlZFNjcm9sbGFibGVPZmZzZXRIZWlnaHQgPSB0aGlzLl9zY3JvbGxDb250YWlu ZXJFbGVtZW50Lm9mZnNldEhlaWdodDsKIAotICAgICAgICBsZXQgc2Nyb2xsVG9wID0gdGhpcy5f Y2FjaGVkU2Nyb2xsVG9wOwotICAgICAgICBsZXQgc2Nyb2xsYWJsZU9mZnNldEhlaWdodCA9IHRo aXMuX2NhY2hlZFNjcm9sbGFibGVPZmZzZXRIZWlnaHQ7CisgICAgICAgIGxldCB2aXNpYmxlUm93 Q291bnQgPSBNYXRoLmNlaWwoKHRoaXMuX2NhY2hlZFNjcm9sbGFibGVPZmZzZXRIZWlnaHQgKyAo b3ZlcmZsb3dQYWRkaW5nICogMikpIC8gcm93SGVpZ2h0KTsKIAotICAgICAgICBsZXQgdmlzaWJs ZVJvd0NvdW50ID0gTWF0aC5jZWlsKChzY3JvbGxhYmxlT2Zmc2V0SGVpZ2h0ICsgKG92ZXJmbG93 UGFkZGluZyAqIDIpKSAvIHJvd0hlaWdodCk7CisgICAgICAgIGlmICghZm9jdXNlZERhdGFHcmlk Tm9kZSkgeworICAgICAgICAgICAgbGV0IGN1cnJlbnRUb3BNYXJnaW4gPSB0aGlzLl90b3BEYXRh VGFibGVNYXJnaW5IZWlnaHQ7CisgICAgICAgICAgICBsZXQgY3VycmVudEJvdHRvbU1hcmdpbiA9 IHRoaXMuX2JvdHRvbURhdGFUYWJsZU1hcmdpbkhlaWdodDsKKyAgICAgICAgICAgIGxldCBjdXJy ZW50VGFibGVCb3R0b20gPSBjdXJyZW50VG9wTWFyZ2luICsgKHZpc2libGVSb3dDb3VudCAqIHJv d0hlaWdodCk7CiAKLSAgICAgICAgbGV0IGN1cnJlbnRUb3BNYXJnaW4gPSB0aGlzLl90b3BEYXRh VGFibGVNYXJnaW5IZWlnaHQ7Ci0gICAgICAgIGxldCBjdXJyZW50Qm90dG9tTWFyZ2luID0gdGhp cy5fYm90dG9tRGF0YVRhYmxlTWFyZ2luSGVpZ2h0OwotICAgICAgICBsZXQgY3VycmVudFRhYmxl Qm90dG9tID0gY3VycmVudFRvcE1hcmdpbiArICh2aXNpYmxlUm93Q291bnQgKiByb3dIZWlnaHQp OworICAgICAgICAgICAgbGV0IGJlbG93VG9wVGhyZXNob2xkID0gIWN1cnJlbnRUb3BNYXJnaW4g fHwgdGhpcy5fY2FjaGVkU2Nyb2xsVG9wID4gY3VycmVudFRvcE1hcmdpbiArIHVwZGF0ZU9mZnNl dFRocmVzaG9sZDsKKyAgICAgICAgICAgIGxldCBhYm92ZUJvdHRvbVRocmVzaG9sZCA9ICFjdXJy ZW50Qm90dG9tTWFyZ2luIHx8IHRoaXMuX2NhY2hlZFNjcm9sbFRvcCArIHRoaXMuX2NhY2hlZFNj cm9sbGFibGVPZmZzZXRIZWlnaHQgPCBjdXJyZW50VGFibGVCb3R0b20gLSB1cGRhdGVPZmZzZXRU aHJlc2hvbGQ7CiAKLSAgICAgICAgbGV0IGJlbG93VG9wVGhyZXNob2xkID0gIWN1cnJlbnRUb3BN YXJnaW4gfHwgc2Nyb2xsVG9wID4gY3VycmVudFRvcE1hcmdpbiArIHVwZGF0ZU9mZnNldFRocmVz aG9sZDsKLSAgICAgICAgbGV0IGFib3ZlQm90dG9tVGhyZXNob2xkID0gIWN1cnJlbnRCb3R0b21N YXJnaW4gfHwgc2Nyb2xsVG9wICsgc2Nyb2xsYWJsZU9mZnNldEhlaWdodCA8IGN1cnJlbnRUYWJs ZUJvdHRvbSAtIHVwZGF0ZU9mZnNldFRocmVzaG9sZDsKLQotICAgICAgICBpZiAoYmVsb3dUb3BU aHJlc2hvbGQgJiYgYWJvdmVCb3R0b21UaHJlc2hvbGQgJiYgIWlzTmFOKHRoaXMuX3ByZXZpb3Vz UmV2ZWFsZWRSb3dDb3VudCkpCi0gICAgICAgICAgICByZXR1cm47CisgICAgICAgICAgICBpZiAo YmVsb3dUb3BUaHJlc2hvbGQgJiYgYWJvdmVCb3R0b21UaHJlc2hvbGQgJiYgIWlzTmFOKHRoaXMu X3ByZXZpb3VzUmV2ZWFsZWRSb3dDb3VudCkpCisgICAgICAgICAgICAgICAgcmV0dXJuOworICAg ICAgICB9CiAKICAgICAgICAgbGV0IHJldmVhbGVkUm93cyA9IHRoaXMuX3Jvd3MuZmlsdGVyKChy b3cpID0+IHJvdy5yZXZlYWxlZCAmJiAhcm93LmhpZGRlbik7CiAKICAgICAgICAgdGhpcy5fcHJl dmlvdXNSZXZlYWxlZFJvd0NvdW50ID0gcmV2ZWFsZWRSb3dzLmxlbmd0aDsKIAotICAgICAgICBs ZXQgdG9wSGlkZGVuUm93Q291bnQgPSBNYXRoLm1heCgwLCBNYXRoLmZsb29yKChzY3JvbGxUb3Ag LSBvdmVyZmxvd1BhZGRpbmcpIC8gcm93SGVpZ2h0KSk7CisgICAgICAgIGlmIChmb2N1c2VkRGF0 YUdyaWROb2RlKSB7CisgICAgICAgICAgICBsZXQgZm9jdXNlZEluZGV4ID0gcmV2ZWFsZWRSb3dz LmluZGV4T2YoZm9jdXNlZERhdGFHcmlkTm9kZSk7CisgICAgICAgICAgICBsZXQgZmlyc3RWaXNp YmxlUm93SW5kZXggPSB0aGlzLl9jYWNoZWRTY3JvbGxUb3AgLyByb3dIZWlnaHQ7CisgICAgICAg ICAgICBpZiAoZm9jdXNlZEluZGV4IDwgZmlyc3RWaXNpYmxlUm93SW5kZXggfHwgZm9jdXNlZElu ZGV4ID4gZmlyc3RWaXNpYmxlUm93SW5kZXggKyB2aXNpYmxlUm93Q291bnQpCisgICAgICAgICAg ICAgICAgdGhpcy5fc2Nyb2xsQ29udGFpbmVyRWxlbWVudC5zY3JvbGxUb3AgPSB0aGlzLl9jYWNo ZWRTY3JvbGxUb3AgPSAoZm9jdXNlZEluZGV4ICogcm93SGVpZ2h0KSAtICh0aGlzLl9jYWNoZWRT Y3JvbGxhYmxlT2Zmc2V0SGVpZ2h0IC8gMikgKyAocm93SGVpZ2h0IC8gMik7CisgICAgICAgIH0K KworICAgICAgICBsZXQgdG9wSGlkZGVuUm93Q291bnQgPSBNYXRoLm1heCgwLCBNYXRoLmZsb29y KCh0aGlzLl9jYWNoZWRTY3JvbGxUb3AgLSBvdmVyZmxvd1BhZGRpbmcpIC8gcm93SGVpZ2h0KSk7 CiAgICAgICAgIGxldCBib3R0b21IaWRkZW5Sb3dDb3VudCA9IE1hdGgubWF4KDAsIHRoaXMuX3By ZXZpb3VzUmV2ZWFsZWRSb3dDb3VudCAtIHRvcEhpZGRlblJvd0NvdW50IC0gdmlzaWJsZVJvd0Nv dW50KTsKIAogICAgICAgICBsZXQgbWFyZ2luVG9wID0gdG9wSGlkZGVuUm93Q291bnQgKiByb3dI ZWlnaHQ7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9W aWV3cy9EYXRhR3JpZE5vZGUuanMgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFj ZS9WaWV3cy9EYXRhR3JpZE5vZGUuanMKaW5kZXggZTk5ZWVkMzg5ZDFhNmI1YzM0YzAwNGMzM2Ux ZWI5YTNjZGYwNGUxNC4uZTU4YWU1ZmRiODBiZDAzYTllYTkzZTU4NDYxNzYxMTg4YjA0MDk4YSAx MDA2NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvRGF0 YUdyaWROb2RlLmpzCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1Zp ZXdzL0RhdGFHcmlkTm9kZS5qcwpAQCAtNTQ0LDcgKzU0NCw3IEBAIFdJLkRhdGFHcmlkTm9kZSA9 IGNsYXNzIERhdGFHcmlkTm9kZSBleHRlbmRzIFdJLk9iamVjdAogICAgICAgICAgICAgY3VycmVu dEFuY2VzdG9yID0gY3VycmVudEFuY2VzdG9yLnBhcmVudDsKICAgICAgICAgfQogCi0gICAgICAg IHRoaXMuZWxlbWVudC5zY3JvbGxJbnRvVmlld0lmTmVlZGVkKGZhbHNlKTsKKyAgICAgICAgdGhp cy5kYXRhR3JpZC51cGRhdGVWaXNpYmxlUm93cyh0aGlzKTsKIAogICAgICAgICB0aGlzLmRpc3Bh dGNoRXZlbnRUb0xpc3RlbmVycygicmV2ZWFsZWQiKTsKICAgICB9Cg==