[Webkit-unassigned] [Bug 20040] Vertical border spacing is doubled between table row groups

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20040


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #130629|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #25 from Julien Chaffraix <jchaffraix at webkit.org>  2012-03-07 10:35:26 PST ---
(From update of attachment 130629)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.