Bug 95521 – Tables without any descendant and auto logical width should have a 0px logical width

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Julien Chaffraix]

Created attachment 161604 [details]
Test case

The current table logic wrongly handles empty tables and still allocates some width for empty tables (ie tables without any cells).

This can be seen in our existing tests:
* http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-win/fast/table/inline-form-assert-expected.txt (final table)
* http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug113235-2-expected.txt
* http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug56405-expected.txt (first table)

There are several bugs leading to this bug: border-spacing is added even if we have no cells (see RenderTable::bordersPaddingAndSpacingInRowDirection) but also because the column counting logic doesn't work well (sections have one column by default regardless of the content).

[Julien Chaffraix]

Changing the title to 'without any descendant' as 'border-spacing' can make a table have a logical width if we have a row / row-group. Whether to apply 'border-spacing' to empty rows is something browsers don't agree on so I don't want to touch it.

[Julien Chaffraix]

Created attachment 162150 [details]
Proposed change: fixed the bug + rebaseline.

[WebKit Review Bot]

Comment on attachment 162150 [details]
Proposed change: fixed the bug + rebaseline.

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

New failing tests:
fast/invalid/018.html
fast/invalid/table-residual-style-crash.html
fast/forms/file/file-input-disabled.html
fast/invalid/017.html
fast/invalid/020.xml
fast/invalid/residual-style.html
fast/invalid/table-inside-stray-table-content.html

[Julien Chaffraix]

Created attachment 162269 [details]
Proposed change #2, with moar rebaselining.

[Abhishek Arya]

Comment on attachment 162269 [details]
Proposed change #2, with moar rebaselining.

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

> Source/WebCore/ChangeLog:9
> +        width even if we didn't have a column.

Please add information on how we match Firefox and Opera, and bug in IE.

> Source/WebCore/rendering/RenderTable.h:185
> +            return (effectiveColumnCount + 1) * hBorderSpacing();

Please do the static_cast<LayoutUnit>(numEffCols() + 1). We don't want to overflow here since it wont use the code path of saturated arithmetic in fractional layout unit.

[Julien Chaffraix]

Created attachment 162864 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162864 [details]
Patch for landing

Rejecting attachment 162864 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
eCellElement.o] Error 1
make: *** Waiting for unfinished jobs....
cc1plus: warnings being treated as errors
In file included from Source/WebCore/html/HTMLTableElement.cpp:41:
Source/WebCore/rendering/RenderTable.h: In member function 'WebCore::LayoutUnit WebCore::RenderTable::borderSpacingInRowDirection() const':
Source/WebCore/rendering/RenderTable.h:184: error: unused variable 'effectiveColumnCount'
make: *** [out/Release/obj.target/webcore_html/Source/WebCore/html/HTMLTableElement.o] Error 1

Full output: http://queues.webkit.org/results/13785604

[Julien Chaffraix]

Created attachment 162871 [details]
Patch for landing

[Julien Chaffraix]

Comment on attachment 162871 [details]
Patch for landing

Removing the patch from the cq as I forgot at least another rebaselining in fast/forms/file/file-input-disabled.html. Letting the EWS run for real.

[Julien Chaffraix]

Created attachment 162929 [details]
Patch for the EWS. Only change is LayoutTests/fast/forms/file/file-input-disabled.html (empty table progression).

[Julien Chaffraix]

Created attachment 164583 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164583 [details]
Patch for landing

Clearing flags on attachment: 164583

Committed r128906: <http://trac.webkit.org/changeset/128906>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.