Vertical border spacing is doubled between table row groups

[Ryan Mitchell]

When using thead/tfoot/tbody in tables, some extra white space appears.
Firefox doesn't display this extra space - and CSS does not seem to be able to remove it!

I've uploaded a test case at:
http://rt.to/table-extra-spacing.html

[Daniel Bates]

Created attachment 108970 [details]
table-extra-spacing.html

Although <http://rt.to/table-extra-spacing.html> has existed since 2008 we should archive the test on the bug itself.

[Daniel Bates]

*** Bug 39986 has been marked as a duplicate of this bug. ***

[Daniel Bates]

*** Bug 68193 has been marked as a duplicate of this bug. ***

[Daniel Bates]

In bug #68193 comment 0, Tab Atkin's wrote:

[[
The vertical component of 'border-spacing' is doubled between rows that are part of different row-groups.  In the test case, the first row is in a <thead> and the latter two are in a <tbody>, so the gap between the first and second row is 20px, while the gap between the second and third is 10px, which is what is actually set in the CSS.

(I suspect we're using machinery similar or identical to margin collapsing, and making row groups prevent collapsing across themselves.)
]]

in reference to the following example given as a data URL:

data:text/html;charset=utf-8,<!DOCTYPE%20html>%0A<table>%0A%20<thead>%0A%20%20<tr>%0A%20%20%20<td>head1%0A%20%20%20<td>head2%0A%20<tbody>%0A%20%20<tr>%0A%20%20%20<td>one%0A%20%20%20<td>two%0A%20%20<tr>%0A%20%20%20<td>three%0A%20%20%20<td>four%0A<%2Ftable>%0A<style>%0Atd%20{%20background%3A%20gold%3B%20border-radius%3A%205px%3B%20border%3A%20thin%20solid%20black%3B%20}%0Atable%20{%20border-spacing%3A%200%2010px%3B%20}%0A<%2Fstyle>

[Vamshi Krishna N]

Created attachment 108994 [details]
Fix for doubled vertical border spacing

This is fix for removing vertical spacing between table sections. Applicable for 20040 & 39986.

It is observed that an extra vBorderSpacing is added for every section at the end,that resulted in extra spacing between consecutive sections.Hence, removed that extra spacing from each section and added one vBorderSpacing to the table at the end, so that it will not overlap with the next render element.

[WebKit Review Bot]

Comment on attachment 108994 [details]
Fix for doubled vertical border spacing

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

New failing tests:
css2.1/t090402-c42-ibx-pad-00-d-ag.html
css2.1/t0804-c5506-padn-t-00-b-a.html
css2.1/t040302-c61-ex-len-00-b-a.html
css2.1/t0805-c5513-brdr-bw-00-b.html
css2.1/t0805-c5521-brdr-l-01-e.html
css2.1/t0803-c5503-mrgn-b-00-b-a.html
css2.1/t0805-c5520-brdr-b-01-e.html
css2.1/t0803-c5501-mrgn-t-00-b-a.html
css3/css3-modsel-33.html
css1/box_properties/border_top.html
css2.1/t0805-c5518-brdr-t-01-e.html
css2.1/t080301-c411-vt-mrgn-00-b.html
css1/box_properties/border_left.html
css2.1/t0805-c5519-brdr-r-01-e.html
css2.1/t0805-c5522-brdr-02-e.html
css1/box_properties/border.html
css1/box_properties/border_bottom.html
css2.1/t0804-c5508-ipadn-b-03-b-a.html
css2.1/t0805-c5511-brdr-tw-00-b.html
css1/box_properties/border_right_inline.html

[WebKit Review Bot]

Created attachment 108995 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Vamshi Krishna N]

Created attachment 109279 [details]
patch proposed for "Vertical border spacing doubled between table row groups"

As there is vertical border spacing in each section below and above the content of the section
which makes doubled spacing when more than one sections are present in a table, now the spacing
at the top of each section is removed in each section except for the top section which ensures
removal of doubled spacing.

[WebKit Review Bot]

Attachment 109279 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderTableSection.cpp:325:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/rendering/RenderTableSection.cpp:326:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/rendering/RenderTableSection.cpp:328:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 109279 [details]
patch proposed for "Vertical border spacing doubled between table row groups"

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

New failing tests:
fast/table/multiple-captions-display.xhtml
fast/table/floating-th.html
fast/table/rowindex.html
fast/table/border-collapsing/004.html
fast/table/table-display-types-strict.html
fast/table/table-display-types-vertical.html
media/media-blocked-by-willsendrequest.html
fast/table/frame-and-rules.html
http/tests/security/xssAuditor/cookie-injection.html
fast/table/border-collapsing/004-vertical.html
fast/forms/input-value.html
fast/table/table-display-types.html
fast/innerHTML/006.html
fast/table/011.html
fast/table/tableInsideCaption.html

