410132 - position MathML Frames before calling FinishReflowChild()

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Karl Tomlinson (:karlt)]

There are a few places in Reflow of MathML frames where child frames are moved after calling FinishReflowChild().

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: FinishReflowChild() with the final position for <msqrt> and <mpadded>

<msqrt> is the most common case where the frames are moved a large amount,
and the fix for <mpadded> is similar.

Comment 2

[Karl Tomlinson (:karlt)]

The next most common case is
 http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp&rev=1.173&mark=1442#1422

The distance moved could be significant for some <mo> operators that are direct children of a <math> element (with no enclosing <mrow>).

We should be able to shift the work that FixInterFrameSpacing does to the
Place() methods.  They could use another method to calculate the offset for the
child frames, probably only necessary when aPlaceOrigin is true.  There are about 13 different implementations of Place(), though.

I hope the same issue with spacing around embellished operators in nsMathMLContainerFrame::Stetch() can be solved in the same way.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp&rev=1.173&mark=402,436-437#402

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 294807 [details] [diff] [review]
FinishReflowChild() with the final position for <msqrt> and <mpadded>

I've noticed a spacing issue that I need to look into.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: FinishReflowChild() with the final position for <msqrt> and <mpadded>

Corrected != that should have been == nsGkAtoms::msqrt.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: FinishReflowChild() with the final position for <msqrt> and <mpadded> (with check for no child frame)

Comment 6

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

+    if (mThinSpace < 0) {
+      // cache away thinspace
+      mThinSpace = GetThinSpace(mParentFrame->GetStyleFont());
+    }

Why not just call GetThinSpace every time instead of caching in mThinSpace at all. The performance issues here are negiligible.

+  nscoord mX;

Put this with the other fields.

+  if (child.Frame()) {
+    ascent = child.Ascent();
+    descent = child.Descent();
+    mBoundingMetrics = child.BoundingMetrics();
+    mBoundingMetrics.leftBearing += child.mX;
+    mBoundingMetrics.rightBearing += child.mX;
+    ++child;
+  }

I'd prefer to avoid this by judiciously initializing things to nscoord_MAX etc.

Looks great otherwise.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: FinishReflowChild() with the final position for <msqrt> and <mpadded> and make nsBoundingMetrics::operator+= handle empty bounds [checked-in]

(In reply to comment #6)
> Why not just call GetThinSpace every time instead of caching in mThinSpace at
> all. The performance issues here are negiligible.

Dunno.  It used to be cached but I've happily removed that.

> +  nscoord mX;
> 
> Put this with the other fields.

Done (and it didn't really need to be public).

> 
> +  if (child.Frame()) {
> +    ascent = child.Ascent();
> +    descent = child.Descent();
> +    mBoundingMetrics = child.BoundingMetrics();
> +    mBoundingMetrics.leftBearing += child.mX;
> +    mBoundingMetrics.rightBearing += child.mX;
> +    ++child;
> +  }
> 
> I'd prefer to avoid this by judiciously initializing things to nscoord_MAX etc.

I've avoided this by modifying nsBoundingMetrics::operator+= .
I think the frame rect should include the baseline, so haven't done anything special with the ascent and descent for the reflow metrics.

Comment 8

[Karl Tomlinson (:karlt)]

+ leftBearing = width - bm.leftBearing;

That should be "width + bm.leftBearing".

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 295181 [details] [diff] [review]
FinishReflowChild() with the final position for <msqrt> and <mpadded> and make nsBoundingMetrics::operator+= handle empty bounds [checked-in]

1.81        gfx/public/nsIRenderingContext.h
1.301       layout/generic/nsContainerFrame.cpp
3.81        layout/generic/nsContainerFrame.h
1.175       layout/mathml/base/src/nsMathMLContainerFrame.cpp
1.78        layout/mathml/base/src/nsMathMLContainerFrame.h
1.65        layout/mathml/base/src/nsMathMLTokenFrame.cpp
1.74        layout/mathml/base/src/nsMathMLmfencedFrame.cpp
1.47        layout/mathml/base/src/nsMathMLmpaddedFrame.cpp
1.17        layout/mathml/base/src/nsMathMLmpaddedFrame.h
1.50        layout/mathml/base/src/nsMathMLmsqrtFrame.cpp

(Comment 2 is not yet addressed.)

Comment 10

[Karl Tomlinson (:karlt)]

Bug tidying.

Filed bug 433064 on comment 2.