367332 - redesign ascent computation to allow for multiple baselines for inline-block vs. inline-table

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Yesterday, I posted the following on dev.tech.layout:

=====
Date: Tue, 16 Jan 2007 14:54:59 -0800
Subject: changing baseline/ascent computation
From: "L. David Baron" <[e-mail]>
To: [e-mail]
Message-ID: <[e-mail]>

Implementing inline-block and inline-table correctly requires that
we be able to get multiple baselines for the same piece of content,
since inline-block uses a last-line-baseline of blocks and
inline-table (like tables) uses a first-line-baseline of blocks.

Doing this given our current mAscent mechanism seems difficult,
since currently reflow returns a single mAscent in the reflow
metrics.  The single mAscent also doesn't work very well for a bunch
of the interesting cases defined in the spec, such as getting the
baseline of the second block if the first block is empty.

So I plan to rewrite the way we compute baselines, at least for
things other than text and inlines.  Roughly (I haven't looked too
closely yet), my plans are to:

 1. Either remove mAscent from nsHTMLReflowMetrics (which, IIRC,
    might simplify some of the form control reflow methods) or stop
    requiring that it be set for anything other than text and inline
    frames (the frames for which we set mComputedWidth to
    NS_UNCONSTRAINEDSIZE).

 2. add virtual methods to nsIFrame to get the various pieces of
    information that are needed.

I plan to look into this in some more detail in the relatively near
future.
====

I've chosen the second option within (1), making the default for ascent to be a special value that says to call nsIFrame::GetBaseline.

At the same time, I also chose to get rid of descent, since nsHTMLReflowMetrics requires the invariant ascent + descent == height, so it seems pointless to have duplicate information, and we basically don't use it.  Doing that was actually a bit of work in MathML, but hopefully I did it right.

I'll be attaching a first patch shortly for examination, but I haven't done any testing of this patch yet.   All I know is that it compiles; I may well have forgotten a major piece.

Comment 1

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

Attachment: patch (untested)

Comment 2

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

(And yes, I plan to add reftests for the various pieces of spec I looked at when writing this...)

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the idea is that doing a separate GetBaseline for text and inlines would be more expensive?  That is, why the two different APIs?

I assume implementing this basically makes bug 64763 wontfix or unneeded or whatever?

I'm not sure I follow the case when there's no first-line baseline in nsTableCellFrame::GetCellBaseline.  Is there a reason the return value of GetFirstLineBaseline doesn't need to have inner->GetPosition().y added to it?  And why do we subtract GetPosition().y from inner->GetContentRect().YMost()?  That's changing from what we used to return, isn't it?

Given scrollable rowgroups, the ascent of an nsTableFrame may be bigger than its height.  This used to be the case anyway, but is it desirable?

Also, nsTableFrame::GetBaseline is pretty slow if there are lots of tbodies, and the block version can be slow if there are lots of empty lines for some reason.  Is it worth documenting somewhere that GetBaseline() is kinda slow in some cases?

Comment 4

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

