Bug 193841 - REGRESSION (r238563): Web Inspector: Selection is erratic when holding Up/Down on Network Table
Summary: REGRESSION (r238563): Web Inspector: Selection is erratic when holding Up/Dow...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-25 13:16 PST by Joseph Pecoraro
Modified: 2019-07-02 09:14 PDT (History)
6 users (show)

See Also:


Attachments
[Video] Inserting Table rows (560.14 KB, video/mp4)
2019-06-27 23:45 PDT, Matt Baker
no flags Details
Patch (4.09 KB, patch)
2019-06-29 21:26 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (2.83 KB, patch)
2019-07-01 17:03 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (2.52 KB, patch)
2019-07-01 17:32 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2019-07-01 17:58 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (4.88 KB, patch)
2019-07-01 18:02 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (5.04 KB, patch)
2019-07-02 08:55 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-01-25 13:16:49 PST
REGRESSION (r238563): Web Inspector: Selection is erratic when holding Up/Down on Network Table

https://trac.webkit.org/changeset/238563/webkit

Steps to Reproduce:
1. Inspect <https://www.harley-davidson.com/us/en/index.html>
2. Show Network Tab
3. Select a resource
4. Hold Up/Down keys
  => Expected selection to travel with the selected row, instead selection is erratic
Comment 1 Radar WebKit Bug Importer 2019-01-25 13:17:26 PST
<rdar://problem/47559124>
Comment 2 Matt Baker 2019-06-27 23:37:04 PDT
Even without holding up/down, a selected row will move around on the example page. As resources continue to finish loading and are updated in the table, the table is continuously reloaded. After each reload, we attempt to restore the selection.

The restored selection appears to move, because the row being re-selected may no longer be in the same location in the table.

To resolve this, we can try extending Table to allow rows to be inserted one at a time, without having to reload everything. This will at least be more efficient, but may not solve the "jumping selection" issue. When the inserted row is above the selected row, the selected row will be forced down.

We could also attempt to adjust the scroll position of the Table, so that the selected row appears to remain in place as rows are inserted above it.
Comment 3 Matt Baker 2019-06-27 23:45:26 PDT
Created attachment 373091 [details]
[Video] Inserting Table rows

This demo exercises a new method, Table.prototype.insertRow. New rows are inserted at random, and do not interfere with the active selection.

Inserting a row does not require a reload, however the Table client is responsible for keeping the data source and Table in sync:

class Table
{
    insertRow(rowIndex)
    {
        console.assert(rowIndex >= 0 && rowIndex <= this.numberOfRows);
        if (rowIndex === this.numberOfRows) {
            this.reloadDataAddedToEndOnly();
            return;
        }

        for (let index = rowIndex; index < this.numberOfRows; ++index)
            this._adjustRowAtIndex(index, 1);

        this._previousRevealedRowCount = NaN;
        this.needsLayout();
    }

    _adjustRowAtIndex(index, delta)
    {
        let row = this._cachedRows.get(index);
        console.assert(row, "Missing row for index " + index);
        if (!row)
            return;

        this._cachedRows.delete(index);
        row.__index += delta;
        this._cachedRows.set(row.__index, row);
    }
}

Removing rows has a similar safety issue.

A possible way around this would be to create a formal binding mechanism where the array data source can be observed via a Proxy, and the Table can be notified when objects are added or removed and update itself.
Comment 4 Matt Baker 2019-06-28 16:02:17 PDT
Inserting a single row instead of reloading the whole table in NetworkTableContentView.prototype._insertResourceAndReloadTable helps, but isn't enough.

Ongoing resource loads and DOM node events cause entries to be added to the current collection. When pending updates are processed the table is reloaded.

Going to take a different approach: maybe we can suffer ongoing table reloads as long as the restored selection has the exact scroll position as the original selection. This should make the reload invisible to the user.