[Dave Hyatt]

Comment on attachment 109279 [details]
patch proposed for "Vertical border spacing doubled between table row groups"

Fix the failing layout tests and eliminate the tabs please.

[Vamshi Krishna N]

Created attachment 110818 [details]
patch proposed for "Vertical border spacing doubled between table row groups" for the failing test cases with the modified expected results

As each section height is reduced in a table with more than a single section, corresponding test case expected results hence are modified for the correct height calculations for the reduced table heights.

[WebKit Review Bot]

Comment on attachment 110818 [details]
patch proposed for "Vertical border spacing doubled between table row groups" for the failing test cases with the modified expected results

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

New failing tests:
fast/table/multiple-captions-display.xhtml
fast/table/floating-th.html
fast/table/table-display-types.html
fast/forms/input-value.html
fast/table/rowindex.html
fast/table/border-collapsing/004.html
fast/table/table-display-types-strict.html
fast/table/table-display-types-vertical.html
fast/table/frame-and-rules.html
http/tests/security/xssAuditor/cookie-injection.html
fast/table/border-collapsing/004-vertical.html
fast/forms/input-spinbutton-capturing.html
fast/forms/input-number-large-padding.html
fast/forms/input-number-events.html
fast/table/011.html
fast/innerHTML/006.html

[Vamshi Krishna N]

Created attachment 111230 [details]
updated patch for the multiple-captions-display.txt

list of updated the test cases : 
  multiple-captions-display-expected.txt
list of test cases not failing in My test environment :
  fast/forms/input-spinbutton-capturing.html
  fast/forms/input-number-large-padding.html
  fast/forms/input-number-events.html
  http/tests/security/xssAuditor/cookie-injection.html
not update the failing IMAGE only mismatched test cases as discussed in #webkit.
  fast/table/floating-th.html
  fast/table/table-display-types.html
  fast/forms/input-value.html
  fast/table/rowindex.html
  fast/table/border-collapsing/004.html
  fast/table/table-display-types-strict.html
  fast/table/table-display-types-vertical.html
  fast/table/frame-and-rules.html
  fast/table/border-collapsing/004-vertical.html
  fast/table/011.html
  fast/innerHTML/006.html

[WebKit Review Bot]

Comment on attachment 111230 [details]
updated patch for the multiple-captions-display.txt

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

New failing tests:
fast/table/multiple-captions-display.xhtml
fast/table/floating-th.html
tables/mozilla/dom/appendTbodyExpand1.html
fast/table/rowindex.html
fast/table/border-collapsing/004.html
fast/table/table-display-types-strict.html
fast/table/table-display-types-vertical.html
fast/table/frame-and-rules.html
fast/table/border-collapsing/004-vertical.html
fast/forms/input-value.html
fast/innerHTML/006.html
fast/table/table-display-types.html
fast/forms/implicit-submission.html
fast/table/011.html
fast/table/tableInsideCaption.html

[Vamshi Krishna N]

Created attachment 112179 [details]
patch proposed for "Vertical border spacing doubled between table row groups"


This time, we modified the test_expectations.txt temporarily for the failing test cases.
Made almost were 130 odd table related test cases to skip as these requires rebaselining of TEXT and IMAGE.
Based on the feedback,will modify the expected test cases

[Vamshi Krishna N]

Created attachment 113154 [details]
patch for "Vertical border spacing doubled between table row groups" with test_expectations.txt updated for all platforms and added TEXT+IMAGE at non bottom place 


This patch comes with test_expectations.txt updated for chromium,mac,qt,win,gtk with the TEXT+IMAGE logs updated into text_Expectations.txt at non bottom place in this file.

[WebKit Review Bot]

Attachment 113154 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

ERROR: Failed to detect Windows version, assuming latest.
[Errno 2] No such file or directory
ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:4123 Duplicate or ambiguous expectation. svg/zoom/page/zoom-coords-viewattr-01-b.svg
ERROR: Line:4124 Duplicate or ambiguous expectation. svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html
ERROR: Line:4125 Duplicate or ambiguous expectation. fast/backgrounds/size/contain-and-cover-zoomed.html
LayoutTests/platform/chromium/test_expectations.txt:4123:  Duplicate or ambiguous expectation. svg/zoom/page/zoom-coords-viewattr-01-b.svg  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:4124:  Duplicate or ambiguous expectation. svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html  [test/expectations] [5]
LayoutTests/platform/chromium/test_expectations.txt:4125:  Duplicate or ambiguous expectation. fast/backgrounds/size/contain-and-cover-zoomed.html  [test/expectations] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vamshi Krishna N]

