[webkit-reviews] review granted: [Bug 20040] Vertical border spacing is doubled between table row groups : [Attachment 130629] Patch to remove extra vertical spacing across table sections

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Kishore Bolisetty
<kbolisetty at innominds.com>'s request for review:
Bug 20040: Vertical border spacing is doubled between table row groups
https://bugs.webkit.org/show_bug.cgi?id=20040

Attachment 130629: Patch to remove extra vertical spacing across table sections
https://bugs.webkit.org/attachment.cgi?id=130629&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.