161155 - Integrals appear clipped sometimes and complete frames are sometimes not repainted

Status: RESOLVED FIXED

(Core :: MathML, defect, P2)

Description

[Henri Sivonen (:hsivonen)]

Build ID: 2002080503

Reproducible: almost always

Steps to reproduce:
1) Load http://www.mozilla.org/projects/mathml/demo/texvsmml.xml or
http://www.mozilla.org/projects/mathml/demo/texvsmml.xml

Expected results:
Expected integral glyphs to be fully displayed.

Actual results:
Sometimes integral glyphs appear only partially. Scrolling or selecting text
usually fixes the rendering.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Screenshot showing clipped integrals

Comment 2

[kitchin]

The URL's for the test cases are now
http://www.mozilla.org/projects/mathml/demo/texvsmml.xhtml
http://www.mozilla.org/projects/mathml/demo/

Comment 3

[Karl Tomlinson (:karlt)]

This is now much more significant on trunk (bug 335405 comment 0 has more info on why) and should be fixed for 1.9.

Text disappears on selection, accents are sometimes not visible, and non-display fractions sometimes lose parts of their denominators.

We need to ensure the overflow area is set correctly, including after a Stretch().

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: provide correct GetBounds() for nsDisplayMathMLCharForeground

nsDisplayItem just used the underlying frame's rect, which didn't include its overflow area.  The bounding metrics of the nsMathMLChar seem more appropriate than those of the underlying frame, because it is the nsMathMLChar that is drawn.

This fixes issues I'm seeing with parts of integrals not being repainted, and is part of the fix for missing accents, when the glyphs for diacritical marks have zero advance width.

Comment 5

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

Comment on attachment 293935 [details] [diff] [review]
provide correct GetBounds() for nsDisplayMathMLCharForeground

+      aBuilder->ToReferenceFrame(mFrame) + nsPoint(rect.x, rect.y);

use rect.TopLeft()

Seems to me that the frame(s) that contain nsMathMLChars should also be setting their overflow areas to be the union of the frame dimensions and the glyph bounds, right?

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to comment #5)
> (From update of attachment 293935 [details] [diff] [review])
> +      aBuilder->ToReferenceFrame(mFrame) + nsPoint(rect.x, rect.y);
> 
> use rect.TopLeft()

I thought a method like that would be useful :)

> Seems to me that the frame(s) that contain nsMathMLChars should also be
> setting their overflow areas to be the union of the frame dimensions and
> the glyph bounds, right?

Right.  And not setting the overflow area is why many frames are sometimes not repainted.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: set overflow area for nsMathMLContainerFrames

This resolves all the other (static) not-painting issues I've seen, including those mentioned in comment 3, except for <mtable> and when <math> is within a <table>.

As roc mentions, we already have all the information in mBoundingMetrics and the frame dimensions.  It just needs transferring to the overflow area.

Comment 8

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

+  // A variant on nsIFrame::FinishAndStoreOverflow() that use the bounding
+  // metrics to calculate the overflow.
+  void FinishAndStoreOverflow(nsHTMLReflowMetrics* aMetrics);

Give it a new name. Otherwise you trigger warnings about hiding overloaded methods because of stupid C++ rules.

Where are the overflow areas of children added to the parent's overflow area?

Have you checked that FinishAndStoreOverflow isn't called twice? That would be bad. I didn't find any occurrence of that but I didn't check very hard.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: set overflow area for nsMathMLContainerFrames v2

