Bug 85652

Created attachment 142763 [details] Patch

Comment on attachment 142763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142763&action=review > Source/WebCore/rendering/RenderTableSection.cpp:633 > + intrinsicPaddingBefore = getBaseline(cell->rowIndex()) - (b - oldIntrinsicPaddingBefore); For the record, I wonder if this is correct. The baseline on a rowspan is 0 except for the last row per calcRowLogicalHeight's computation. This is causing other bugs with rowspans but for now, it looks like you should be using the last row in the row span. This should work in the children flexing case above as we do the computation for each row in a row span (this is a HUGE FIXME btw).

Comment on attachment 142763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142763&action=review By the way, while touching this code, let's use better names (|b| is really uncool). >> Source/WebCore/rendering/RenderTableSection.cpp:633 >> + intrinsicPaddingBefore = getBaseline(cell->rowIndex()) - (b - oldIntrinsicPaddingBefore); > > For the record, I wonder if this is correct. > > The baseline on a rowspan is 0 except for the last row per calcRowLogicalHeight's computation. This is causing other bugs with rowspans but for now, it looks like you should be using the last row in the row span. This should work in the children flexing case above as we do the computation for each row in a row span (this is a HUGE FIXME btw). After thinking it over, the patch is correct when the baseline is not determined by rowspans. I still think the patch could make us regress some combination of rowspan and baseline alignment (I wish I had a concrete example but I don't have time to look into that) due to calcRowLogicalHeight putting the extra space to the last row in a rowspan. AFAICT our testing is lacking for vertical-align + rowspan. If you added more tests for that (or pointed out some testing I missed), it would help to convince me.

Created attachment 145135 [details] Patch

Comment on attachment 145135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145135&action=review > LayoutTests/ChangeLog:33 > + * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-004-expected.png: Added. > + * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-004-expected.txt: Added. > + * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-005-expected.png: Added. > + * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-005-expected.txt: Added. > + * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-006-expected.png: Added. > + * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-006-expected.txt: Added. I've kept png results here to ease review - I plan to add ref tests to the patch for landing.

Comment on attachment 145135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145135&action=review > Source/WebCore/rendering/RenderTableSection.cpp:633 > LayoutUnit b = cell->cellBaselinePosition(); As mentioned, let's rename |b| to something understandable (baselinePosition or cellBaselinePosition) > LayoutTests/ChangeLog:18 > + * fast/css/vertical-align-baseline-rowspan-001.htm: Added. > + * fast/css/vertical-align-baseline-rowspan-002.htm: Added. > + * fast/css/vertical-align-baseline-rowspan-003.htm: Added. > + These are a few variations on table-height-algorithm-012.htm > + * fast/css/vertical-align-baseline-rowspan-004.htm: Added. > + * fast/css/vertical-align-baseline-rowspan-005.htm: Added. > + * fast/css/vertical-align-baseline-rowspan-006.htm: Added. > + These last 3 test the calculation of the baseline RenderTableSection::calcRowLogicalHeight, > + where we now always use the first row in the span. Without this the 'Filler Text' in the middle > + cell aligns beyond the top of the table. Do you intent to contribute those tests back to the CSS WG? Also all your example have the cells that don't belong to the first one in the rowspan empty (they end up being 2 pixels due to the default border spacing). Would it be possible to give them some real size (maybe also some color) as this would cover more the issue I had in mind? >> LayoutTests/ChangeLog:33 >> + * platform/chromium-linux-x86/fast/css/vertical-align-baseline-rowspan-006-expected.txt: Added. > > I've kept png results here to ease review - I plan to add ref tests to the patch for landing. Thanks for pointing this out. Aren't some of the baselines a bit off? (like for 006, I would have expected the middle text's baseline to be aligned with the other cells' text baselines)

Created attachment 145868 [details] Patch

Comment on attachment 145868 [details] Patch 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)

Landed: http://trac.webkit.org/changeset/120616 webkit-patch didn't update the bug for some reason.