Bug 91137

Created attachment 152033 [details] Reproduction - the 2 boxes should be aligned

*** Bug 91136 has been marked as a duplicate of this bug. ***

WebKit matches Opera here btw. :)

Hi Gérard, Do you have a view on this one? Should adjacent inline-blocks interact with each other like this or not? Thanks, Robert

I am going to attach a patch to this bug that solves the issue to get it out of my repository. It's not meant for review until Gérard has replied.

Created attachment 152055 [details] WIP patch: solves the issue, some testing but not much. Comments welcome.

> Do you have a view on this one? > Should adjacent inline-blocks interact with each other like this or not? The test of attachment 152033 [details] should indicate what are the expected results in an non-equivocal manner. I think middle-vertical-alignment of cells decides here: both cells should be vertically-aligned like no5 of the diagram in section 17.5.3 with legend "Diagram showing the effect of various values of 'vertical-align' on table cells." http://www.w3.org/TR/CSS21/tables.html#height-layout Ideally, you would need a test like http://test.csswg.org/suites/css2.1/nightly-unstable/html4/vertical-align-applies-to-007.htm and then modify it to emulate the test of attachment 152033 [details] --------- Btw, I am using Chrome 20.0.1132.57 and none of the tests in bug 23544 are passed by Chrome 20.0.1132.57. So, I'm still wondering if/when I will be able to check these with Chrome or Safari. Gérard

