[webkit-reviews] review granted: [Bug 85652] CSS 2.1 failure: table-height-algorithm-012 fails : [Attachment 145868] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Robert Hogan
<robert at webkit.org>'s request for review:
Bug 85652: CSS 2.1 failure: table-height-algorithm-012 fails
https://bugs.webkit.org/show_bug.cgi?id=85652

Attachment 145868: Patch
https://bugs.webkit.org/attachment.cgi?id=145868&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145868&action=review


r=me, with some comment and don't forget to turn the tests into ref-tests.

> LayoutTests/css2.1/20110323/table-height-algorithm-012.htm:25
> +	   <p>Test passes if the bottom of "Filler Text" below is aligned.</p>

I would put it this way:

This test passes if the "Filler Text" are baseline aligned. (baseline has a
signification outside CSS)

The use of the singular is kind of weird in the sentence and bottom is not very
precise. (not repeat for the other tests)

> LayoutTests/fast/css/vertical-align-baseline-rowspan-002.htm:41
> +		   <td></td>

It's technically missing a cell here but it's probably good to see that we
properly baseline align.

> LayoutTests/fast/css/vertical-align-baseline-rowspan-004.htm:24
> +	       #spanning
> +	       {
> +		   vertical-align: baseline;
> +	       }

This is unneeded as td has the same property.

> LayoutTests/fast/css/vertical-align-baseline-rowspan-005.htm:33
> +<!--    The first cell in the row-span is the only one that has a baseline -
the others are vertical-align:top.
> +	   Observing its baseline leads it to align the top of its text to the
top of the text of the other two cells.  -->

This could be dumped in the output as it is a valuable explanation (ojan is a
fan of descriptions in tests and I tend to agree (not repeated for other
description comments).

> LayoutTests/fast/css/vertical-align-baseline-rowspan-005.htm:34
> +	   <p>Test passes if the top of "Filler Text" below is aligned.</p>

This test passes if the cap-height of the "Filler Text" are aligned? (not
repeated for the "top" tests)