10207 - vertical-align: baseline not implemented for table cells

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

vertical-align: baseline is not yet working on tables.  Test case to be
attached.  Also see bug 2886 for other examples, except you can't see the
difference there...

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that you should be sure the testcase works with the window very narrow so
the cells carry to multiple lines.  Probably, more complicated testcases are
needed.

Also, the rules governing this weren't formally given until CSS2, even though
the properties existed in CSS1.  So this isn't technically a CSS1 bug.

Comment 3

[Nicholas Cull]

Marking [TESTCASE].

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Many further tests for this are in
http://www.fas.harvard.edu/~dbaron/css/test/sec170503 (which crashes in builds
before (and possibly including) the August 10 build).

This page also tests that the values 'sub', 'super', 'text-top', 'text-bottom'
(see CSS2 17.5.3) and length and percentage values (see
http://lists.w3.org/Archives/Public/www-style/1999Jul/0083.html ) should be
treated as baseline, not top as they are currently treated.  Once 'baseline' is
implemented, this is trivial to fix.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 2886 (for which the fix is attached) also causes problems on the above test
page.

Comment 6

[karnaze (gone)]

mass move to m14.

Comment 7

[rbs]

This bug is currently a severe hindrance to the progress of MathML. That's why
I have increased the severity to critical. By default, all MathML alignments 
involve the baseline. Expressions are difficult do decipher if their alignments 
are wrong. Moreover, we cannot properly layout matrices, arrays, commutative 
diagrams and such.

Chris, I don't know when you were planning to address this bug. If it is not 
in your "urgent" list, I would really like to start helping to get it fixed.
If you provide me with some directions as to where to look at, I could try
to investigative further. Please help.

What is needed:
- each cell should cache its ascent (presently, they don't cache their ascent)
- each row should cache the max-ascent amongst all cells which have 
vertical-align: baseline (presently each row is only caching the max height
of cells).

From my reading of the code, it is not trivial to get the ascent out of the 
innards of layout. Acccording to CSS, the "ascent" of the cell should be the 
ascent of the first linebox on that cell. But this information seems to be
lost in the entrails of layout.

PS: I will probably submit other issues to your consideration later on. But
for now, this is the most urgent thing that the MathML effort needs help for. 

Comment 8

[rbs]

The stumbling block is that the nsTableCellFrame code has no way to know the 
ascent of its content. Layout doesn't seem to care about this information. If it
is possible to export that information when the table row code is reflowing its
children, then the cell can cache its ascent and the table row code can later
pass the max of the ascents when it is doing VerticallyAlignChild().

Comment 9

[karnaze (gone)]

rbs, I've asked the folks who are working on the block code how to get the 
baseline of the table cell's block. If it isn't being set, I'm hoping it won't 
be difficult for the block code to set it, since lines understand how to align 
text on baselines and the block's baseline should just be the baseline of its 
first line.

Comment 10

[rbs]

Apparently, the block code is not yet setting the ascent. Printing aDesiredSize 
in the table cell code gives invariably zero for the descent. But indeed, if the 
block code properly set the ascent, then getting the vertical-align on the 
baseline should be fine.

Comment 11

[karnaze (gone)]

Troy said that ascent of the block should be the baseline. In the following 
html, the ascent of the cell's block during reflow is always equal to its 
desired height. Reassigning to Kipp and CCing the block busters (pun intended, 
Steve).

<table border width=100>
 <tr>
  <td>some wrapped text some wrapped text some wrapped text some</td>
 </tr>
</table> 

Comment 12

[ekrock's old account (dead)]

Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!

Comment 13

[rbs]

Yes, Chris. Agreed. The ascent is (presently) always set to the height (and 
that's why the descent is zero). 

Comment 14

[rbs]

Block folks, any comments/good news? Is this a demaning job for you?

Comment 15

[troy]

Fixing this will require that the block frame cache the first line's "ascent" 
information, because when doing an incremental reflow we'll need to recover that 
information.

It's not a large job, but it's a days worth of work, and I know Steve has more 
urgent things to deal with like the compatibility problem with <FONT size=1> 
problem. Nisheeth is working on finishing up reflow command coalescing

Comment 16

[Hixie (not reading bugmail)]

As MathML QA, I'm switching from critical to blocker, since it is blocking 
MathML work. As CSS QA, I'm flagging this as CSS2 compliance. 

I'm also marking this 4xp as IE5 does this correctly (!).

Comment 17

[buster]

adding beta1 keyword to potential beta blockers

Comment 18

[troy]

Removing beta1 keyword. Come on guys, this is not a beta1 blocker. We have 
hundreds of bugs, real compatibility issues to resolve, and enough performance 
issues to keep us all busy for many months

Comment 19

[buster]

here's the deal:  as I grovel through the block/inline reflow code, I'll keep 
this in mind. If it just falls out, I'll do it.  But I can't commit to it until 
M16.  I haven't been working with the code long enough to know just how big this 
job might become.  Sorry to be vague, but it's all I can offer today.  If 
someone else is interested in working on this, I'd be happy to guide them as 
best I can.

Comment 20

[Hixie (not reading bugmail)]

Making summary clearer.

Comment 21

[buster]

mine! mine mine mine!  all mine!  whoo-hoo!

Comment 22

[John Morrison]

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

Comment 23

[buster]

sorry, there's no way I can get this in for M16.  Hoping for M16

Comment 24

[rbs]

OK Steve, I can see that there is a lot of pressure in the air...

I have now managed to get the ascent of the first line, and it gets
propagated quite well out of the block reflow operation up into the table-cell.
It has turned that it is no necessary to worry about incremental reflows since
the first line is either clean (in which case the ascent doesn't change),
or it is dirty (in which case it is reflowed anyway so that the ascent
is recomputed by the reflow code).

I am attaching the patch. There is one thing that is still unclear
to me regarding what the code is doing when there is these line-height and
min/max values. Perhaps, the CSS experts (David and Ian) would have more 
insights as to what to do?

The other part in the table-cell is a bit circonvoluted since the
baseline can change the height of the cell... I am not yet done with that
table-cell part yet.

Comment 25

[rbs]

Attachment: Diff to export the baseline out of the block frame code

Comment 26

[rbs]

Attachment: Tescase examples of 'vertical-align:baseline' in table cells

Comment 27

[rbs]

Just attached a screenshot of what I got so far. Recovering state
for incremental reflow and the fact that a rowgroup can fire ReflowCellFrame() 
are not yet tuned up.

[The ascent should stay the same, no matter what happens when applying the 
line-height and min-max values to the block frame, right? In this case,
eveything is fine with the block frame code then.]

Comment 28

[rbs]

Attachment: Diff to support 'vertical-align: baseline' in the table code

Comment 29

[rbs]

While testing the testcase attached on bug 32436, I have observed that the
diff that I attached for the block code doesn't take care of special cases
where the code path executed by the block code does not involve
VerticalAlignFrames(). This happens, for example, when the first child
of <td> is a block, as in <td><div>...</div></td>.

I have made a further change to test the case where mLines->IsBlock(),
and fetch the ascent of mLines->mFisrtChild. Unfortunately, this still
doesn't fix the problem. Any clues as to remaining missing link? I will
be attaching a screenshot to demonstrate the problem.

Comment 30

[rbs]

Attachment: Diff with change to fetch the ascent of mLines->mFirstChild

Comment 31

[rbs]

Attachment: Testcase with rowspans blocks inside <td>

Comment 32

[rbs]

Attachment: Screenshot: rendering of the testcase

Comment 33

[rbs]

Adding chris to the Cc list so that he may have a look at the table code.
(I used cvs diff -u10 to produce a wider context and have kept everything
in #ifdef MOZ_MATHML for now to ease development/identification).

Comment 34

[rbs]

Checked-in

Comment 35

[rbs]

Note: Although the code was reviewed by buster and karnaze, it is still in 
#ifdef MOZ_MATHML and so 'vertical-align: baseline' is still not available for
normal HTML documents, pending QA after M16.

Comment 36

[buster]

I suggest we check this in as soon as beta2 ships

Comment 37

[rbs]

Pursuing further the issue I mentioned on 2000-05-30 06:26, I have observed
that the problem is due to the fact that the dectection of the case where the
first child of <td> is a block was insufficient.

If on the test case, I remove all the whitespace/blank-lines before <div>,
i.e, if 

<td> 
  <div> ... </div>
</td>

becomes

<td><div>
       ... </div>
</td>

then the code works as expected. I will try to attach a screenshot if I can.
(On the screenshot, all the <td><div>...</div></td> are represented
by red rectangles inside green rectangles. Of note is the fact the
baselines from inside the red <div>s are aligned properly.)

Therefore, it is not enough to just check

+    if (mLines && mLines->mFirstChild && mLines->IsBlock()) {

because the real first line may be something else.

Comment 38

[rbs]

Attachment: Screenshot after removing the whitespace between <td> and <div>

Comment 39

[rbs]

Index: nsBlockFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v
retrieving revision 3.389
diff -u -r3.389 nsBlockFrame.cpp
--- nsBlockFrame.cpp	2000/06/14 23:15:58	3.389
+++ nsBlockFrame.cpp	2000/06/22 06:20:50
@@ -2330,11 +2330,20 @@
     aMetrics.ascent = aMetrics.height;
     aMetrics.descent = 0;
 #else
-    if (mLines && mLines->mFirstChild && mLines->IsBlock()) {
-      // mAscent is not yet set because we didn't call VerticalAlignFrames()
-      // on mLines. So we need to fetch the ascent of the first child of mLines
+    // Need to check for special cases where mAscent is not yet properly set.
+    // There are two cases to consider: when the first line is a block, or
+    // when the first line is empty and is followed by a second line that
+    // is a block (e.g., <td>\n<div>). In these two cases, we need to fetch
+    // the ascent of the first child of the line.
+    nsLineBox* line = mLines;
+    // see if this first line is empty. if so, move on to the second line
+    if (line && (0 == line->GetHeight())) {
+      line = line->mNext;
+    }
+    // see if the line contains a block. if so, fetch the ascent of the block
+    if (line && line->mFirstChild && line->IsBlock()) {
       nsBlockFrame* bf;
-      nsresult res = mLines->mFirstChild->QueryInterface(kBlockFrameCID, 
(void**)&bf);
+      nsresult res = line->mFirstChild->QueryInterface(kBlockFrameCID, 
(void**)&bf);
       if (NS_SUCCEEDED(res) && bf) {
         mAscent = bf->GetAscent();
       }

Change to account for the special cases.
Need to fill r=? before heading to waterson!

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do you want to initialize mAscent to 0 in case there are no lines?  Or is it 
somehow set correctly already (in PlaceLine?)?  Also, do you want to change the 
way mAscent is set in nsBlockFrame::PlaceLine to correspond to these changes?  
(Also, I assume the comment you removed about VerticalAlignFrames() not having 
been called was wrong, since you have to call VerticalAlignFrames before you 
know what the ascent is.)

Comment 41

[rbs]

> Do you want to initialize mAscent to 0 in case there are no lines?  Or is it 
> somehow set correctly already (in PlaceLine?)?
buster, do you want to chime in here? None of the other data members of the
block frame (like mLines itself) are initialized. Is there a zeroing new
somewhere in the hierarchy? Or should I put 'mAscent = 0' in the constructor
to account for this special case that dbaron mentions?

>  Also, do you want to change the 
> way mAscent is set in nsBlockFrame::PlaceLine to correspond to these changes? 
> (Also, I assume the comment you removed about VerticalAlignFrames() not having 
> been called was wrong, since you have to call VerticalAlignFrames before you 
> know what the ascent is.)

There are some code-paths that don't lead to VerticalAlignFrames(), so the
comment wasn't wrong. Perhaps I should put that comment back...
nsBlockFrame::PlaceLine() is fine. If we ever get there, that will mean
that mLines is not a block, so mAscent will be set properly.
If we don't get there (or if the first line was empty), then the
final adjustments that I added will take care of setting mAscent properly.

Comment 42

[rbs]

> (... since you have to call VerticalAlignFrames before you 
> know what the ascent is.)
Yep, that's right. But in the case of a containing block, mAscent just
needs to 'bubble up'. (In this case, VerticalAlignFrames() would have called 
in the inner-most block frame for which mLines wasn't a containing block).

Comment 43

[rbs]

BTW, after enabling this patch, it will remain one important piece of
the puzzle before this bug is nailed down for HTML/CSS tables. The remaing
case is that of <td> <table> ... </table> </td>. This other special case (but
frequent case) won't work because the table code is not yet honoring the ascent.
Hence the table code still needs some further tuning so that it returns its
ascent (i.e., the ascent of its first row).

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

rbs, should these changes get turned on now?  (Since it looks like, from
previous comments, you have an r= but not an a=, adding approval keyword.)

Comment 45

[rbs]

Now got a=waterson. Will check-in once I have updated my tree to the tip with
the latest flood of changes that have followed the branch.

Comment 46

[rbs]

Checked-in. Code to support valign=baseline is now active in default builds.

Comment 47

[rbs]

dbaron, your tests with invalid length and percentage values will not work as
you documented because the Style System layer is filtering invalid length and
percentage values and mapping the corresponding valign to a default or whatever
is inherited from the parent (i.e., middle in most case).

The only invalid values that reach the table code are 'sub', 'super',
'text-top', 'text-bottom', which the code will remap to baseline as per 
your comments of 1999-08-10 14:12.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

rbs:  Are you sure it's not coming through from the style system?  Isn't it just
a bug in nsTableCellFrame::VerticallyAlignChild - the "default:" in the case
statement should be the same as BASELINE instead of the same as MIDDLE, right?

Comment 49

[rbs]

Yep. When I was unit-testing the code, my printfs couldn't catch length and 
percentage values and I had a quick look at the Style System (though I don't 
remember the section). In fact, the "default:" in the case is not effective 
because the verticalAlignFlags used in the switch is defaulted to BASELINE
earlier, for any value other than top, middle, bottom. (the same goes for the
HasVerticalAlignBaseline() function that are queried by parent frames).

Comment 50

[buster]

What's the state of this bug?  According to:
Additional Comments From [e-mail] 2000-07-28 02:56
sounds like a fix was checked in.
But it's still assigned to me and marked "NEW", with no nsbeta specification.
Need an update, please.  And thanks for carrying the torch on this while I was 
out!

Comment 51

[rbs]

Yes the fix is in, and is gaining some maturity. So the bug can be closed.
But for things to work right, all frames should honor the ascent information.
Currently, <table> is the remaining HTML frame that is not honoring that 
information, and 
   <td valign="baseline"> <table>...</table> </td>
doesn't mean anything.

So the table code should return the ascent of its first row. But this is a 
different problem and should probably be filed as a separate bug. dbaron
also asked for clarifications about the mapping of erroneous valign values.
This is a separate issue (from the Style System) and the fix is not going to be
affected.

Comment 52

[Loco]

Verified fixed in MacOS 9.0.4 build 2000-09-04-08, Linux  2000-09-06-08, and Win 
98 2000-09-06-08.

Comment 53

[Loco]

Actually folks, I take that back.  baseline works in most places but not in <col> 
or <colgroup>.

Testcase URLs:
http://slip/projects/marvin/html/col_valign_baseline.html
http://slip/projects/marvin/html/colgroup_valign_baseline.html

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Ummm...  why *should* it work for col or colgroup?  CSS properties inherit
through the document tree.

Comment 55

[Christine Hoffman]

This bug should be marked fixed. Valign is working but, as stated, not for COL 
and COLRGROUP (and HTML attribute for these elements). However, that problem is 
written up 12 bug #915.

Comment 56

[Hixie (not reading bugmail)]

I actually ran into the problem with baseline info not propagating out of nested
tables in a real world page yesterday. :-/

Is there a bug on that issue? Chris?

Comment 57

[buster]

fixed, per chrid d's comment.  thanks, chris.
this bug is a request for support, and we support it now.  if there are bugs
with specific aspects of that support, please open a new bug with a focused test
case.

Comment 58

[Christine Hoffman]

Verified fixed.

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40300ddedad7
[components] Close https://github.com/mozilla-mobile/android-components/issues/10207: Add support for private requests in Glean