Thanks for the comment, Gérard. (In reply to comment #7) > > Do you have a view on this one? > Should adjacent inline-blocks interact with each other like this or not? > > The test of attachment 152033 [details] should indicate what are the expected results in an non-equivocal manner. I thought the output was clear here but it's likely me staring at this test for too long: the 2 green boxes should be vertically aligned for the test to pass. > I think middle-vertical-alignment of cells decides here: both cells should be vertically-aligned like no5 of the diagram in section 17.5.3 with legend "Diagram showing the effect of various values of 'vertical-align' on table cells." > http://www.w3.org/TR/CSS21/tables.html#height-layout For some reason, the vertical-align: middle is needed in WebKit to trigger this bug but I don't think this is the core of the issue: the 2 anonymous tables have only one cell that should have the same size as the table (modulo the border-spacing). It looks like WebKit/ Opera are aligning the anonymous tables' last line boxes where FF / IE 9 are aligning their baselines. > Btw, I am using Chrome 20.0.1132.57 and none of the tests in bug 23544 are passed by Chrome 20.0.1132.57. > > So, I'm still wondering if/when I will be able to check these with Chrome or Safari. Chrome 20 was branched at WebKit r116185 which is why you don't see it. You need M21+ to get this change. For Safari, you can always use a nightly for testing (http://nightly.webkit.org/).

(In reply to comment #8) > Thanks for the comment, Gérard. > > (In reply to comment #7) > > > Do you have a view on this one? > Should adjacent inline-blocks interact with each other like this or not? > > > > The test of attachment 152033 [details] [details] should indicate what are the expected results in an non-equivocal manner. > > I thought the output was clear here The test pass/fail condition of attachment 152033 [details] could be clearer and the test could be designed to be more demanding, requiring so that we really challenge implementations. Often, vertical-align tests require very precise layout. Ideally, in testing, an inline-block should always have at least (minimum) 2 blocks. Something like (using inline style for demo purpose only): <div>LLL <div style="display: inline-block;"> <span style="display: block;">TOP</span> <span style="display: block;">LLL</span> </div>LLL </div> with the bottom part of all 9 "L" glyohs all vertically aligned and emulating the baseline. If an inline-block only has 1 sole child (table or block, whatever), then the whole inline-block should behave like an inline. We have 13 tests at the end of http://test.csswg.org/suites/css2.1/nightly-unstable/html4/chapter-10.htm#s10.8.1 section (all tests starting with vertical-align-baseline-*) and I would not be surprised if a few of them were weak or not very trustworthy. > but it's likely me staring at this test for too long: the 2 green boxes > should be vertically aligned for the test to pass. > > > I think middle-vertical-alignment of cells decides here: both cells should be vertically-aligned like no5 of the diagram in section 17.5.3 with legend "Diagram showing the effect of various values of 'vertical-align' on table cells." > > http://www.w3.org/TR/CSS21/tables.html#height-layout > One detail to have in mind is Issue 26 Does vertical-align affect cell contents or cell's content box? http://wiki.csswg.org/spec/css2.1#issue-26 > For some reason, the vertical-align: middle is needed in WebKit to trigger this bug but I don't think this is the core of the issue: the 2 anonymous tables have only one cell that should have the same size as the table (modulo the border-spacing). It looks like WebKit/ Opera are aligning the anonymous tables' last line boxes where FF / IE 9 are aligning their baselines. > If a table is contained by an inline element (and this is the case here), then the anonymous tables should be inline-tables. Bullet 3 "Generate missing parents" of section 17.2.1 http://www.w3.org/TR/CSS2/tables.html#anonymous-boxes states this, as far as I can say. But inline-blocks are also supposed to be laid out as inline for itself (on the baseline) but its own children content are supposed to behave like blocks with the last one sitting on the baseline. "[inline-block] causes an element to generate an inline-level block container. The inside of an inline-block is formatted as a block box, and the element itself is formatted as an atomic inline-level box. " <deep breath> You have definitely a difficult, challenging situation here and .. I'm supposed to be taking a vacation/break these days to recuperate, recharge my batteries :) . > > Btw, I am using Chrome 20.0.1132.57 and none of the tests in bug 23544 are passed by Chrome 20.0.1132.57. > > > > So, I'm still wondering if/when I will be able to check these with Chrome or Safari. > > Chrome 20 was branched at WebKit r116185 which is why you don't see it. You need M21+ to get this change. For Safari, you can always use a nightly for testing (http://nightly.webkit.org/). I'm very often under Linux KDE; I'm now using Chromium which seems to be lagging 2 whole releases behind Chrome in terms of stable releases. ------- Okay. I suggest you try to improve the test, maybe create a few others and present these in www-style mailing list. Gérard P.S.: I met Simon Fraser at Test the Web Forward in San Francisco on june 15th and 16th; were you attending, Julien? Were you there, Robert? Just asking.. am just curious..

> If a table is contained by an inline element (and this is the case here), then the anonymous tables should be inline-tables. Bullet 3 "Generate missing parents" of section 17.2.1 > http://www.w3.org/TR/CSS2/tables.html#anonymous-boxes > states this, as far as I can say. WebKit doesn't properly implement that so the anonymous tables are blocks. See bug 15365. Maybe fixing this bug would help us. I will improve the test case(s) and follow off bugzilla for the other questions.

OK, here is my understanding of why our new baseline is wrong and why this is a bug. The baseline of an inline-block is defined as the "baseline of its last line box in the normal flow, unless it has either no in-flow line boxes or if its 'overflow' property has a computed value other than 'visible', in which case the baseline is the bottom margin edge." [1]. Because an inline-block creates an inline formatting context, each cell (and its anonymous wrappers) gets a line box. This means that the inline-block baseline is the anonymous inline table's baseline. Now an inline-table baseline is defined as the first row baseline [1]. The baseline of the row is the baseline of the only cell present as we have a very simple case [2]. Finally the cell's baseline is "baseline of the first in-flow line box in the cell, or the first in-flow table-row in the cell, whichever comes first." [2] [1] http://www.w3.org/TR/CSS2/visudet.html#line-height (see the vertical-align text) [2] http://www.w3.org/TR/CSS2/tables.html#height-layout

We only calculate the baseline for a cell if it's one of b(In reply to comment #11) > Now an inline-table baseline is defined as the first row baseline [1]. The baseline of the row is the baseline of the only cell present as we have a very simple case [2]. Finally the cell's baseline is "baseline of the first in-flow line box in the cell, or the first in-flow table-row in the cell, whichever comes first." [2] "If a row has no cell box aligned to its baseline, the baseline of that row is the bottom content edge of the lowest cell in the row." [1] In this case, the cell is vertical-align middle so the above statement seems to apply - which would mean this is not a bug. [1] http://www.w3.org/TR/CSS21/tables.html#height-layout

(In reply to comment #12) > We only calculate the baseline for a cell if it's one of b(In reply to comment #11) > > Now an inline-table baseline is defined as the first row baseline [1]. The baseline of the row is the baseline of the only cell present as we have a very simple case [2]. Finally the cell's baseline is "baseline of the first in-flow line box in the cell, or the first in-flow table-row in the cell, whichever comes first." [2] > > "If a row has no cell box aligned to its baseline, the baseline of that row is the bottom content edge of the lowest cell in the row." [1] > > In this case, the cell is vertical-align middle so the above statement seems to apply - which would mean this is not a bug. Your argument makes sense indeed - all the more since changing vertical-align to anything that resolve to 'baseline' removes the issue. I will still send an email to www-style to see if we didn't miss anything.

http://lists.w3.org/Archives/Public/www-style/2012Jul/0542.html Looks like we were wrong about the 'inline-table' wrapper, it should be a 'table' wrapper for which the baseline doesn't seem to be defined in CSS 2.1.

(In reply to comment #14) > http://lists.w3.org/Archives/Public/www-style/2012Jul/0542.html > > Looks like we were wrong about the 'inline-table' wrapper, it should be a 'table' wrapper for which the baseline doesn't seem to be defined in CSS 2.1. So I had a quick look and this works without regressions: --- a/Source/WebCore/rendering/RenderTableSection.cpp +++ b/Source/WebCore/rendering/RenderTableSection.cpp @@ -948,6 +948,9 @@ LayoutUnit RenderTableSection::firstLineBoxBaseline() const if (!m_grid.size()) return -1; + if (table()->parent()->isInlineBlockOrInlineTable()) + return -1; + Do we need to worry about anything beyond whether the table is wrapped by an inline-block?

(In reply to comment #15) > (In reply to comment #14) > > http://lists.w3.org/Archives/Public/www-style/2012Jul/0542.html > > > > Looks like we were wrong about the 'inline-table' wrapper, it should be a 'table' wrapper for which the baseline doesn't seem to be defined in CSS 2.1. > > So I had a quick look and this works without regressions: > > --- a/Source/WebCore/rendering/RenderTableSection.cpp > +++ b/Source/WebCore/rendering/RenderTableSection.cpp > @@ -948,6 +948,9 @@ LayoutUnit RenderTableSection::firstLineBoxBaseline() const > if (!m_grid.size()) > return -1; > > + if (table()->parent()->isInlineBlockOrInlineTable()) > + return -1; > + > > Do we need to worry about anything beyond whether the table is wrapped by an inline-block? BTW, this is apropos of http://lists.w3.org/Archives/Public/www-style/2012Jul/0721.html

(In reply to comment #15) > (In reply to comment #14) > > http://lists.w3.org/Archives/Public/www-style/2012Jul/0542.html > > > > Looks like we were wrong about the 'inline-table' wrapper, it should be a 'table' wrapper for which the baseline doesn't seem to be defined in CSS 2.1. > > So I had a quick look and this works without regressions: > > --- a/Source/WebCore/rendering/RenderTableSection.cpp > +++ b/Source/WebCore/rendering/RenderTableSection.cpp > @@ -948,6 +948,9 @@ LayoutUnit RenderTableSection::firstLineBoxBaseline() const > if (!m_grid.size()) > return -1; > > + if (table()->parent()->isInlineBlockOrInlineTable()) > + return -1; > + > > Do we need to worry about anything beyond whether the table is wrapped by an inline-block? I don't think this is correct: you shouldn't be poking at the table's parent. On top of that, the issue is not with firstLineBoxBaseline but lastLineBoxBaseline that is used for inline-block alignment. The WIP fix I attached matches the spec from that perspective so I am going to revive it.

Created attachment 156508 [details] WIP fix 2. Fix the code to do the right thing per the spec.

Created attachment 156740 [details] Proposed fix 1: Fix the baseline table logic to match the discussion on www-style.

Comment on attachment 156740 [details] Proposed fix 1: Fix the baseline table logic to match the discussion on www-style. 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.

Thanks Tony! > 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? Sure, I will land it tomorrow. > > 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. Yes, baselines are pretty fragmented in the spec with 'inline-table' baseline requiring you to move around to understand how it's actually defined. Flexbox needed to define the baseline of a 'table' which is why it is there and not in CSS 2.1. > > 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). Indeed. > > 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. I see what you mean. Here the table content is reversed between the test and the expected to catch how we compute the baseline. It should be possible to remove the table from the expected and I will do that before landing.

Created attachment 157566 [details] Patch for landing

Comment on attachment 157566 [details] Patch for landing Clearing flags on attachment: 157566 Committed r125229: <http://trac.webkit.org/changeset/125229>

All reviewed patches have been landed. Closing bug.