Bug 95521

Summary: Tables without any descendant and auto logical width should have a 0px logical width
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: TablesAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, dglazkov, eric, gyuyoung.kim, hyatt, inferno, mitz, pravind.2k4, rakuco, robert, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test case
none
Proposed change: fixed the bug + rebaseline.
none
Proposed change #2, with moar rebaselining.
none
Patch for landing
none
Patch for landing
none
Patch for the EWS. Only change is LayoutTests/fast/forms/file/file-input-disabled.html (empty table progression).
none
Patch for landing none

Description Julien Chaffraix 2012-08-30 19:08:42 PDT
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).
Comment 1 Julien Chaffraix 2012-08-31 14:36:20 PDT
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.
Comment 2 Julien Chaffraix 2012-09-04 19:44:50 PDT
Created attachment 162150 [details]
Proposed change: fixed the bug + rebaseline.
Comment 3 WebKit Review Bot 2012-09-05 08:13:19 PDT
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
Comment 4 Julien Chaffraix 2012-09-05 09:42:21 PDT
Created attachment 162269 [details]
Proposed change #2, with moar rebaselining.
Comment 5 Abhishek Arya 2012-09-05 16:08:25 PDT
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.
Comment 6 Julien Chaffraix 2012-09-07 14:06:17 PDT
Created attachment 162864 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-09-07 14:20:23 PDT
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
Comment 8 Julien Chaffraix 2012-09-07 14:30:26 PDT
Created attachment 162871 [details]
Patch for landing
Comment 9 Julien Chaffraix 2012-09-07 17:01:05 PDT
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.
Comment 10 Julien Chaffraix 2012-09-07 18:54:02 PDT
Created attachment 162929 [details]
Patch for the EWS. Only change is LayoutTests/fast/forms/file/file-input-disabled.html (empty table progression).
Comment 11 Julien Chaffraix 2012-09-18 10:26:20 PDT
Created attachment 164583 [details]
Patch for landing
Comment 12 WebKit Review Bot 2012-09-18 10:50:59 PDT
Comment on attachment 164583 [details]
Patch for landing

Clearing flags on attachment: 164583

Committed r128906: <http://trac.webkit.org/changeset/128906>
Comment 13 WebKit Review Bot 2012-09-18 10:51:04 PDT
All reviewed patches have been landed.  Closing bug.