Bug 191328 – Web Inspector: Table should recalculate scrollable height when resized

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

[Matt Baker]

Table.prototype._cachedScrollableHeight is the height of the scroll container, and determines the number of visible rows. It should be recalculated whenever the table is resized.

[Radar WebKit Bug Importer]

<rdar://problem/45854412>

[Matt Baker]

Created attachment 353997 [details]
Patch

[Devin Rousso]

Comment on attachment 353997 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/Table.js:625
> +            this._cachedScrollableHeight = NaN;

`_cachedScrollableHeight` is updated inside `_updateVisibleRows`.  Should we move this if-else to the top of `layout` and do any resetting there?

Also, would we want to reset this value inside `resize`, similar to `_cachedWidth` and `_cachedHeight`?

[Matt Baker]

Comment on attachment 353997 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/Table.js:625
>> +            this._cachedScrollableHeight = NaN;
> 
> `_cachedScrollableHeight` is updated inside `_updateVisibleRows`.  Should we move this if-else to the top of `layout` and do any resetting there?
> 
> Also, would we want to reset this value inside `resize`, similar to `_cachedWidth` and `_cachedHeight`?

Doh! This should absolutely go above this._updateVisibleRows. That's where I had it originally, I don't know why I moved it. Your comment made consider this change in more detail:

1) Table should override View.prototype.sizeDidChange and clear cached values there. This is guaranteed to be called before View.prototype.layout.
2) _cachedScrollableHeight and _cachedHeight refer to the same value. I'll try to simplify this.
3) There is a bug on in Table.prototype._resizeColumnsAndFiller (Table.js:864):

if (this._columnWidths && availableWidth === oldWidth && availableWidth === oldHeight && numberOfRows === oldNumberOfRows) {

Should be:

if (this._columnWidths && availableWidth === oldWidth && availableHeight === oldHeight && numberOfRows === oldNumberOfRows) {

[Matt Baker]

I noticed a few more issues with _resizeColumnsAndFiller:

1) oldWidth and _cachedWidth will always be equal. Seems like this needs to be fixed; we need an _oldCachedWidth that gets updated in sizeDidChange for this to work as originally intended.
2) oldHeight isn't needed. By the time the value is tested, the filler height has been updated, and whether we resize columns doesn't depend on the height.

[Matt Baker]

(In reply to Matt Baker from comment #5)
> I noticed a few more issues with _resizeColumnsAndFiller:
> 
> 1) oldWidth and _cachedWidth will always be equal. Seems like this needs to
> be fixed; we need an _oldCachedWidth that gets updated in sizeDidChange for
> this to work as originally intended.

Correction: as Devin pointed out, this isn't true. oldWidth will either be equal to _cachedWidth, or NaN.

[Matt Baker]

Created attachment 354023 [details]
Patch

[Devin Rousso]

Comment on attachment 354023 [details]
Patch

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

r-, see comments below

> Source/WebInspectorUI/UserInterface/Views/Table.js:534
> +        this._resizeColumnsAndFiller();

I think this will have a different effect than what was there before.  Calling `resize` guaranteed that `_cachedWidth` was recalculated, whereas just calling `_resizeColumnsAndFiller` doesn't.  Setting `_columnWidths` to `null` does ensure that it is recalculated, but it doesn't affect `_cachedWidth`.