(In reply to comment #8)
> +  void FinishAndStoreOverflow(nsHTMLReflowMetrics* aMetrics);
> 
> Give it a new name. Otherwise you trigger warnings about hiding overloaded
> methods because of stupid C++ rules.

Done.

> Where are the overflow areas of children added to the parent's overflow area?

Usually in the Place() method.  nsMathMLContainerFrames set up mBoundingMetrics to include child bounding metrics.

The one case where this wasn't quite done was where foreign frames were children of mathml frames, so now in this case the child overflow rect is used in calculation of the parent's mBoundingMetrics.

> 
> Have you checked that FinishAndStoreOverflow isn't called twice? That would be
> bad. I didn't find any occurrence of that but I didn't check very hard.
> 

There are just two places: in Reflow() for the <msqrt> and <mpadded> frames.
(<mroot> is fine.)
These methods call nsMathMLContainerFrame::Reflow() and then shift child frames around and modify the bounding metrics.

I don't think it would be that bad to call FinishAndStoreOverflow twice because the modified mOverflowArea is not reused, but I'm not comfortable with the child frames being moved after FinishReflowChild() is called on them (without invalidation or updating views), so I'll look at doing their Reflow() differently.  I've left <msqrt> and <mpadded> unfixed for now.

Other changes:

StoreOverflowFromBoundingMetrics uses a reference rather than a pointer to the nsHTMLReflowMetrics as this is usually what the caller has, and we no longer have any reason to maintain compatibility with nsIFrame::FinishAndStoreOverflow.

nsMathMLTokenFrame::Place now gives child frames the rect they ask for.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: set overflow area for nsMathMLContainerFrames v3

StoreOverflowFromBoundingMetrics() is now called after FixInterFrameSpacing(), as that can modify mBoundingMetrics.

(FixInterFrameSpacing actually can move child frames a little after FinishReflowchild, but I guess this doesn't happen too often, and movements should be small)

Comment 11

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

(In reply to comment #10)
> (FixInterFrameSpacing actually can move child frames a little after
> FinishReflowchild, but I guess this doesn't happen too often, and movements
> should be small)

Eeek, that doesn't sounds good. is there anything we can do to fix this part?

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to comment #11)

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.

I hope the same issue in Stetch() can be solved in the same way.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp&rev=1.172&mark=437-438#416

Is there a reason why there is no problem with moving after DidReflow() in nsLineLayout::VerticalAlignFrames()?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsLineLayout.cpp&rev=3.293&mark=966,1005-1008,2009#1004

There are about 13 different implementations of Place(), so some testing will be required in changing these.  This doesn't change the need for FinishAndStoreOverflow though and has little effect on how that is done (maybe it would look like attachment 294174 [details] [diff] [review] rather than attachment 294190 [details] [diff] [review].

I think the right thing to do is get the FinishAndStoreOverflow doje first to sort out the big issues.

The extra method for offsets for Place() may be useful in changing Reflow() for <mpadded> and <msqrt>, so those changes may be best done together.

Comment 13

[Karl Tomlinson (:karlt)]

(Attachment 219725 [details], in bug 323733 comment 10, shows the issues solved by
attachment 293935 [details] [diff] [review].)

Comment 14

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

(In reply to comment #12)
> Is there a reason why there is no problem with moving after DidReflow() in
> nsLineLayout::VerticalAlignFrames()?

Yes, lines later do a RelativePositionFrames pass that among other things recomputes the overflow area for the frames in the line.

> There are about 13 different implementations of Place(), so some testing will
> be required in changing these.  This doesn't change the need for
> FinishAndStoreOverflow though and has little effect on how that is done (maybe
> it would look like attachment 294174 [details] [diff] [review] rather than attachment 294190 [details] [diff] [review].
> 
> I think the right thing to do is get the FinishAndStoreOverflow doje first to
> sort out the big issues.

OK.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 294190 [details] [diff] [review]
set overflow area for nsMathMLContainerFrames v3

The overflow area of <mpadded> should not be calculated from mBoundingMetrics, which are modified to change positioning.  So I'll think a bit more about this.

Comment 16

[Karl Tomlinson (:karlt)]

For <mpadded>, mBoundingMetrics is set to the "bounding box" of element, as adjusted by attributes.

http://www.w3.org/TR/MathML2/chapter3.html#presm.mpadded

"If the "padding" is negative, it is possible for the content of mpadded to be rendered outside the mpadded element's bounding box"

It looks like what to do with content outside this so-called bounding box is up for debate in MathML3.  Although the most recent draft still has a statement similar to that from MathML2 above, it has tentatively added the following statement

http://www.w3.org/TR/2007/WD-MathML3-20070427/chapter3.html#id.3.3.6.3

"The bounding box of the mpadded element acts as a clipping rectangle for the rendering of the child content."

and added Issue mpadded-clipped:

"Should the bounding box act as a clipping rectangle, or only be used for positioning, background color and so on?"

with Resolution currently "None recorded".

I think the statement regarding rendering outside the bounding box strongly suggests that content should not be clipped, so I hope that the concept of clipping will be removed from the MathML3 draft, and for now we'll follow MathML2.

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: set overflow area for nsMathMLContainerFrames v4

This uses the union of child overflow areas and mBoundingMetrics (and frame bounds) to calculate the total overflow (before outline).  There are places where considering child overflows is not necessary and there are places where considering mBoundingMetrics is not necessary but it is easier and tidier and less code not to special-case these places.

StoreOverflowFromBoundingMetrics has changed name to GatherAndStoreOverflow and I've kept the pointer argument semantics of FinishAndStoreOverflow as this could have been an implementation of FinishAndStoreOverflow if it were not for the overloaded method.  This method is now protected as there should be no need to use it from outside MathML.

<msqrt> and <mpadded> have been changed so that the overflow is stored (once only) after the frames are positioned.  I'll follow up with another patch to tidy up the FinishReflowChild/SetPosition issues in these frames.

Other changes:

* No longer using foreign child overflow rect in calculation of the parent's
  mBoundingMetrics, as this could have affected spacing, and is now not
  necessary with ConsiderChildOverflow().

* Small changes in nsMathMLContainerFrame::Stretch() and Reflow() to make
  variable scoping clearer.

Comment 18

[Karl Tomlinson (:karlt)]

Checked in these patches:

1.134       mozilla/layout/mathml/base/src/nsMathMLChar.cpp

1.173       mozilla/layout/mathml/base/src/nsMathMLContainerFrame.cpp
1.77        mozilla/layout/mathml/base/src/nsMathMLContainerFrame.h
1.63        mozilla/layout/mathml/base/src/nsMathMLTokenFrame.cpp
1.72        mozilla/layout/mathml/base/src/nsMathMLmfencedFrame.cpp
1.95        mozilla/layout/mathml/base/src/nsMathMLmoFrame.cpp
1.45        mozilla/layout/mathml/base/src/nsMathMLmpaddedFrame.cpp
1.16        mozilla/layout/mathml/base/src/nsMathMLmpaddedFrame.h
1.57        mozilla/layout/mathml/base/src/nsMathMLmrootFrame.cpp
1.48        mozilla/layout/mathml/base/src/nsMathMLmsqrtFrame.cpp
1.23        mozilla/layout/mathml/base/src/nsMathMLmsqrtFrame.h

Filed bug 410132 on the FinishReflowChild/SetPosition issues.