(In reply to comment #3)
> So the idea is that doing a separate GetBaseline for text and inlines would be
> more expensive?  That is, why the two different APIs?

more expensive, and more code.  (Probably more the latter.)  Although maybe it's not that bad and I should just do it...

> I assume implementing this basically makes bug 64763 wontfix or unneeded or
> whatever?

I think so.  Baselines are really only used in limited cases -- inline layout and table rows.  Though when doing inline layout without a separate VerticalAlignFrames pass at the end (as MathML does) it is somewhat useful to be carrying ascent and descent around -- but it's a pain for everyone else.

> I'm not sure I follow the case when there's no first-line baseline in
> nsTableCellFrame::GetCellBaseline.  Is there a reason the return value of
> GetFirstLineBaseline doesn't need to have inner->GetPosition().y added to it? 

GetCellBaseline is the baseline of the contents of the cell, i.e., the inner frame.  I should probably comment that.  (Note that the SetDesiredSize call is in Reflow, not VerticallyAlignChild.)  I think that's basically the way it was before although I haven't actually traced through it.

> And why do we subtract GetPosition().y from inner->GetContentRect().YMost()? 
> That's changing from what we used to return, isn't it?

Because http://www.w3.org/TR/CSS21/tables.html#height-layout says:

# The baseline of a cell is the baseline of the first in-flow line box in the
# cell, or the first in-flow table-row in the cell, whichever comes first. If
# there is no such line box or table-row, the baseline is the bottom of content
# edge of the cell box.

> Given scrollable rowgroups, the ascent of an nsTableFrame may be bigger than
> its height.  This used to be the case anyway, but is it desirable?

We should probably report the height if the first row is in a scrollable rowgroup.

> Also, nsTableFrame::GetBaseline is pretty slow if there are lots of tbodies,
> and the block version can be slow if there are lots of empty lines for some
> reason.  Is it worth documenting somewhere that GetBaseline() is kinda slow in
> some cases?

I suppose.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> GetCellBaseline is the baseline of the contents of the cell

Measured from the top border-edge of the cell?  Of from the top border-edge of the contents?

Comment 6

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

Well, the contents don't actually have a border edge, but sure.  It's measured from the top of the thing that gets aligned when you do cell vertical alignment, which for us happens to be represented by a block frame with no margin or border.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Then the code makes sense, sure.  Might be good to document this in nsTableCellFrame.h.

But then in nsTableRowFrame::CalcHeight we should be adding the cellpadding (or rather the y-position of the block inside the cell, which is nonzero because of cellpadding) to it, no?  And same in nsTableRowGroupFrame::CalculateRowHeight, possibly, etc...

Comment 8

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

Attachment: patch

This is now somewhat tested, but still failing some of the included regression tests and some of the regression tests in the patch just attached to bug 9458.

Comment 9

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

And actually, Boris, you were right about GetCellBaseline.

Comment 10

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

Attachment: patch

This passes the tests enclosed, and also the additional tests in bug 9458 (although some of the tests there are buggy, still working on that, and will attach a new patch once I finish the tests there).

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (fType == nsGkAtoms::tableOuterFrame) {
+      nsTableFrame *table = NS_STATIC_CAST(nsTableFrame*,
+        aFrame->GetFirstChild(nsnull));
+      *aResult = table->GetBaseline() + table->GetPosition().y;
+      return PR_TRUE;
+    }

Can't you just return aFrame->GetBaseline() here?

+      // XXX Is this the right test?  We have some bogus empty lines
+      // floating around, but IsEmpty is perhaps too weak.
+      if (line->GetHeight() != 0 || !line->IsEmpty()) {

What are these "bogus empty lines"? I would expect just !line->IsEmpty() to work.

In GetCellBaseline:
+  return inner->GetContentRect().YMost() - inner->GetPosition().y +
+         borderPadding;
Why do you need to do this, can't you just use inner->GetContentRect().YMost()?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We still have nsIFrame::GetAscent, which seems to be equivalent to GetBaseline except for that annoying nsBoxLayoutState parameter. I guess we should leave that for future box/block work.

Comment 13

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

(In reply to comment #11)
> Can't you just return aFrame->GetBaseline() here?

Right.

> +      // XXX Is this the right test?  We have some bogus empty lines
> +      // floating around, but IsEmpty is perhaps too weak.
> +      if (line->GetHeight() != 0 || !line->IsEmpty()) {
> 
> What are these "bogus empty lines"? I would expect just !line->IsEmpty() to
> work.

It looks like IsEmpty() can return true for lines that take up space.  (See also the XXX comment in nsInlineFrame::IsEmpty about not matching nsLineLayout.cpp's setting of ZeroEffectiveSpanBox.)  If a line takes up space, I think it has a baseline.

> In GetCellBaseline:
> +  return inner->GetContentRect().YMost() - inner->GetPosition().y +
> +         borderPadding;
> Why do you need to do this, can't you just use inner->GetContentRect().YMost()?

Because vertical alignment in table cells is done by moving the inner around, and I want nsTableCellFrame's GetCellBaseline method to be independent of the current vertical alignment (which is the main reason it isn't GetBaseline).  I suppose I should add a comment.

Comment 14

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

Attachment: patch

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd rather go with the simpler code, just call IsEmpty(), and say that those lines don't have baselines. Do you know what these lines are that take up space and return true for IsEmpty, or are you just suspicious? Seems to me that if those lines have baselines then we should change IsEmpty to return false for them.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 252396 [details] [diff] [review]
patch

On reflection, I guess we should just take this and disentangle line emptiness some other time.

Comment 17

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

Checked in to trunk.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Is there a followup on the situation with scrollable table row groups, btw?

Comment 19

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/reftests&command=DIFF_FRAMESET&file=reftest.list&rev1=1.24&rev2=1.25&root=/cvsroot