Closed Bug 10207 Opened 25 years ago Closed 24 years ago

vertical-align: baseline not implemented for table cells

Categories

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

Product:
Core
Component:
Layout: Tables
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: dbaron, Assigned: buster)

References

Details

(Keywords: css2, testcase, Whiteboard: [TESTCASE] blocking mathml)

Attachments

(8 files)

testcase.
771 bytes, text/html
Details
Diff to export the baseline out of the block frame code
5.35 KB, text/plain
Details
Tescase examples of 'vertical-align:baseline' in table cells
11.76 KB, image/gif
Details
Diff to support 'vertical-align: baseline' in the table code
38.04 KB, text/plain
Details
Diff with change to fetch the ascent of mLines->mFirstChild
11.99 KB, text/plain
Details
Testcase with rowspans blocks inside <td>
2.10 KB, text/html
Details
Screenshot: rendering of the testcase
23.91 KB, image/gif
Details
Screenshot after removing the whitespace between <td> and <div>
19.77 KB, image/gif
Details 
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...
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.
Status: NEW → ASSIGNED
Target Milestone: M12
Whiteboard: [TESTCASE]
Marking [TESTCASE].
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.
Bug 2886 (for which the fix is attached) also causes problems on the above test
page.
mass move to m14.
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.
Severity: normal → critical
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().
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.
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.
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>
Assignee: karnaze → kipp
Status: ASSIGNED → NEW
Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!
Keywords: testcase
Yes, Chris. Agreed. The ascent is (presently) always set to the height (and 
that's why the descent is zero). 
Block folks, any comments/good news? Is this a demaning job for you?
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
Keywords: beta1
Severity: critical → blocker
Keywords: 4xp, css2
Whiteboard: [TESTCASE] → [TESTCASE] blocking mathml
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 (!).

adding beta1 keyword to potential beta blockers
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
Keywords: beta1
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.
Priority: P3 → P2
Target Milestone: M14 → M16
Making summary clearer.
Summary: vertical-align: baseline not implemented → vertical-align: baseline not implemented for table cells
mine! mine mine mine!  all mine!  whoo-hoo!
Assignee: kipp → buster
*** Bug 32436 has been marked as a duplicate of this bug. ***
sorry, there's no way I can get this in for M16.  Hoping for M16
Target Milestone: M16 → M17
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.

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.]
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.
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).

Checked-in
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.

I suggest we check this in as soon as beta2 ships
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.
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!
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.)
> 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.
> (... 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).
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).

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.)
Keywords: approval
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.
Checked-in. Code to support valign=baseline is now active in default builds.
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.
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?
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).
What's the state of this bug?  According to:
Additional Comments From rbs@maths.uq.edu.au 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!
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.
Verified fixed in MacOS 9.0.4 build 2000-09-04-08, Linux  2000-09-06-08, and Win 
98 2000-09-06-08.

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

Ummm...  why *should* it work for col or colgroup?  CSS properties inherit
through the document tree.
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.
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?
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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.

Verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: