401178 - incorrect ascents for foreign children in nsMathMLContainerFrame::ReflowChild

Status: RESOLVED FIXED

(Core :: MathML, defect, P1)

Description

[Karl Tomlinson (:karlt)]

nsMathMLContainerFrame::ReflowChild initializes aDesiredSize.ascent to 0, but nsBlockFrame::Reflow does not set this value, so it stays 0.

Initializing instead to nsHTMLReflowMetrics::ASK_FOR_BASELINE may be appropriate but aChildFrame->GetBaseline() is called before DidReflow().

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: hack to touch things up

You could say this mostly fixes the problem, but there's a bit of luck in that.

Comment 2

[Karl Tomlinson (:karlt)]

Requesting blocking 1.9 because this bug makes non-stretchy MathML frames look like they each have a blank line inserted above them.

Comment 3

[Karl Tomlinson (:karlt)]

Bug 348577 blocks the complete fix of this, but I'm marking 348577 as depending on this as there is a dependency loop somewhere.  I guess its due to marking regressions as blockers.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: get ascent from nsLayoutUtils::GetLastLineBaseline()

Use nsHTMLReflowMetrics constructor to initialize instead of initializing
ourselves.  This ensures that aDesiredSize.ascent will be ASK_FOR_BASELINE when not set in Reflow().

Set aDesiredSize.ascent (as well as mBoundingMetrics.ascent) when
ASK_FOR_BASELINE.

Use nsLayoutUtils::GetLastLineBaseline() instead of nsIFrame::GetBaseline().
GetBaseline() is "Only valid when Reflow is not needed" according to
nsIFrame.h, and nsFrame::GetBaseline() interprets this as !NS_SUBTREE_DIRTY()
as it needs mRect.height.  We don't have a HaveSetHeight() API, so it seems
that GetBaseline() shouldn't be used until after
DidReflow(NS_FRAME_REFLOW_FINISHED).

nsBlockFrame::Reflow and nsTableRowFrame::ReflowCellFrame use
nsLayoutUtils::GetFirstLineBaseline() before DidReflow(), so it seems safe to
use GetLastLineBaseline() before DidReflow(), and this is suitable in the
cases we care about.

Only ComputeTightBounds when requested with NS_REFLOW_CALC_BOUNDING_METRICS.
(We should probably get rid of mBoundingMetrics and mFlags in
nsHTMLReflowMetrics, and just use GetBoundingMetrics().  One way to do this
would be to make nsMathMLmathBlockFrame a nsIMathMLFrame and override
GetBoundingMetrics() with a ComputeTightBounds() implementation.  We could
then also store the (frame-coordinate) ascent in the nsIMathMLFrame instead of
borrowing the frame's origin.)

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: turn off NS_REFLOW_CALC_BOUNDING_METRICS assertion in nsTextFrameThebes.cpp

This will need to go with the above patch, as NS_REFLOW_CALC_BOUNDING_METRICS will now be set (sometimes).

NS_REFLOW_CALC_BOUNDING_METRICS is only used with nsMathMLContainerFrame::ReflowChild() so there is no need for the assert.

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to comment #5)
> This will need to go with the above patch, as NS_REFLOW_CALC_BOUNDING_METRICS
> will now be set (sometimes).

Correction: this flag seems to be lost between the nsBlockFrame and the nsTextFrame so we don't need to remove this.  Should we leave it in to detect if an nsTextFrame is Reflow()ed directly from a MathML frame?

Comment 7

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

Yes, let's leave that in.

We should be getting rid of NS_REFLOW_CALC_BOUNDING_METRICS at some point. ComputeTightBounds is a better API and we should use it in MathML frames too.

Comment 8

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

Comment on attachment 290967 [details] [diff] [review]
get ascent from nsLayoutUtils::GetLastLineBaseline()

+        nsHTMLReflowMetrics childDesiredSize;

Shadowing local variables is rather evil (very easy to get confused while reading code). Please give this inner variable a new name.

+        nsHTMLReflowMetrics childDesiredSize;

Ditto.

Comment 9

[Karl Tomlinson (:karlt)]

childDesiredSize is not shadowing but used in separate scopes (loops), and in the second it scope tries to reproduce the value from the first, so using the same name is appropriate.  Checked-in as is after discussion with roc:

1.171       layout/mathml/base/src/nsMathMLContainerFrame.cpp
1.62        layout/mathml/base/src/nsMathMLTokenFrame.cpp
1.71        layout/mathml/base/src/nsMathMLmfencedFrame.cpp
1.56        layout/mathml/base/src/nsMathMLmrootFrame.cpp

The ascents are still not right in tables (bug 348577) but that is not so noticeable yet because tables usually appear empty (bug 363240).