[webkit-reviews] review granted: [Bug 91137] REGRESSION(r117339): cell in block-level table in inline-block are aligned with their last line box : [Attachment 156740] Proposed fix 1: Fix the baseline table logic to match the discussion on www-style.

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 91137: REGRESSION(r117339): cell in block-level table in inline-block are
aligned with their last line box
https://bugs.webkit.org/show_bug.cgi?id=91137

Attachment 156740: Proposed fix 1: Fix the baseline table logic to match the
discussion on www-style.
https://bugs.webkit.org/attachment.cgi?id=156740&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=156740&action=review


This change looks sane to me, but I don't know much about RenderTable.	Maybe
give Hyatt a day or two to take a look before landing?

> Source/WebCore/rendering/RenderTable.cpp:1226
> +    // The baseline of a 'table' is the same as the 'inline-table' baseline
per CSS 3 Flexbox (CSS 2.1

Huh, it's interesting that this detail is in the flexbox spec.

> Source/WebCore/rendering/RenderTable.h:254
> +    virtual LayoutUnit baselinePosition(FontBaseline, bool,
LineDirectionMode, LinePositionMode) const OVERRIDE;

Nit: I would copy this line from RenderBlock.h (i.e., name the bool and make
sure that LinePositionMode has a default value).

> LayoutTests/fast/table/anonymous-table-no-baseline-align-expected.html:12
> +    display: table-cell;

Nit: When making a ref test, I think it's slightly better if the -expected.html
doesn't use the element your testing.  E.g., if you can get the same layout
without using tables, then that would be preferred.  Maybe that's not possible
in this test.