Bug 99861 – RenderTableCol::computePreferredLogicalWidths should never be called

• Status: REOPENED
• Component: Tables

[Julien Chaffraix]

<col> and <colgroup> don't really have the concept of preferred logical widths as they have no content. We use their styling information to size the cell's logical width.

There are several (forceful) calls to RenderTableCol::computePreferredLogicalWidths in the code because we need to propagate any preferred logical widths dirtying to the table but that's just artificial.

Forwarding any layout hint to the table would remove some clunky code inside table. Patch forthcoming.

[Julien Chaffraix]

Created attachment 169670 [details]
Proposed change: Enforced that RenderTableCol::computePreferredLogicalWidths is never called, forward any layout hint to the table and clean-up AutoTableLayout.cpp.

[WebKit Review Bot]

Comment on attachment 169670 [details]
Proposed change: Enforced that RenderTableCol::computePreferredLogicalWidths is never called, forward any layout hint to the table and clean-up AutoTableLayout.cpp.

Attachment 169670 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14456848

New failing tests:
fast/table/border-collapsing/cached-change-colgroup-border-width.html

[Julien Chaffraix]

(In reply to comment #2)
> (From update of attachment 169670 [details])
> Attachment 169670 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14456848
> 
> New failing tests:
> fast/table/border-collapsing/cached-change-colgroup-border-width.html

The pixel difference is a regression. We are now repainting the whole table instead of just the colgroup. This is due to LayoutRepainter picking up the layout cue.

[Julien Chaffraix]

While fiddling around, I found out that we also call layout() on our table col / colgroup. We should be consistent between the 2 methods so we should also patch layout().

[Julien Chaffraix]

Created attachment 169952 [details]
Updated patch: also changed layout(). Added a test for a case the previous patch broke.

[Ojan Vafai]

Comment on attachment 169952 [details]
Updated patch: also changed layout(). Added a test for a case the previous patch broke.

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

> Source/WebCore/rendering/RenderTable.cpp:381
> +    // We ignore table col / colgroup in this iteration as they are used to size the cell's widths during auto / fixed table layout.

s/as they are/as they are only/ ?

> Source/WebCore/rendering/RenderTableCol.cpp:136
> +    // Also we don't clear any flags, which may never mark the table.

Don't understand this sentence. Please clarify.

[Julien Chaffraix]

Comment on attachment 169952 [details]
Updated patch: also changed layout(). Added a test for a case the previous patch broke.

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

>> Source/WebCore/rendering/RenderTable.cpp:381
>> +    // We ignore table col / colgroup in this iteration as they are used to size the cell's widths during auto / fixed table layout.
> 
> s/as they are/as they are only/ ?

Sure.

>> Source/WebCore/rendering/RenderTableCol.cpp:136
>> +    // Also we don't clear any flags, which may never mark the table.
> 
> Don't understand this sentence. Please clarify.

Will change it to:

// Also we don't clear the needs layout / preferred logical widths dirty flags so the marking logic may never reach the table.

I hope this is clearer.

[Ojan Vafai]

Comment on attachment 169952 [details]
Updated patch: also changed layout(). Added a test for a case the previous patch broke.

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

>>> Source/WebCore/rendering/RenderTableCol.cpp:136
>>> +    // Also we don't clear any flags, which may never mark the table.
>> 
>> Don't understand this sentence. Please clarify.
> 
> Will change it to:
> 
> // Also we don't clear the needs layout / preferred logical widths dirty flags so the marking logic may never reach the table.
> 
> I hope this is clearer.

I still don't see what this says that the above comment doesn't. Not a big deal.

[Julien Chaffraix]

Comment on attachment 169952 [details]
Updated patch: also changed layout(). Added a test for a case the previous patch broke.

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

>>>> Source/WebCore/rendering/RenderTableCol.cpp:136
>>>> +    // Also we don't clear any flags, which may never mark the table.
>>> 
>>> Don't understand this sentence. Please clarify.
>> 
>> Will change it to:
>> 
>> // Also we don't clear the needs layout / preferred logical widths dirty flags so the marking logic may never reach the table.
>> 
>> I hope this is clearer.
> 
> I still don't see what this says that the above comment doesn't. Not a big deal.

OK, let's remove it then.

[Julien Chaffraix]

Created attachment 169995 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 169995 [details]
Patch for landing

Clearing flags on attachment: 169995

Committed r132149: <http://trac.webkit.org/changeset/132149>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 100098

[Julien Chaffraix]

Created attachment 170678 [details]
Updated change: removed ASSERT_NOT_REACHED() from layout() as simplified normal flow layout hit it.

[Ojan Vafai]

Comment on attachment 170678 [details]
Updated change: removed ASSERT_NOT_REACHED() from layout() as simplified normal flow layout hit it.

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

Would be nice if you could add a targetted test for the simplified layout case as well. I realize the inspector tests hit that code accidentally, but it'd be nice to cover it explicitly. Not a big deal.

> LayoutTests/fast/table/col-span-change-relayout.html:41
> +            {
> +                var col = document.getElementById('column');
> +                var oldCellWidth = getComputedStyle(document.getElementById('table')).width;
> +                col.setAttribute("span", "1");
> +                var newCellWidth = getComputedStyle(document.getElementById('table')).width;
> +
> +                log("Table width was " + oldCellWidth);
> +                log("Table width is " + newCellWidth);
> +                if (oldCellWidth != newCellWidth)
> +                    log("PASSED: Table changed width");
> +                else
> +                    log("FAILED: Table did not change width");
> +            }

FWIW, you could make this a check-layout.js change.

function changeColSpan()
{
    document.getElementById('table').setAttribute('data-expected-width', 300);
    checkLayout('table');
    document.getElementById('table').setAttribute('data-expected-width', 200);
    checkLayout('table');
}

I find that easier to read, also, I prefer for it to say the exact width's expected. These shouldn't be platform dependent in anyway, so we should be able to make this test stricter.

[Julien Chaffraix]

Comment on attachment 170678 [details]
Updated change: removed ASSERT_NOT_REACHED() from layout() as simplified normal flow layout hit it.

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

> Would be nice if you could add a targetted test for the simplified layout case as well. I realize the inspector tests hit that code accidentally, but it'd be nice to cover it explicitly. Not a big deal.

Totally agree, added a test to cover that.

>> LayoutTests/fast/table/col-span-change-relayout.html:41
>> +            }
> 
> FWIW, you could make this a check-layout.js change.
> 
> function changeColSpan()
> {
>     document.getElementById('table').setAttribute('data-expected-width', 300);
>     checkLayout('table');
>     document.getElementById('table').setAttribute('data-expected-width', 200);
>     checkLayout('table');
> }
> 
> I find that easier to read, also, I prefer for it to say the exact width's expected. These shouldn't be platform dependent in anyway, so we should be able to make this test stricter.

Done.

[Julien Chaffraix]

Created attachment 170871 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 170871 [details]
Patch for landing

Clearing flags on attachment: 170871

Committed r132612: <http://trac.webkit.org/changeset/132612>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 100512

[Julien Chaffraix]

Not doing a fake layout pass would trigger an ASSERT on one test. This means that the layout() part of the change is not that great (we could still keep the current behavior in debug but that feels bad).

Will post an - hopefully - final patch without the layout() part of the change. It will be landed tomorrow if the EWS are fine and no one objects.

[Julien Chaffraix]

Created attachment 171677 [details]
Updated patch: Removed the layout() change as we need to clear up the review flag or trip on some ASSERT.

[Julien Chaffraix]

Comment on attachment 171677 [details]
Updated patch: Removed the layout() change as we need to clear up the review flag or trip on some ASSERT.

Ojan landed a change that goes in the same direction for RenderTableCol::computePreferredLogicalWidths in bug 106931.

[Ahmad Saleem]

Is this still required? Because Comment 23 mentioned that another bug was able to tackle same issue as this bug and this one landed in Comment 19 but cause regressions (from Comment 20).

Appreciate if someone can mark this bug accordingly. Thanks!