REGRESSION (r238563): Web Inspector: Selection is erratic when holding Up/Down on Network Table

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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

[Radar WebKit Bug Importer]

<rdar://problem/47559124>

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 373185 [details]
Patch

[Matt Baker]

(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.

[Matt Baker]

Created attachment 373280 [details]
Patch

[Matt Baker]

(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.

[Devin Rousso]

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.

[Matt Baker]

Created attachment 373285 [details]
Patch

[Matt Baker]

(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.

[Matt Baker]

Created attachment 373291 [details]
Patch

[Matt Baker]

Created attachment 373292 [details]
Patch

[Devin Rousso]

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`?

[Matt Baker]

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 {
...

[Matt Baker]

Created attachment 373326 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 373326 [details]
Patch for landing

Clearing flags on attachment: 373326

Committed r247052: <https://trac.webkit.org/changeset/247052>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.