385265 - "ASSERTION: Wrong line container hint" in BuildTextRuns (nsTextFrameThebes.cpp) with MathML table

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Wrong line container hint: '!aForFrame || aLineContainer == FindLineContainer(aForFrame)', file /Users/jruderman/trunk/mozilla/layout/generic/nsTextFrameThebes.cpp, line 1063

Comment 1

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

Attachment: fix

We should create a standalone nsLineLayout to reflow text frames with. As it stands (in this testcase, for example) the textframe can actually get a block's outer textframe and use it, which is not a good idea. So this patch makes all IsFrameOfType(eLineParticipant)s go through the nsLineLayout creation path.

This triggers more assertions due to passing in unconstrained widths to Reflow, which is no longer allowed post-reflow-branch. So I avoid those warnings by reflowing the frames with their preferred width.

Comment 2

[Jesse Ruderman]

Typo in patch: "aChildFraame".

Comment 3

[rbs]

> We should create a standalone nsLineLayout to reflow text frames with.

Does this then mean that the ability to directly return the bounding metrics from the text frame is lost? (which, as a consequence, means that LL should be modified to feed the returned metrics.mBoundingMetrics when the CALC_BOUNDING_METRICS is set? I don't think LL bothers setting the field now).

Comment 4

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

Yes, that's true.

Comment 5

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

Comment on attachment 269170 [details] [diff] [review]
fix

I'll revise the patch to do that, although it will be hard to test.

Comment 6

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

Attachment: better fix

Okay, hopefully this will do it. This patch allows nsLineLayout to capture tight bounding boxes and return the union of them to the caller. To get this to work properly MathML needs to call VerticalAlignLine, so I made that work when there is no mLineBox. I'm also now capturing the line's overflow area correctly, just because we can!

Comment 7

[rbs]

Comment on attachment 269300 [details] [diff] [review]
better fix

sr=rbs, looks reasonable to me now. Plus... fingers crossed for this to eventually enable solving the famous (or should I say infamous) bended ∫ clipping problem.

Comment 8

[rbs]

BTW, roc, you might want to add #ifdef MOZ_MATHML so as not break those builds (such as Minimo) that are still with --disable-mathml.

Comment 9

[Jesse Ruderman]

Bug 387126 is also about this assertion.

Comment 10

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

(In reply to comment #1)
> As it
> stands (in this testcase, for example) the textframe can actually get a block's
> outer textframe and use it, which is not a good idea.

roc: I can't figure out what you meant by this -- do you remember?

Comment 11

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

Comment on attachment 269300 [details] [diff] [review]
better fix

>-  nsSize availSize(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);
>+  // provide a local, self-contained linelayout where to reflow the frame
>+  nsSize availSize(aChildFrame->GetPrefWidth(aReflowState.rendContext), NS_UNCONSTRAINEDSIZE);

Why do you need to use the pref width?  Shouldn't the containing block width from the reflow state be sufficient?  nsLineLayout::Reflowframe uses mBlockReflowState->ComputedWidth().  I think you want aReflowState.mCBReflowState->ComputedWidth()?

Comment 12

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

Comment on attachment 269300 [details] [diff] [review]
better fix

>+nsRect
>+nsLineLayout::ComputeTightBounds(PerSpanData* psd)
>+{
>+  nsRect result(0,0,0,0);
>+  for (PerFrameData* pfd = psd->mFirstFrame; pfd; pfd = pfd->mNext) {
>+    if (pfd->GetFlag(PFD_ISTEXTFRAME)) {
>+      result.UnionRect(result, pfd->mTightBounds + pfd->mBounds.TopLeft());
>+    }
>+    if (pfd->mSpan) {
>+      result.UnionRect(result, ComputeTightBounds(pfd->mSpan) + pfd->mBounds.TopLeft());
>+    }

Don't you want to consider the bounds of non-text frames that aren't spans, like images?

>Index: layout/generic/nsLineLayout.h
>+   * LL_NEEDTIGHTBOUNDINGBOX must have been set during line reflow if you want
>+   * to call this.

The flag is internal; you should instead mention the boolean passed to BeginLineReflow.

Comment 13

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

Comment on attachment 269300 [details] [diff] [review]
better fix

>+    // only valid for text frames when LL_NEEDTIGHTBOUNDINGBOX. The rect is relative
>+    // to the baseline left point.
>+    nsRect mTightBounds;

This comment seems wrong; it looks like it's relative to the top left of the bounds.

Comment 14

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

Comment on attachment 269300 [details] [diff] [review]
better fix

>+   * This computes a tight bounding box for the frames on the line, containing
>+   * only inked areas of glyphs. Borders on inlines etc are currently not
>+   * included. The bounding box is relative to the left origin of the baseline.

You might want to mention images if you include them.

>+   * You can call this before or after calling RelativePositionFrames, but
>+   * you probably want to call it before... You must call VerticalAlignLine()
>+   * before calling this.

I'd say that RelativePositionFrames must not have been called yet.  (And without the "...".)

>+  // XXX Call ll.HorizontalAlignFrames() and/or ll.TrimTrailingWhiteSpace()?

Seems like you probably should call TrimTrailingWhiteSpace, although maybe that's a followup bug.

I think you shouldn't call HorizontalAlignFrames, although I don't follow all of the RTL stuff it does, so maybe you need to -- but you'd need to figure out the right amount of space to do the alignment in, which means maybe you would need that GetPrefWidth call after all.

r=dbaron with all the above comments (comment 2, comment 8?, comment 11, comment 12, comment 13, and this comment) addressed.  (Note also Jesse's spelling comment.)

Comment 15

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

(In reply to comment #10)
> (In reply to comment #1)
> > As it stands (in this testcase, for example) the textframe can actually get a
> > block's outer textframe and use it, which is not a good idea.
> 
> roc: I can't figure out what you meant by this -- do you remember?

I meant outer line-layout. In particular, given a situation like this:

nsBlockFrame ->
    nsMathMLTokenFrame ->
        nsTextFrame

The nsMathMLTokenFrame has an nsLineLayout in its nsHTMLReflowState, and it actually passes that nsLineLayout as the nsLineLayout for the nsTextFrame. This is not good.

> I think you want aReflowState.mCBReflowState->ComputedWidth()?

I think this could cause the text frame to break, which doesn't seem like a good idea at all since MathML can't handle that. Which makes me think about white-space:pre text with a hard line break --- we probably have a serious bug there. Should I put a flag in the line layout that can inhibit all breaking, set that flag here, and pass in aReflowState.mCBReflowState->ComputedWidth()?

> Don't you want to consider the bounds of non-text frames that aren't spans,
> like images?

Yeah, I think so.

> The flag is internal; you should instead mention the boolean passed to
> BeginLineReflow.

Good point.

> This comment seems wrong; it looks like it's relative to the top left of the
> bounds.

Ah yes. I had a hard time deciding on the best coordinate system.

> I'd say that RelativePositionFrames must not have been called yet.  (And
> without the "...".)

OK

> Seems like you probably should call TrimTrailingWhiteSpace, although maybe
> that's a followup bug.

Yeah, I think that can be a followup bug --- not trimming whitespace shouldn't create any major problems. In this bug I just want to get MathML to play nice with the text code.

I won't call HorizontalAlignFrames. I don't think it makes sense for text inside each MathML inline element to be aligned independently.

Comment 16

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

David:

> Should I put a flag in the line layout that can inhibit all breaking,
> set that flag here, and pass in aReflowState.mCBReflowState->ComputedWidth()?

Comment 17

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

Attachment: updated patch

updated to comments. Still waiting on an answer to an outstanding question.

Comment 18

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

(In reply to comment #15)
> there. Should I put a flag in the line layout that can inhibit all breaking,
> set that flag here, and pass in aReflowState.mCBReflowState->ComputedWidth()?

Yeah, I suppose so.

> I won't call HorizontalAlignFrames. I don't think it makes sense for text
> inside each MathML inline element to be aligned independently.

My concern wasn't about the horizontal alignment; it was that there was some potentially-important stuff for Bidi text in it.

Comment 19

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

(In reply to comment #18)
> (In reply to comment #15)
> > there. Should I put a flag in the line layout that can inhibit all breaking,
> > set that flag here, and pass in aReflowState.mCBReflowState->ComputedWidth()?
> 
> Yeah, I suppose so.

I'm actually not sure how to do this if we encounter a preformatted newline or even a <br> element. 

Any suggestions, Roger? It seems to me that perhaps MathML should be using block frames to wrap inline and text frames?

Comment 20

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

Fixed by checkin for bug 397518.

Comment 21

[Jesse Ruderman]

Testcase checked in as a crashtest.