Bug 20040

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.

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

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

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>

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.

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

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

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.

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.

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

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.

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.

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

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

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

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

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.

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.

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

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?

(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).

Comment on attachment 112179 [details] patch proposed for "Vertical border spacing doubled between table row groups" Cleaned up obsolete attachment r?.

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.

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.

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.

Created attachment 130865 [details] Patch to remove extra vertical spacing across table sections attempt to turn build bots to green

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

Created attachment 130887 [details] Patch to remove extra vertical spacing across table sections another attempt to turn build bots green.

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.

(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.

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

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.

(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.

(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 )

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.

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.

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.

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

Comment on attachment 133244 [details] Patch to remove extra border spacing across table sections I will provide another patch, so making this one obsolete.

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.

Comment on attachment 130887 [details] Patch to remove extra vertical spacing across table sections I will provide another patch, so making this one obsolete.

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

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

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

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?

(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.

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.

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

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

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

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

Created attachment 217045 [details] Patch

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

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

Created attachment 217202 [details] Patch

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.

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

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

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

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

Created attachment 217554 [details] Patch

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

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

Comment on attachment 217554 [details] Patch r=me and let's update the results from the bots.

Comment on attachment 217554 [details] Patch Clearing flags on attachment: 217554 Committed r159747: <http://trac.webkit.org/changeset/159747>

All reviewed patches have been landed. Closing bug.

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)

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.