Vertical double spacing between row groups is fixed as a side effect for the bug Id 69425, can some body mark this as a duplicate for the Bug Id 69425 and change the status

[Eric Seidel (no email)]

Please obsolete your old patches when uploading new ones.  webkit-patch upload will do that automatically for you.  It's difficult to tell what's actually up for review when 4 patches are marked r?

[Julien Chaffraix]

(In reply to comment #19)
> Vertical double spacing between row groups is fixed as a side effect for the bug Id 69425, can some body mark this as a duplicate for the Bug Id 69425 and change the status

I double checked the test case and we can still see the doubled border-spacing in the table. I don't think this bug is fixed (especially since bug 69425 involves box-sizing: border-box which does not appear in the test case).

[Julien Chaffraix]

Comment on attachment 112179 [details]
patch proposed for "Vertical border spacing doubled between table row groups"

Cleaned up obsolete attachment r?.

[Julien Chaffraix]

Comment on attachment 113154 [details]
patch for "Vertical border spacing doubled between table row groups" with test_expectations.txt updated for all platforms and added TEXT+IMAGE at non bottom place 

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

r- mostly for the missing baselines & test cases. Also there may be a better way to solve the bug.

> Source/WebCore/ChangeLog:7
> +        

Where are the test cases? The bug has a test case that would be a nice addition if it's not properly tested by our existing cases.

> Source/WebCore/rendering/RenderTableSection.cpp:330
> +    if (this == table()->topSection())
> +        m_rowPos[0] = spacing;
> +    else
> +        m_rowPos[0] = 0;

From the ChangeLog explanation, it looks like the issue is that we shouldn't be happily adding the vertical spacing twice inside a section.

If it's the case, this fix is wrong and there should be a way to create a test case where a section overflows where it shouldn't due to the double spacing.

> LayoutTests/ChangeLog:18
> +        * platform/chromium/test_expectations.txt:
> +        * platform/gtk/test_expectations.txt:
> +        * platform/mac/test_expectations.txt:
> +        * platform/qt/test_expectations.txt:
> +        * platform/win/test_expectations.txt:

I would like to see a baseline for one platform before saying if it makes sense to skip (and eventually rebaseline) tons of tests on all platforms.

[Kishore Bolisetty]

Created attachment 130629 [details]
Patch to remove extra vertical spacing across table sections

Hi Julien,
As per your review comments and as discussed in IRC, I baselined the test results for GTK port and modified test_expectations.txt and Skipped files for the other ports.

[Julien Chaffraix]

Comment on attachment 130629 [details]
Patch to remove extra vertical spacing across table sections

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

For this patch to pass the EWS, you need a super up-to-date checkout as chromium's text_expectations is modified a lot. Try to upload a new patch that could pass the EWS as I would like to land it through the commit-queue.

> Source/WebCore/ChangeLog:9
> +        A default 2px vertical border space is used while calculating row logical heights with in a section.
> +        This default 2px is being added both at the top and bottom of a section, thus leaving 4px gap across any two adjacent sections.
> +        This changes ensures only 2px border space across any two adjacent sections.

That is way too precise. Here you fix the case regardless of the vertical border-spacing (it's *border-spacing* not border-space btw).

> Source/WebCore/rendering/RenderTableSection.cpp:337
> +    if (this == table()->topSection())
> +        m_rowPos[0] = spacing;
> +    else
> +        m_rowPos[0] = 0;

I am not super happy about this change and you have completely ignored my previous comment about it.

The current computation has some issues and the most important one is that you include the border-spacing of the next section's in your current section. I am pretty sure this is needed to properly account for it at the table level (per CSS 2.1).

AFAICT your change is correct but hackish. I can't think of a better way to fix that without rethinking how we handle border-spacing. Please add a comment explaining 'why' this is needed. Example:

// We ignore the border-spacing on any non-top section as it is already included in the previous section's last row position.

> LayoutTests/fast/table/table-vertical-space-across-sections.html:27
> +            var testContainer = document.createElement("div");
> +            document.body.appendChild(testContainer);
> +            testContainer.innerHTML =
> +            '<table id="test"> \
> +            <thead> <tr><th></th></tr>                                       </thead> \
> +            <tbody> <tr><td></td></tr> <tr><td></td></tr>                    </tbody> \
> +            <tfoot> <tr><td></td></tr> <tr><td></td></tr> <tr><td></td></tr> </tfoot> \
> +            </table>';

Why do we this code? Just having an inline table would give you the same result and be waaay more readable.

> LayoutTests/fast/table/table-vertical-space-across-sections.html:29
> +            var style = document.defaultView.getComputedStyle(element, null);

You can just use: window.getComputedStyle(element, null) AFAICT.

[Kishore Bolisetty]

Created attachment 130865 [details]
Patch to remove extra vertical spacing across table sections

attempt to turn build bots to green

[WebKit Review Bot]

Comment on attachment 130865 [details]
Patch to remove extra vertical spacing across table sections

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

New failing tests:
fast/table/multiple-captions-display.xhtml
tables/mozilla_expected_failures/bugs/bug106966.html
tables/mozilla/bugs/bug27038-2.html
tables/mozilla/bugs/bug27038-1.html
fast/forms/input-value.html

[Kishore Bolisetty]

Created attachment 130887 [details]
Patch to remove extra vertical spacing across table sections

another attempt to turn build bots green.

[Julien Chaffraix]

Comment on attachment 130887 [details]
Patch to remove extra vertical spacing across table sections

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

Please, don't include unneeded rebaselines (I am responsible for the layer disappearances - see r110072). Those changes you are adding are neat but irrelevant to this bug and hides the important change. We prefer to have very focused patches so that we can catch regressions easily. Those irrelevant baselines should be postponed to another bug that I would happily review!

> Source/WebCore/ChangeLog:9
> +        A default 2px vertical border-spacing is used while calculating row logical heights with in a section.
> +        This border-spacing is being added both at the top and bottom of a section, thus leaving a gap of double border-spacing across
> +        any two adjacent sections.This changes ensures proper border-spacing across any two adjacent sections.

As mentioned, I don't understand why you mention the default border-spacing here. It has very little to do with the problem and hides the fact that we used to count the border-spacing regardless of it.

Also there should be a line between 'Reviewed by' and you description.

And while at it, 
* with in -> within
* top / bottom are ambiguous here and matches your reading direction (horizontally I would bet), if you were for example japanese (reading vertically), those would be right / left so we prefer the name: before / after to be writing-mode agnostic.

> LayoutTests/platform/efl/Skipped:2871
> +# Needs baseline

Nit: EFL does support new-run-webkit-tests and thus normally those changes should have gone into test_expectations.txt.

[Kishore Bolisetty]

(In reply to comment #29)
> (From update of attachment 130887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130887&action=review
> 
> Please, don't include unneeded rebaselines (I am responsible for the layer disappearances - see r110072). Those changes you are adding are neat but irrelevant to this bug and hides the important change. We prefer to have very focused patches so that we can catch regressions easily. Those irrelevant baselines should be postponed to another bug that I would happily review!
> 
> > Source/WebCore/ChangeLog:9
> > +        A default 2px vertical border-spacing is used while calculating row logical heights with in a section.
> > +        This border-spacing is being added both at the top and bottom of a section, thus leaving a gap of double border-spacing across
> > +        any two adjacent sections.This changes ensures proper border-spacing across any two adjacent sections.
> 
> As mentioned, I don't understand why you mention the default border-spacing here. It has very little to do with the problem and hides the fact that we used to count the border-spacing regardless of it.
> 
> Also there should be a line between 'Reviewed by' and you description.
> 
> And while at it, 
> * with in -> within
> * top / bottom are ambiguous here and matches your reading direction (horizontally I would bet), if you were for example japanese (reading vertically), those would be right / left so we prefer the name: before / after to be writing-mode agnostic.
> 
> > LayoutTests/platform/efl/Skipped:2871
> > +# Needs baseline
> 
> Nit: EFL does support new-run-webkit-tests and thus normally those changes should have gone into test_expectations.txt.
Hi Julien,
I will try to provide much cleaner patch next time incorporating all these comments.  Thank you so much for the review comments.

[Kishore Bolisetty]

Created attachment 131319 [details]
Patch to remove extra vertical border-spacing across table sections 

The following are the major updates in this patch.
1. Tried to address previous review comments.
2. corrected the change log to be more meaningful
3. Rebaselined only relevant test on GTK port

[WebKit Review Bot]

Attachment 131319 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 838, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 103, in check
    overrides = self._port_obj.test_expectations_overrides()
AttributeError: 'NoneType' object has no attribute 'test_expectations_overrides'


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kishore Bolisetty]

(In reply to comment #32)
> Attachment 131319 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> Traceback (most recent call last):
>   File "Tools/Scripts/check-webkit-style", line 48, in <module>
>     sys.exit(CheckWebKitStyle().main())
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
>     patch_checker.check(patch)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
>     self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
>     self._processor.process(lines, file_path, **kwargs)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 838, in process
>     checker.check(lines)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 103, in check
>     overrides = self._port_obj.test_expectations_overrides()
> AttributeError: 'NoneType' object has no attribute 'test_expectations_overrides'
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Hi Julien/All,
This is an invalid issue. 
This occurs if style-check is performed on LayoutTests/platform/efl/test_expectations.txt.
i.e., ./Tools/Scripts/check-webkit-style LayoutTests/platform/efl/test_expectations.txt
This issue can be reproduced even on untouched file also.
Its has got nothing to do with my modification on efl test_expectations.
Due this reason, In one of my last patches I added my changes in Skipped file of efl instead of test_expectations. But this time, I created a proper patch based on Julien's review comments to bring to your comments.
Please suggest me how do proceed further.

[Kishore Bolisetty]

(In reply to comment #33)
> (In reply to comment #32)
> > Attachment 131319 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
> > Traceback (most recent call last):
> >   File "Tools/Scripts/check-webkit-style", line 48, in <module>
> >     sys.exit(CheckWebKitStyle().main())
> >   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
> >     patch_checker.check(patch)
> >   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
> >     self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
> >   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
> >     self._processor.process(lines, file_path, **kwargs)
> >   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 838, in process
> >     checker.check(lines)
> >   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 103, in check
> >     overrides = self._port_obj.test_expectations_overrides()
> > AttributeError: 'NoneType' object has no attribute 'test_expectations_overrides'
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> Hi Julien/All,
> This is an invalid issue. 
> This occurs if style-check is performed on LayoutTests/platform/efl/test_expectations.txt.
> i.e., ./Tools/Scripts/check-webkit-style LayoutTests/platform/efl/test_expectations.txt
> This issue can be reproduced even on untouched file also.
> Its has got nothing to do with my modification on efl test_expectations.
> Due this reason, In one of my last patches I added my changes in Skipped file of efl instead of test_expectations. But this time, I created a proper patch based on Julien's review comments to bring to your comments.
> Please suggest me how do proceed further.

**********But this time, I created a proper patch based on Julien's review comments to bring it to your notice (typo )

[Julien Chaffraix]

Comment on attachment 131319 [details]
Patch to remove extra vertical border-spacing across table sections 

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

> LayoutTests/platform/gtk/fast/table/multiple-captions-display-expected.txt:5
> -layer at (72,134) size 450x18
> -  RenderBlock (positioned) {a} at (72,134) size 450x18
> -    RenderText {#text} at (0,0) size 450x17
> -      text run at (0,0) width 450: "PASS: Caption with opacity 0.4 and fixed position of one of the child."
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x584

This rebaseline looks wrong as it means the test is regressing.

> LayoutTests/platform/mac/test_expectations.txt:537
> +BUGWK20040 : tables/mozilla_expected_failures/other/empty_cells.html = TEXT

I think all those should also be IMAGE+TEXT as Mac has image baselines. IIRC the bots don't check the images so I don't think this would turn our bots red but there will be an image diff. You could put IMAGE+TEXT on all platforms as this is what is expected even if they don't have necessarily up-to-date image baselines.

[Julien Chaffraix]

Comment on attachment 131319 [details]
Patch to remove extra vertical border-spacing across table sections 

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

> LayoutTests/fast/table/table-vertical-space-across-sections.html:24
> +            var testContainer = document.createElement("div");
> +            document.body.appendChild(testContainer);
> +            testContainer.innerHTML = '<table id="test"> <thead> <td/> </thead> <td/><td/> <tfoot> <td/> </tfoot> </table>';
> +            var element = document.getElementById("test");
> +            var style = window.getComputedStyle(element, null);

For the record, I mentioned to Kishore that we could just put the HTML for the table directly inside the page instead of injecting it. Also it would be each sections had 2 cells not just the middle one.

[Kishore Bolisetty]

Created attachment 133244 [details]
Patch to remove extra border spacing across table sections

The following are the updates with this patch:

update the test case with review comments.
another temp to pass style and all build bots.

[WebKit Review Bot]

Comment on attachment 133244 [details]
Patch to remove extra border spacing across table sections

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

New failing tests:
fast/table/multiple-captions-display.xhtml
tables/mozilla/bugs/bug27038-2.html
tables/mozilla/bugs/bug27038-1.html

[Kishore Bolisetty]

Comment on attachment 133244 [details]
Patch to remove extra border spacing across table sections

I will provide another patch, so making this one obsolete.

[Kishore Bolisetty]

Comment on attachment 131319 [details]
Patch to remove extra vertical border-spacing across table sections 

I will provide another patch, so making this one obsolete.

[Kishore Bolisetty]

Comment on attachment 130887 [details]
Patch to remove extra vertical spacing across table sections

I will provide another patch, so making this one obsolete.

[Kishore Bolisetty]

Created attachment 134285 [details]
Patch to remove extra border spacing across table sections

[WebKit Review Bot]

Comment on attachment 134285 [details]
Patch to remove extra border spacing across table sections

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

New failing tests:
fast/table/multiple-captions-display.xhtml

[WebKit Review Bot]

Created attachment 134313 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Julien Chaffraix]

Comment on attachment 134285 [details]
Patch to remove extra border spacing across table sections

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

Looks like fast/table/multiple-captions-display.html also needs to be skipped or rebaselined on all platforms.

> LayoutTests/platform/chromium/test_expectations.txt:4126
> +//BUGWK37244 : tables/mozilla/bugs/bug27038-1.html = IMAGE+TEXT
> +//BUGWK37244 : tables/mozilla/bugs/bug27038-2.html = IMAGE+TEXT

That doesn't look right. Why are you removing those entries without rebaselining?

[Kishore Bolisetty]

(In reply to comment #45)
Hi julien, Thank you for the feedback.
> > LayoutTests/platform/chromium/test_expectations.txt:4126
> > +//BUGWK37244 : tables/mozilla/bugs/bug27038-1.html = IMAGE+TEXT
> > +//BUGWK37244 : tables/mozilla/bugs/bug27038-2.html = IMAGE+TEXT
> 
> That doesn't look right. Why are you removing those entries without rebaselining?

Kindly take a look at this link. The above 2 test were baselined. I commented them under BUGWK37244, due to duplicate entries. Instead I should not have added them in BUGWK20040.

https://bugs.webkit.org/attachment.cgi?id=134285&action=diff#LayoutTests/platform/gtk/tables/mozilla/bugs/bug27038-1-expected.txt_sec1

I will try to provide a better patch soon.

[Julien Chaffraix]

Comment on attachment 134285 [details]
Patch to remove extra border spacing across table sections

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

Sorry, it looks like this bug slipped through my mail box. There is still one test failling which mandates another pass.

>> LayoutTests/platform/chromium/test_expectations.txt:4126
>> +//BUGWK37244 : tables/mozilla/bugs/bug27038-2.html = IMAGE+TEXT
> 
> That doesn't look right. Why are you removing those entries without rebaselining?

OK, you really shouldn't touch that as part of this change. Unless they are now passing (in which case you remove them and explain it), your change is already big enough without this orthogonal change.

[Julien Chaffraix]

*** Bug 22640 has been marked as a duplicate of this bug. ***

[Robert Hogan]

*** Bug 93029 has been marked as a duplicate of this bug. ***

[Mark]

Hi,

I just had this issue as well and was quite surprised to see that it's here since 2008! Is there any workaround for this while it isn't fixed yet?

Is there any ETA on a fix for this?

Cheers,
Mark

[Ryosuke Niwa]

Fixed in Blink: https://chromium.googlesource.com/chromium/blink/+/eb615069267f895c59bc576f9d65b3fa5add41e9

[Laszlo Vidacs]

Created attachment 217045 [details]
Patch

[Build Bot]

Comment on attachment 217045 [details]
Patch

Attachment 217045 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22869539

New failing tests:
fast/table/table-display-types-strict.html
tables/mozilla/marvin/backgr_layers-opacity.html
tables/mozilla/marvin/table_rules_groups.html
tables/mozilla/marvin/tbody_align_center.html
tables/mozilla/marvin/backgr_simple-table-cell.html
tables/mozilla/marvin/backgr_simple-table-column-group.html
tables/mozilla/marvin/body_thead.html
fast/table/floating-th.html
accessibility/table-detection.html
fast/table/anonymous-table-section-removed.html
fast/table/border-collapsing/004-vertical.html
fast/forms/input-value.html
tables/mozilla/marvin/tbody_align_char.html
tables/mozilla/dom/appendTbodyExpand1.html
tables/mozilla/marvin/backgr_simple-table-row.html
fast/table/border-collapsing/004.html
fast/table/table-display-types-vertical.html
tables/mozilla/marvin/backgr_simple-table.html
fast/table/table-display-types.html
fast/table/multiple-captions-display.xhtml
tables/mozilla/marvin/backgr_position-table.html
tables/mozilla/bugs/bug3263.html
fast/table/011.html
fast/table/rowindex.html
tables/mozilla/marvin/backgr_simple-table-row-group.html
fast/table/frame-and-rules.html
tables/mozilla/marvin/backgr_simple-table-column.html
tables/mozilla/marvin/tbody_align_justify.html
tables/mozilla/marvin/body_tfoot.html
fast/table/tableInsideCaption.html

[Build Bot]

Created attachment 217054 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Laszlo Vidacs]

Created attachment 217202 [details]
Patch

[Csaba Osztrogonác]

If the Mac EWS is unable to create results for rebasing tests in a day,
it would be better to land this patch as is and then rebase the tests
from the buildbots.

[Build Bot]

Comment on attachment 217202 [details]
Patch

Attachment 217202 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/27408093

New failing tests:
editing/unsupported-content/list-type-before.html
fast/table/table-display-types-strict.html
tables/mozilla/marvin/backgr_layers-opacity.html
tables/mozilla/bugs/bug26178.html
fast/table/011.html
editing/unsupported-content/table-delete-002.html
editing/unsupported-content/list-delete-001.html
accessibility/table-detection.html
fast/table/anonymous-table-section-removed.html
fast/table/border-collapsing/004-vertical.html
fast/forms/input-value.html
tables/mozilla/dom/appendTbodyExpand1.html
tables/mozilla/marvin/backgr_position-table.html
fast/table/border-collapsing/004.html
fast/table/table-display-types-vertical.html
tables/mozilla/bugs/bug19061-1.html
editing/unsupported-content/table-type-before.html
tables/mozilla/bugs/bug13118.html
editing/unsupported-content/table-type-after.html
fast/table/table-display-types.html
fast/table/multiple-captions-display.xhtml
fast/table/floating-th.html
tables/mozilla/bugs/bug3263.html
fast/table/rowindex.html
editing/unsupported-content/list-delete-003.html
fast/table/frame-and-rules.html
editing/unsupported-content/list-type-after.html
tables/mozilla/bugs/bug10296-1.html
tables/mozilla/bugs/bug19061-2.html
fast/table/tableInsideCaption.html

[Build Bot]

Created attachment 217290 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 217202 [details]
Patch

Attachment 217202 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/29988031

New failing tests:
fast/table/table-display-types-strict.html
tables/mozilla/marvin/backgr_layers-opacity.html
tables/mozilla/marvin/backgr_simple-table-cell.html
tables/mozilla/marvin/backgr_simple-table-column-group.html
tables/mozilla/marvin/body_thead.html
fast/table/floating-th.html
tables/mozilla/marvin/backgr_simple-table-column.html
accessibility/table-detection.html
fast/table/anonymous-table-section-removed.html
fast/table/011.html
fast/forms/input-value.html
tables/mozilla/dom/appendTbodyExpand1.html
tables/mozilla/marvin/backgr_simple-table-row.html
fast/table/border-collapsing/004.html
fast/table/table-display-types-vertical.html
tables/mozilla/bugs/bug19061-1.html
tables/mozilla/marvin/backgr_simple-table.html
tables/mozilla/bugs/bug13118.html
fast/table/table-display-types.html
fast/table/multiple-captions-display.xhtml
tables/mozilla/marvin/backgr_position-table.html
tables/mozilla/bugs/bug3263.html
platform/mac/editing/deleting/deletionUI-single-instance.html
fast/table/rowindex.html
tables/mozilla/marvin/backgr_simple-table-row-group.html
fast/table/frame-and-rules.html
fast/table/border-collapsing/004-vertical.html
tables/mozilla/bugs/bug10296-1.html
tables/mozilla/marvin/body_tfoot.html
fast/table/tableInsideCaption.html

[Build Bot]

Created attachment 217376 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Laszlo Vidacs]

Created attachment 217554 [details]
Patch

[Build Bot]

Comment on attachment 217554 [details]
Patch

Attachment 217554 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/31788045

New failing tests:
tables/mozilla_expected_failures/bugs/bug51000.html
tables/mozilla/marvin/table_rules_groups.html
tables/mozilla/bugs/bug26178.html
tables/mozilla_expected_failures/dom/insertTbodyRebuild1.html
tables/mozilla/bugs/bug46268-3.html
tables/mozilla_expected_failures/bugs/bug106966.html
tables/mozilla/bugs/bug38916.html
tables/mozilla/bugs/bug220536.html
tables/mozilla/bugs/bug57378.html
tables/mozilla/bugs/bug27038-1.html
tables/mozilla/bugs/bug30418.html
tables/mozilla_expected_failures/marvin/backgr_border-table-column-group.html
tables/mozilla/bugs/bug119786.html
fast/forms/input-value.html
tables/mozilla/bugs/bug27038-2.html
tables/mozilla_expected_failures/core/captions1.html
tables/mozilla_expected_failures/marvin/backgr_border-table-column.html
tables/mozilla/bugs/bug46924.html
tables/mozilla/bugs/bug278385.html
tables/mozilla/bugs/bug46268.html
tables/mozilla_expected_failures/dom/insertTbodyExpand1.html
tables/mozilla_expected_failures/bugs/bug4294.html
tables/mozilla/bugs/bug19061-2.html
tables/mozilla_expected_failures/core/backgrounds.html
tables/mozilla/bugs/bug55789.html
tables/mozilla_expected_failures/marvin/backgr_border-table-cell.html
tables/mozilla/bugs/bug46268-5.html
tables/mozilla/bugs/bug27038-3.html
tables/mozilla_expected_failures/bugs/bug46268-4.html

[Build Bot]

Created attachment 217561 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Csaba Osztrogonác]

Comment on attachment 217554 [details]
Patch

r=me and let's update the results from the bots.

[Csaba Osztrogonác]

Comment on attachment 217554 [details]
Patch

Clearing flags on attachment: 217554

Committed r159747: <http://trac.webkit.org/changeset/159747>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

Alas, this is consistently crashing a couple of tests on the Mac Mountain Lion bots.

+tables/mozilla/bugs/bug46268-3.html	crash log sample	history
+tables/mozilla_expected_failures/bugs/bug46268-4.html	crash log sample	history

Application Specific Information:
CRASHING TEST: tables/mozilla/bugs/bug46268-3.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010fee957a WTFCrash + 42 (Assertions.cpp:341)
1   com.apple.WebCore             	0x0000000112d6ab44 WebCore::RenderObject::drawLineForBoxSide(WebCore::GraphicsContext*, int, int, int, int, WebCore::BoxSide, WebCore::Color, WebCore::EBorderStyle, int, int, bool) + 14388 (RenderObject.cpp:1004)
2   com.apple.WebCore             	0x0000000112c43892 WebCore::RenderBoxModelObject::paintOneBorderSide(WebCore::GraphicsContext*, WebCore::RenderStyle const*, WebCore::RoundedRect const&, WebCore::RoundedRect const&, WebCore::IntRect const&, WebCore::BoxSide, WebCore::BoxSide, WebCore::BoxSide, WebCore::BorderEdge const*, WebCore::Path const*, WebCore::BackgroundBleedAvoidance, bool, bool, bool, WebCore::Color const*) + 2338 (RenderBoxModelObject.cpp:1699)
3   com.apple.WebCore             	0x0000000112c45d93 WebCore::RenderBoxModelObject::paintBorderSides(WebCore::GraphicsContext*, WebCore::RenderStyle const*, WebCore::RoundedRect const&, WebCore::RoundedRect const&, WebCore::IntPoint const&, WebCore::BorderEdge const*, unsigned int, WebCore::BackgroundBleedAvoidance, bool, bool, bool, WebCore::Color const*) + 2067 (RenderBoxModelObject.cpp:1760)
4   com.apple.WebCore             	0x0000000112c473bf WebCore::RenderBoxModelObject::paintBorder(WebCore::PaintInfo const&, WebCore::LayoutRect const&, WebCore::RenderStyle const*, WebCore::BackgroundBleedAvoidance, bool, bool) + 3807 (RenderBoxModelObject.cpp:1972)
5   com.apple.WebCore             	0x0000000112e272d9 WebCore::RenderTableCell::paintBoxDecorations(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 521 (RenderTableCell.cpp:1332)
6   com.apple.WebCore             	0x0000000112bbdf9d WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 397 (RenderBlock.cpp:2500)
7   com.apple.WebCore             	0x0000000112bbbbd4 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 404 (RenderBlock.cpp:2204)
8   com.apple.WebCore             	0x0000000112e2528a WebCore::RenderTableCell::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 106 (RenderTableCell.cpp:1043)
9   com.apple.WebCore             	0x0000000112e318cc WebCore::RenderTableSection::paintCell(WebCore::RenderTableCell*, WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 412 (RenderTableSection.cpp:1012)
10  com.apple.WebCore             	0x0000000112e30db8 WebCore::RenderTableSection::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 2312 (RenderTableSection.cpp:1289)
11  com.apple.WebCore             	0x0000000112e303e5 WebCore::RenderTableSection::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 245 (RenderTableSection.cpp:960)

[Dean Jackson]

We're getting an ASSERT because y2 < y1 - deep down in the calculation for RenderTableSection we're calculating a height of -18.

I'll skip these two tests for now because it would be a huge pain to rollout the many commits here.

[Dean Jackson]

webkit.org/b/124857