> Source/WebInspectorUI/UserInterface/Views/Table.js:844
> +        if (this._columnWidths && this._cachedWidth === this._previousCachedWidth) {

You should update `_previousCachedWidth` after this so that the next time this is called it will be equal.

[Matt Baker]

Comment on attachment 354023 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/Table.js:534
>> +        this._resizeColumnsAndFiller();
> 
> I think this will have a different effect than what was there before.  Calling `resize` guaranteed that `_cachedWidth` was recalculated, whereas just calling `_resizeColumnsAndFiller` doesn't.  Setting `_columnWidths` to `null` does ensure that it is recalculated, but it doesn't affect `_cachedWidth`.

The effect should be the same, let's walk through it to make I didn't break things. Previously we had:

layout()
{
    this._updateVisibleRows();

    if (this.layoutReason === WI.View.LayoutReason.Resize)
        this.resize();
    else
        this._resizeColumnsAndFiller();
}

Which was equivalent to:

layout()
{
    this._updateVisibleRows();

    if (this.layoutReason === WI.View.LayoutReason.Resize) {
        this._cachedWidth = NaN;
        this._heightWidth = NaN;
    }

    this._resizeColumnsAndFiller();
}

I broke out the resize logic, which runs before `layout`, so now we have:

sizeDidChange()
{
    this._cachedWidth = NaN;
    this._heightWidth = NaN;
}

layout()
{
    this._updateVisibleRows();
    this._resizeColumnsAndFiller();
}

During a resize, the logic is different from what we had previously; the cached values are cleared before `_updateVisibleRows` is called. That should be an improvement, since that method will recompute `_cachedHeight` instead of using a stale value. Since `_resizeColumnsAndFiller` only refers to `_cachedWidth` the change should not be visible to that method.

>> Source/WebInspectorUI/UserInterface/Views/Table.js:844
>> +        if (this._columnWidths && this._cachedWidth === this._previousCachedWidth) {
> 
> You should update `_previousCachedWidth` after this so that the next time this is called it will be equal.

Good catch!

[Matt Baker]

Created attachment 354353 [details]
Patch

[Devin Rousso]

Comment on attachment 354023 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/Table.js:534
>>> +        this._resizeColumnsAndFiller();
>> 
>> I think this will have a different effect than what was there before.  Calling `resize` guaranteed that `_cachedWidth` was recalculated, whereas just calling `_resizeColumnsAndFiller` doesn't.  Setting `_columnWidths` to `null` does ensure that it is recalculated, but it doesn't affect `_cachedWidth`.
> 
> The effect should be the same, let's walk through it to make I didn't break things. Previously we had:
> 
> layout()
> {
>     this._updateVisibleRows();
> 
>     if (this.layoutReason === WI.View.LayoutReason.Resize)
>         this.resize();
>     else
>         this._resizeColumnsAndFiller();
> }
> 
> Which was equivalent to:
> 
> layout()
> {
>     this._updateVisibleRows();
> 
>     if (this.layoutReason === WI.View.LayoutReason.Resize) {
>         this._cachedWidth = NaN;
>         this._heightWidth = NaN;
>     }
> 
>     this._resizeColumnsAndFiller();
> }
> 
> I broke out the resize logic, which runs before `layout`, so now we have:
> 
> sizeDidChange()
> {
>     this._cachedWidth = NaN;
>     this._heightWidth = NaN;
> }
> 
> layout()
> {
>     this._updateVisibleRows();
>     this._resizeColumnsAndFiller();
> }
> 
> During a resize, the logic is different from what we had previously; the cached values are cleared before `_updateVisibleRows` is called. That should be an improvement, since that method will recompute `_cachedHeight` instead of using a stale value. Since `_resizeColumnsAndFiller` only refers to `_cachedWidth` the change should not be visible to that method.

The issue I'm referring to has nothing to do with `layout` or `sizeDidChange`.  The previous implementation of `resize` cleared `_cachedWidth`, which you no longer do in this function.  I'd expect the `_cachedWidth` to update whenever a new column is shown via `showColumn`, so you either need to call `this.needsLayout(WI.View.LayoutReason.Resize)` or set `_cachedWidth` to `NaN` here (async vs sync).

Another thing I just noticed is that the same is true for `hideColumn` as well.

[Matt Baker]

Comment on attachment 354023 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/Table.js:534
>>>> +        this._resizeColumnsAndFiller();
>>> 
>>> I think this will have a different effect than what was there before.  Calling `resize` guaranteed that `_cachedWidth` was recalculated, whereas just calling `_resizeColumnsAndFiller` doesn't.  Setting `_columnWidths` to `null` does ensure that it is recalculated, but it doesn't affect `_cachedWidth`.
>> 
>> The effect should be the same, let's walk through it to make I didn't break things. Previously we had:
>> 
>> layout()
>> {
>>     this._updateVisibleRows();
>> 
>>     if (this.layoutReason === WI.View.LayoutReason.Resize)
>>         this.resize();
>>     else
>>         this._resizeColumnsAndFiller();
>> }
>> 
>> Which was equivalent to:
>> 
>> layout()
>> {
>>     this._updateVisibleRows();
>> 
>>     if (this.layoutReason === WI.View.LayoutReason.Resize) {
>>         this._cachedWidth = NaN;
>>         this._heightWidth = NaN;
>>     }
>> 
>>     this._resizeColumnsAndFiller();
>> }
>> 
>> I broke out the resize logic, which runs before `layout`, so now we have:
>> 
>> sizeDidChange()
>> {
>>     this._cachedWidth = NaN;
>>     this._heightWidth = NaN;
>> }
>> 
>> layout()
>> {
>>     this._updateVisibleRows();
>>     this._resizeColumnsAndFiller();
>> }
>> 
>> During a resize, the logic is different from what we had previously; the cached values are cleared before `_updateVisibleRows` is called. That should be an improvement, since that method will recompute `_cachedHeight` instead of using a stale value. Since `_resizeColumnsAndFiller` only refers to `_cachedWidth` the change should not be visible to that method.
> 
> The issue I'm referring to has nothing to do with `layout` or `sizeDidChange`.  The previous implementation of `resize` cleared `_cachedWidth`, which you no longer do in this function.  I'd expect the `_cachedWidth` to update whenever a new column is shown via `showColumn`, so you either need to call `this.needsLayout(WI.View.LayoutReason.Resize)` or set `_cachedWidth` to `NaN` here (async vs sync).
> 
> Another thing I just noticed is that the same is true for `hideColumn` as well.

Why does `_cachedWidth` need to be updated every time a column is hidden/shown? The width of the Table isn't changing, so using the existing `_cachedWidth` makes sense (and if it's NaN it will be computed).

Interestingly, `showColumn` used to call `resize` but `hideColumn` just triggered a (non-resize) layout instead.

[Devin Rousso]

Comment on attachment 354023 [details]
Patch

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

>>>>> Source/WebInspectorUI/UserInterface/Views/Table.js:534
>>>>> +        this._resizeColumnsAndFiller();
>>>> 
>>>> I think this will have a different effect than what was there before.  Calling `resize` guaranteed that `_cachedWidth` was recalculated, whereas just calling `_resizeColumnsAndFiller` doesn't.  Setting `_columnWidths` to `null` does ensure that it is recalculated, but it doesn't affect `_cachedWidth`.
>>> 
>>> The effect should be the same, let's walk through it to make I didn't break things. Previously we had:
>>> 
>>> layout()
>>> {
>>>     this._updateVisibleRows();
>>> 
>>>     if (this.layoutReason === WI.View.LayoutReason.Resize)
>>>         this.resize();
>>>     else
>>>         this._resizeColumnsAndFiller();
>>> }
>>> 
>>> Which was equivalent to:
>>> 
>>> layout()
>>> {
>>>     this._updateVisibleRows();
>>> 
>>>     if (this.layoutReason === WI.View.LayoutReason.Resize) {
>>>         this._cachedWidth = NaN;
>>>         this._heightWidth = NaN;
>>>     }
>>> 
>>>     this._resizeColumnsAndFiller();
>>> }
>>> 
>>> I broke out the resize logic, which runs before `layout`, so now we have:
>>> 
>>> sizeDidChange()
>>> {
>>>     this._cachedWidth = NaN;
>>>     this._heightWidth = NaN;
>>> }
>>> 
>>> layout()
>>> {
>>>     this._updateVisibleRows();
>>>     this._resizeColumnsAndFiller();
>>> }
>>> 
>>> During a resize, the logic is different from what we had previously; the cached values are cleared before `_updateVisibleRows` is called. That should be an improvement, since that method will recompute `_cachedHeight` instead of using a stale value. Since `_resizeColumnsAndFiller` only refers to `_cachedWidth` the change should not be visible to that method.
>> 
>> The issue I'm referring to has nothing to do with `layout` or `sizeDidChange`.  The previous implementation of `resize` cleared `_cachedWidth`, which you no longer do in this function.  I'd expect the `_cachedWidth` to update whenever a new column is shown via `showColumn`, so you either need to call `this.needsLayout(WI.View.LayoutReason.Resize)` or set `_cachedWidth` to `NaN` here (async vs sync).
>> 
>> Another thing I just noticed is that the same is true for `hideColumn` as well.
> 
> Why does `_cachedWidth` need to be updated every time a column is hidden/shown? The width of the Table isn't changing, so using the existing `_cachedWidth` makes sense (and if it's NaN it will be computed).
> 
> Interestingly, `showColumn` used to call `resize` but `hideColumn` just triggered a (non-resize) layout instead.

Oh good point.

[Devin Rousso]

Comment on attachment 354353 [details]
Patch

r=me, nicely done :)

[WebKit Commit Bot]

Comment on attachment 354353 [details]
Patch

Clearing flags on attachment: 354353

Committed r238203: <https://trac.webkit.org/changeset/238203>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.