Bug 91137 - REGRESSION(r117339): cell in block-level table in inline-block are aligned with their last line box
: REGRESSION(r117339): cell in block-level table in inline-block are aligned wi...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: All All
: P1 Normal
Assigned To: Julien Chaffraix
: HasReduction
Depends on: 15365
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-12 12:59 PDT by Julien Chaffraix
Modified: 2012-08-09 18:06 PDT (History)
8 users (show)

See Also:


Attachments
Reproduction - the 2 boxes should be aligned (435 bytes, text/html)
2012-07-12 12:59 PDT, Julien Chaffraix
no flags Details
WIP patch: solves the issue, some testing but not much. Comments welcome. (7.28 KB, patch)
2012-07-12 14:10 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
WIP fix 2. Fix the code to do the right thing per the spec. (12.20 KB, patch)
2012-08-03 19:46 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed fix 1: Fix the baseline table logic to match the discussion on www-style. (10.91 KB, patch)
2012-08-06 12:09 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (10.76 KB, patch)
2012-08-09 15:48 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-07-12 12:59:15 PDT
Bug 84167 changed RenderTable to properly account for its baseline. This made block-level table behaves like normal divs when inserted into inline-block and align according their last line box.

This doesn't match Firefox and IE 9 though and is a deviation from our old behavior.
Comment 1 Julien Chaffraix 2012-07-12 12:59:40 PDT
Created attachment 152033 [details]
Reproduction - the 2 boxes should be aligned
Comment 2 Julien Chaffraix 2012-07-12 13:01:10 PDT
*** Bug 91136 has been marked as a duplicate of this bug. ***
Comment 3 Robert Hogan 2012-07-12 13:07:13 PDT
WebKit matches Opera here btw. :)
Comment 4 Robert Hogan 2012-07-12 13:08:31 PDT
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
Comment 5 Julien Chaffraix 2012-07-12 14:08:45 PDT
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.
Comment 6 Julien Chaffraix 2012-07-12 14:10:20 PDT
Created attachment 152055 [details]
WIP patch: solves the issue, some testing but not much. Comments welcome.
Comment 7 Gérard Talbot 2012-07-12 16:13:54 PDT
> 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
Comment 8 Julien Chaffraix 2012-07-12 19:01:07 PDT
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/).
Comment 9 Gérard Talbot 2012-07-12 21:32:16 PDT
(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..
Comment 10 Julien Chaffraix 2012-07-13 10:46:53 PDT
> 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.
Comment 11 Julien Chaffraix 2012-07-23 09:27:26 PDT
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
Comment 12 Robert Hogan 2012-07-23 13:01:55 PDT
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
Comment 13 Julien Chaffraix 2012-07-23 16:11:09 PDT
(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.
Comment 14 Julien Chaffraix 2012-07-25 14:45:49 PDT
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.
Comment 15 Robert Hogan 2012-08-02 11:11:30 PDT
(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?
Comment 16 Robert Hogan 2012-08-02 11:12:14 PDT
(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
Comment 17 Julien Chaffraix 2012-08-03 09:55:35 PDT
(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.
Comment 18 Julien Chaffraix 2012-08-03 19:46:51 PDT
Created attachment 156508 [details]
WIP fix 2. Fix the code to do the right thing per the spec.
Comment 19 Julien Chaffraix 2012-08-06 12:09:42 PDT
Created attachment 156740 [details]
Proposed fix 1: Fix the baseline table logic to match the discussion on www-style.
Comment 20 Tony Chang 2012-08-07 17:33:03 PDT
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.
Comment 21 Julien Chaffraix 2012-08-08 18:36:32 PDT
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.
Comment 22 Julien Chaffraix 2012-08-09 15:48:40 PDT
Created attachment 157566 [details]
Patch for landing
Comment 23 WebKit Review Bot 2012-08-09 18:06:12 PDT
Comment on attachment 157566 [details]
Patch for landing

Clearing flags on attachment: 157566

Committed r125229: <http://trac.webkit.org/changeset/125229>
Comment 24 WebKit Review Bot 2012-08-09 18:06:17 PDT
All reviewed patches have been landed.  Closing bug.