Eventually it would be good to eliminate unnecessary reloads, but that may be beyond the scope of this bug.
Comment 5 Matt Baker 2019-06-28 17:21:41 PDT
(In reply to Matt Baker from comment #4)
> Inserting a single row instead of reloading the whole table in
> NetworkTableContentView.prototype._insertResourceAndReloadTable helps, but
> isn't enough.
> 
> Ongoing resource loads and DOM node events cause entries to be added to the
> current collection. When pending updates are processed the table is reloaded.
> 
> Going to take a different approach: maybe we can suffer ongoing table
> reloads as long as the restored selection has the exact scroll position as
> the original selection. This should make the reload invisible to the user.

Saving and restoring the scroll position across reloads solves the problem:

class NetworkTableContentView
{
    _reloadTable()
    {
        let scrollTop = this._table.scrollContainer.scrollTop;

        this._table.reloadData();
        this._restoreSelectedRow();

        this._table.scrollContainer.scrollTop = scrollTop;
    }
}

If the number of rows _above_ the selected row changes, the selection will still move, but usually only by a low multiple of the row height!

As setting the `scrollTop` in this way is unsafe (it may not be valid for the new table size), I'm going to add a `scrollToPosition` method to Table to do this safely.

Note:
The existing method `Table.prototype.restoreScrollPosition` doesn't help here, since it does not work across reloads.
Comment 6 Matt Baker 2019-06-29 21:26:50 PDT
Created attachment 373185 [details]
Patch
Comment 7 Matt Baker 2019-06-29 21:29:07 PDT
(In reply to Matt Baker from comment #5)
> (In reply to Matt Baker from comment #4)
> > Inserting a single row instead of reloading the whole table in
> > NetworkTableContentView.prototype._insertResourceAndReloadTable helps, but
> > isn't enough.
> > 
> > Ongoing resource loads and DOM node events cause entries to be added to the
> > current collection. When pending updates are processed the table is reloaded.
> > 
> > Going to take a different approach: maybe we can suffer ongoing table
> > reloads as long as the restored selection has the exact scroll position as
> > the original selection. This should make the reload invisible to the user.
> 
> Saving and restoring the scroll position across reloads solves the problem:
> 
> class NetworkTableContentView
> {
>     _reloadTable()
>     {
>         let scrollTop = this._table.scrollContainer.scrollTop;
> 
>         this._table.reloadData();
>         this._restoreSelectedRow();
> 
>         this._table.scrollContainer.scrollTop = scrollTop;
>     }
> }
> 
> If the number of rows _above_ the selected row changes, the selection will
> still move, but usually only by a low multiple of the row height!
> 
> As setting the `scrollTop` in this way is unsafe (it may not be valid for
> the new table size), I'm going to add a `scrollToPosition` method to Table
> to do this safely.

My concern was unwarranted. Setting scrollTop causes Table to perform a layout, which will constrain the new scroll position if needed.
Comment 8 Matt Baker 2019-07-01 17:03:31 PDT
Created attachment 373280 [details]
Patch
Comment 9 Matt Baker 2019-07-01 17:05:21 PDT
(In reply to Matt Baker from comment #8)
> Created attachment 373280 [details]
> Patch

Thanks to Devin for his insight into the role `Table.prototype.revealRow` played in the issue, allowing us to avoid adding a new Table method which would have been more of a band-aid than a true fix.
Comment 10 Devin Rousso 2019-07-01 17:16:00 PDT
Comment on attachment 373280 [details]
Patch

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

r-, only because I think this would no longer ensure that a `layout()` call happens.  The logic looks good :)

> Source/WebInspectorUI/UserInterface/Views/Table.js:369
> +        if (isNaN(this._cachedScrollTop))
> +            this._cachedScrollTop = this._scrollContainerElement.scrollTop;

NIT: I'd love it if we had some sort of `_ensureScrollTop()` that did this logic for us, instead of having this same code in multiple places.

> Source/WebInspectorUI/UserInterface/Views/Table.js:-376
> -        if (this._isRowVisible(rowIndex)) {
> -            let row = this._cachedRows.get(rowIndex);
> -            console.assert(row, "Visible rows should always be in the cache.");
> -            if (row)
> -                row.scrollIntoViewIfNeeded(false);
> -            this.needsLayout();

I still think we want to do this, as we should try to use the browser method whenever possible. This works exactly as we want it to, so there's no reason to change it.

> Source/WebInspectorUI/UserInterface/Views/Table.js:-379
> -            this.updateLayout();

This seems to have been removed.  We still want to trigger a `layout()` at some point.  This will cause a "scroll" event to be fired, but the code for the `_handleScroll` will (I think) early return because the `event.wheelDeltaY` would be `0`.

> Source/WebInspectorUI/UserInterface/Views/Table.js:371
> +        if (isNaN(this._cachedHeight))

Ditto (>368).

> Source/WebInspectorUI/UserInterface/Views/Table.js:372
> +            this._cachedHeight = Math.floor(this._scrollContainerElement.getBoundingClientRect().height);

Why is it that we need `getBoundingClientRect`?  Could we not just use the `offsetHeight`?

NIT: you could use `Math.floor(this._scrollContainerElement.realOffsetHeight)` instead.
Comment 11 Matt Baker 2019-07-01 17:32:23 PDT
Created attachment 373285 [details]
Patch
Comment 12 Matt Baker 2019-07-01 17:33:26 PDT
(In reply to Matt Baker from comment #11)
> Created attachment 373285 [details]
> Patch

Ah, I didn't see your most recent comments. Disregard this patch.
Comment 13 Matt Baker 2019-07-01 17:58:35 PDT
Created attachment 373291 [details]
Patch
Comment 14 Matt Baker 2019-07-01 18:02:31 PDT
Created attachment 373292 [details]
Patch
Comment 15 Devin Rousso 2019-07-01 23:32:01 PDT
Comment on attachment 373292 [details]
Patch

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

r=me, with one issue/fix inside `revealRow`.  Please address before landing.

> Source/WebInspectorUI/ChangeLog:9
> +        Selecting and revealing a row after reload Table data, but before the

"after reloading Table data"

> Source/WebInspectorUI/UserInterface/Views/Table.js:-369
> -        // Force our own scroll update because we may have scrolled.
> -        this._cachedScrollTop = NaN;

Why was this removed?  In the `_isRowVisible` path, we'd still want to reset the `_cachedScrollTop` after calling `scrollIntoViewIfNeeded`.

> Source/WebInspectorUI/UserInterface/Views/Table.js:1457
> +    _ensureOffsetHeight()

NICE!  This is awesome.  I think I may have been a bit "overzealous" with the name though, as "ensure" is a bit weird.  Perhaps we could settle on `_calculateOffsetHeight`?
Comment 16 Matt Baker 2019-07-02 08:51:35 PDT
Comment on attachment 373292 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/Table.js:-369
>> -        this._cachedScrollTop = NaN;
> 
> Why was this removed?  In the `_isRowVisible` path, we'd still want to reset the `_cachedScrollTop` after calling `scrollIntoViewIfNeeded`.

I'll move this down, and skip the layout if `!row` in the first branch path:

if (this._isRowVisible(rowIndex)) {
    let row = this._cachedRows.get(rowIndex);
    console.assert(row, "Visible rows should always be in the cache.");
    if (row) {
        row.scrollIntoViewIfNeeded(false);
        this._cachedScrollTop = NaN;
        this.needsLayout();
    }
} else {
...
Comment 17 Matt Baker 2019-07-02 08:55:47 PDT
Created attachment 373326 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2019-07-02 09:14:25 PDT
Comment on attachment 373326 [details]
Patch for landing

Clearing flags on attachment: 373326

Committed r247052: <https://trac.webkit.org/changeset/247052>
Comment 19 WebKit Commit Bot 2019-07-02 09:14:29 PDT
All reviewed patches have been landed.  Closing bug.