Closed Bug 415413 Opened 16 years ago Closed 11 years ago

Incorrect widths and heights of MathML with italics in table cells

Categories

(Core :: MathML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: distler, Assigned: fredw)

References

(Blocks 1 open bug)

Details

Attachments

(12 files, 18 obsolete files)

9.55 KB, application/xhtml+xml
Details
23.29 KB, image/png
Details
816 bytes, application/xml
Details
7.12 KB, image/png
Details
1.61 KB, text/html
Details
13.34 KB, image/png
Details
12.87 KB, image/png
Details
1.59 KB, text/html
Details
30.86 KB, image/png
Details
17.88 KB, image/png
Details
21.18 KB, image/png
Details
36.57 KB, patch
MatsPalmgren_bugz
: feedback+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008020101 SeaMonkey/2.0a1pre
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008020101 SeaMonkey/2.0a1pre

<math> elements as content of table cells leads to terrible rendering.

See the attached testcase.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Attached file testcase
Attached image correct rendering
Attached image Rendering in current nightly build (obsolete) —
Dunno if this bug is related to bug 363240. Either way, I thought it would be good to have a testcase.
Bug 364359 seems to provide another testcase. The original problem (<mtext> truncated at the first whitespace) seems to have been fixed. But the rendering is still not correct and -- in particular -- the border on the <math> element is clearly wrong. 
Could be a regression from Bug 300030 (regressed between 7-12-2006 and 14-12-2006).
A fix for bug 363240 should fix this.

Depends on: 363240
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file italic widths testcase
Things look better with the changes made so far in bug 363240 but no enough space is being provided for italic characters.
Assignee: nobody → mozbugz
Status: NEW → ASSIGNED
Looks like we'll need the GetPrefTightInkHBounds API of bug 363240 comment 61.
(or change the spacing of italic characters)
Flags: blocking1.9?
OS: Mac OS X → All
Priority: -- → P3
Flags: tracking1.9? → blocking1.9?
Flags: blocking1.9? → blocking1.9-
Priority: P3 → P1
On the page http://www.mozilla.org/projects/mathml/demo/texvsmml.xhtml , the MathML content in case 13 overflows on the right. This is the last case that's obviously wrong on this MathML Torture Test.

Is this covered by this bug ? The two attached test case seem right with the same nightly (2008031605).

That is the case  the remaining work-in-prograss patch on bug 363240 fixes.
(In reply to comment #11)
> This is the last case that's obviously wrong on this MathML Torture Test.

The j in 23 is too close to the closing bracket but that is not as obvious.

> The two attached test case seem right with the same nightly (2008031605).

That probably depends on your fonts (and possibly the platform).  

On Linux, with STIX fonts:

With attachment 301094 [details] the table row headers (under j) are (incorrectly) wrapped so that a second line is formed.

With attachment 306114 [details], I'm still seeing the behavior shown in attachment 306117 [details].
(In reply to comment #13)
Yes, in fact there's still is the incorrect table row headers (under j) wrapping under Windows with STIX fonts.

The strange thing about the second case is that it's correct for me with 100% zoom (except that the characters might be too close to the border), but it breaks again when you change the zoom level, in exactly the old way (both with text only and full zoom, but not at exactly the same levels).

(In reply to comment #12)
Would be good to have more test cases on bug 363240 now that the one that's attached works correctly.


Marking wanted1.9.0x+ per discussion with Karl.  Roc, if you disagree, please comment and change.  
Flags: wanted1.9.0.x+
I'd make it wanted-next, but this is fine.
I have a <table> with <math><frac bevelled="true"> in it. The width is calculated, as it is bevelled="falce".
If there is a mfrac bevelled="true" in table, the total width is calculated, as it is mfrac bevelled="falce".
I use Firefox 3 beta 4, STIX beta fonts, Windows XP SP2
Comment on attachment 311841 [details]
Shows, how math is incorrectly rendered in a table cell

(In reply to comment #18)
> I use Firefox 3 beta 4, STIX beta fonts, Windows XP SP2

That was fixed in bug 363240 and so should render fine with the nightly builds and Beta 5 (when that is available).
Attachment #311841 - Attachment is obsolete: true
Blocks: 428906
Summary: Incorrect widths and heights of MathML in table cells → Incorrect widths and heights of MathML with italics in table cells
Decreasing bug priorities, to follow David's definition:
http://dbaron.org/log/20090120-bug-priorities
(i.e. make these MathML bugs not blocking any release)
Priority: P1 → P3
This seems like basically the same bug.
Attachment #624638 - Attachment mime type: text/plain → text/html
Depends on: 785956
It seems to me that nsMathMLTokenFrame::Reflow calls nsMathMLContainerFrame::ReflowChild for each child frame. Something specific is done to compute the bounding box of foreign elements. Can we do something similar for text frames? What is the bounding box that we would like to get?
nsMathMLContainerFrame::ReflowChild uses ComputeTightBounds for bounding metrics of foreign frames.
When frames are positioned beside adjacent frames usually essentially the union of frame bounds and these bounding metrics are used for the frame's width.

I doubt it will be appropriate to call ComputeTightBounds() while getting intrinsic widths as Reflow would not have happened at that stage.

If the child text frames of token frames can be marked so as to include (tight) ink overflow in their width (intrinsic at least, but probably also frame width to be consistent), then that would be a solution here.
Assignee: karlt → nobody
Attached patch Patch V1 (obsolete) — Splinter Review
Does that make sense if I replace TEXT_FORCE_TRIM_WHITESPACE by TEXT_IS_IN_TOKEN_MATHML? I think this bit is only used by MathML tokens. And we'll probably need a way to identify MathML tokens if we do more stuff with mathvariant, mi or primes so it's probably best to use only one single bit.

> If the child text frames of token frames can be marked so as to include
> (tight) ink overflow in their width (intrinsic at least, but probably also
> frame width to be consistent), then that would be a solution here.

I'm not sure which measurement function I'm supposed to use and which text frame members I need to modify, here. Can you be more specific?
(In reply to Frédéric Wang (:fredw) from comment #25)
> Does that make sense if I replace TEXT_FORCE_TRIM_WHITESPACE by
> TEXT_IS_IN_TOKEN_MATHML?

Sounds reasonable.

> > If the child text frames of token frames can be marked so as to include
> > (tight) ink overflow in their width (intrinsic at least, but probably also
> > frame width to be consistent), then that would be a solution here.
> 
> I'm not sure which measurement function I'm supposed to use

I expect it should match what ComputeTightBounds uses.

> and which text
> frame members I need to modify, here. Can you be more specific?

I'm expecting intrinsic widths should be returned by GetMinWidth() and GetPrefWidth().
Frame width should be returned by Reflow().
(In reply to Karl Tomlinson (:karlt) from comment #26)
> I'm expecting intrinsic widths should be returned by GetMinWidth() and
> GetPrefWidth().

These functions need a nsRenderingContext if I remember what I've read yesterday. So perhaps you are suggesting that we call them directly in Place, instead of these SaveReflowAndBoundingMetricsFor / GetReflowAndBoundingMetricsFor used to save the result of nsMathMLContainerFrame::ReflowChild.

> Frame width should be returned by Reflow().

Isn't this already returned by the call to nsContainerFrame::ReflowChild in nsMathMLContainerFrame::ReflowChild?
(In reply to Frédéric Wang (:fredw) from comment #27)
> These functions need a nsRenderingContext if I remember what I've read
> yesterday. So perhaps you are suggesting

I'm not really making a suggestion, because I haven't worked out the best way to solve this, but it is nsMathMLContainerFrame::GetIntrinsicWidth() that is getting things wrong.

> > Frame width should be returned by Reflow().
> 
> Isn't this already returned by the call to nsContainerFrame::ReflowChild in
> nsMathMLContainerFrame::ReflowChild?

I expect so, but this is the width without ink overflow.
If intrinsic width methods return width including overflow, then it would be more consistent for frame width from Reflow to also include the overflow.
No longer blocks: 428906
Mass change: setting priority to 3 for bugs preventing Gecko's Native MathML to be enabled by default in MathJax.
Keywords: helpwanted
Status: ASSIGNED → NEW
Hardware: PowerPC → All
Whiteboard: [mentor=fredw][lang=c++]
(In reply to dbarton from comment #22)
> Created attachment 624638 [details]
> Preferred width wrong for <input> in <mtext> in MathML
> 
> This seems like basically the same bug.

I guess the problem is different here: it's foreign objects, not text frames, that have the wrong size. I'm not sure I see the bug on Dave's testcase, though. Here is a screenshot on Linux.
Attached patch Patch V2 (obsolete) — Splinter Review
Just uploading the latest version of my patch if someone wants to work on this. From by basic experiments, nsTextFrameThebes::GetPrefWidth and nsTextFrameThebes::GetMinWidth are never called (it is indicated in the code that they are necessary for XUL). I tried to add something to nsTextFrame::AddInlineMinWidthForFlow and nsTextFrame::AddInlinePrefWidthForFlow but the TEXT_IS_IN_TOKEN_MATHML state bit does not seem to be set when the functions are called. I moved this state bit to another location not shared with NS_FRAME_IS_PUSHED_FLOAT, without success.
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Keywords: helpwanted
Whiteboard: [mentor=fredw][lang=c++]
Attachment #301096 - Attachment is obsolete: true
Attachment #686720 - Attachment is obsolete: true
Attachment #687683 - Attachment is obsolete: true
Attached patch Patch V3 (obsolete) — Splinter Review
Just refreshing the patch...

This still does not seem to work. I don't know the text rendering code at all, so I'm asking feedback to check if I'm doing something wrong...
Attachment #753303 - Flags: feedback?(robert)
(In reply to Frédéric Wang (:fredw) from comment #31)
> I tried to add something to
> nsTextFrame::AddInlineMinWidthForFlow and
> nsTextFrame::AddInlinePrefWidthForFlow but the TEXT_IS_IN_TOKEN_MATHML state
> bit does not seem to be set when the functions are called.

Those are the right functions. The state bit is set in SetInitialChildList on your MathML frames, so I don't see how they could fail to be set when those functions are called. This shouldn't be that hard to debug though: break in nsMathMLTokenFrame::SetInitialChildList, ensure that the state bit is set on your frame, then use a hardware data breakpoint to ensure that the state bit is not unset (or detect where it is).
Attached patch Patch V4 (obsolete) — Splinter Review
So it turns out that the state bit is never set on the text frames and I'm not sure Ehsan's code to trim the whitespaces really works. In nsMathMLTokenFrame, it seems to be assumed that these text frames are children of the MathML token frames. However, debugging it and looking at nsCSSFrameConstructor, I understand that the text frames are wrapped in a block frame and this block frame is the child of the token frame.

This new version of the patch modifies nsMathMLTokenFrame to set the state bit correctly. The code to use gfxFont::TIGHT_HINTED_OUTLINE_EXTENTS is now executed and that returns a larger width in some cases. However, that still does not always give a large enough intrinsic width.
Attachment #753303 - Attachment is obsolete: true
Attachment #753303 - Flags: feedback?(robert)
Attached patch Patch V5 (obsolete) — Splinter Review
So this patch should fix the remaining issues with Jacques's and Karl's testcases. I've never been able to reproduce the bug with Dave's testcase, but it's possible that the patch fixes issues with foreign objects as well.

The remaining issue was again due to
http://hg.mozilla.org/mozilla-central/rev/d2943858ac51
I'm guessing that in the past computing the reflow metrics of text with the NS_REFLOW_CALC_BOUNDING_METRICS in nsMathMLTokenFrame::Reflow used to set mBoundingMetrics correctly. Now that the text frames are wrapped in a block frame and that NS_REFLOW_CALC_BOUNDING_METRICS does not have any effect, we have to set mBoundingMetrics directly in nsMathMLTokenFrame::Reflow.

I've set the left/right bearings as in nsMathMLContainerFrame::GetIntrinsicWidth, but I'm not really sure it is correct. I think a better approach would be to finish the refactoring suggested in bug 397518 comment 12: remove CALC_BOUNDING_METRICS and use ComputeTightBounds. Thus I'll work on a new patch.
Attachment #754164 - Attachment is obsolete: true
I've just tried to debug a simple test case

    <math>
      <mtable>
        <mtr>
          <mtd>
            <mtext><span style="font-style: italic;">f</span></mtext>
          </mtd>
        </mtr>
      </mtable>
    </math>

Here is what I see:

1) nsMathMLContainerFrame::GetMinWidth/GetPrefWidth => nsMathMLContainerFrame::MeasureForWidth => nsMathMLTokenFrame::Place is called several times to measure the intrinsic width. At that time, the bounding box for me is something like

{ leftBearing = 0, rightBearing = 300, ascent = 0, descent = 0, width = 300 }

2) nsMathMLTokenFrame::Reflow is called and uses nsMathMLContainerFrame::ReflowChild => ComputeTightBounds so that the bounding box now includes the ink overflow e.g.

{leftBearing = -20, rightBearing = 440, ascent = 680, descent = 200, width = 300}

and this value is saved via SaveReflowAndBoundingMetricsFor.

3) nsMathMLTokenFrame::Place is called again to give the final position and GetReflowAndBoundingMetricsFor now returns the metrics saved in 2).

Here is a new experimental patch to call ComputeTightBounds during the width computation and include the ink overflow in the width (as Karl suggested). Some issues:

- I'm not sure ComputeTightBounds was intended to be called before reflow. Currently there is an assertion in the trim whitespace code of the text frame to prevent people from doing that.

- The way the values are cached/retrieved should probably be improved, but it's just an experimental patch.

- The last test in table-width-3 (token frame with several italic children) has a random success/failure.

- Using Karl's suggestion may break a few MathML reftests (I need to verify, but that was the case with my previous patches). It certainly changes the MathML text spacing.
Attachment #780368 - Flags: feedback?(roc)
Attachment #780368 - Flags: feedback?(karlt)
(In reply to Frédéric Wang (:fredw) from comment #37)
> - I'm not sure ComputeTightBounds was intended to be called before reflow.
> Currently there is an assertion in the trim whitespace code of the text
> frame to prevent people from doing that.

Yes. We could change the assertions in nsTextFrame::GetTrimmedOffsets to not assert in a MathML context, but you should probably set TEXT_START_OF_LINE and TEXT_END_OF_LINE flags manually in that case (assuming you actually want trimming).
I'm not understanding the problem with the approach in attachment 772577 [details] [diff] [review], except that it needs to add any overflow on the left hand side of the bounding box from MeasureText(), in AddInlinePrefWidthForFlow().

Doesn't that claim a similar width as the italic correction in RowChildFrameIterator produces during reflow with bounding metrics from ComputeTightBounds() in ReflowChild().
(In reply to Karl Tomlinson (:karlt) from comment #39)
> I'm not understanding the problem with the approach in attachment 772577 [details] [diff] [review]
> [details] [diff] [review], except that it needs to add any overflow on the
> left hand side of the bounding box from MeasureText(), in
> AddInlinePrefWidthForFlow().
> 

(Regarding the overflow on the left hand side: I tried that too and IIRC it broke two or three MathML reftests while adding only the right overflow broke less tests... so that's the only reason why I only included the right overflow. But I need to verify that)

I think either of the approach will work. Note however that we are only adding the trim space flag to direct text nodes of the token element (and I don't think the MathML spec says anything for text nodes from foreign languages) while the bug happens for text at arbitrarily level e.g.

<mtext><span><span style="font-style: italic">f</span><span><i>f</i></span></span></mtext>

so we would like to use another flag to indicate "include ink overflow" to any text node descendants. (BTW, I think I finally see the bug in attachment 624638 [details] if I use a smaller zoom level. So there is at least a concrete use case.)
But I suspect that's similar to what the old code did with the NS_REFLOW_CALC_BOUNDING_METRICS except that it only applied to reflow, and Robert changed that to use ComputeTightInkBound instead of propagating the tight ink bound?
Although, on the surface, ComputeTightBounds() sounds like it will deal with arbitrary hierarchies in foreign languages better, I'm not sure what to expect from that method when Reflow has not been performed.
As discussed with Karl and indicated in a TODO comment, I've introduced a GetIntrinsicHBounds function. At the moment it essentially does the same as ComputeTightBounds but does not trim whitespace (I think this does not matter since white spaces do not change the ink metrics). I'm not quite sure about whether I can use GetPosition() on children if they are not reflowed yet, though... I still get a random failure with the last test of attachment 780368 [details] [diff] [review] but otherwise this patch seems to pass the MathML tests and fixes the bug. I've submitted it to the try server:

https://tbpl.mozilla.org/?tree=Try&rev=1af7b276ff64
Attachment #780368 - Flags: feedback?(roc)
Attachment #780368 - Flags: feedback?(karlt)
Attachment #781982 - Flags: feedback?(karlt)
Comment on attachment 781982 [details] [diff] [review]
Patch with GetIntrinsicHBounds - V1

(In reply to Frédéric Wang (:fredw) from comment #42)
> At the moment it essentially does the same as
> ComputeTightBounds but does not trim whitespace (I think this does not
> matter since white spaces do not change the ink metrics).

Although leading whitespace does not change the distance between left and
right bounds, I would have expected it to alter the position of the ink
metrics, which might be important, but not very often.

> I'm not quite sure
> about whether I can use GetPosition() on children if they are not reflowed
> yet, though...

Yes, I don't think that can work because frames haven't been positioned if
they haven't yet been reflowed.

The base nsFrame method can just return what GetPrefWidth returns.

The nsBlockFrame implementation is more complicated because it needs to use
AddInlinePrefWidth() or similar to calculated projected child frame
x-positions.

If that is getting complicated, then I suggest getting feedback from :roc or
:dbaron before going to too much trouble.
Attachment #781982 - Flags: feedback?(karlt) → feedback+
Comment on attachment 781982 [details] [diff] [review]
Patch with GetIntrinsicHBounds - V1

Any hints on how to implement GetIntrinsicHBounds for the block frame without copying all the complexity of the reflow code?

(see comment 42 and comment 43)
Attachment #781982 - Flags: feedback?(roc)
Adding link to MathJax's issue 558. This bug has made necessary to add several hacks in MathJax to fix the widths of the <math> elements as well as other issues with zooming and printing but they don't seem to work very well together.
Comment on attachment 781982 [details] [diff] [review]
Patch with GetIntrinsicHBounds - V1

Review of attachment 781982 [details] [diff] [review]:
-----------------------------------------------------------------

For nsBlockFrame I think you can just duplicate nsBlockFrame::GetPrefWidth and modify that.

::: layout/generic/nsFrame.cpp
@@ +4004,5 @@
> +      nsIFrame* child = childFrames.get();
> +      nscoord lB, rB;
> +      child->GetIntrinsicHBounds(aContext, lB, rB);
> +      leftBearing = std::min(lB + child->GetPosition().x, leftBearing); 
> +      rightBearing = std::max(rB + child->GetPosition().x, rightBearing); 

I thought you wanted to call this before reflow? In that case, you can't use GetPosition here.

You might need to just implement GetIntrinsicHorizontalBounds on every frame type that MathML needs, and just NS_ERROR here to indicate that the frame doesn't support this method.

::: layout/generic/nsIFrame.h
@@ +1801,5 @@
>     */
>    virtual nsRect ComputeTightBounds(gfxContext* aContext) const;
>  
>    /**
> +   * Compute intrinsic ink metrics.

You'll need to describe this in more detail.

Also instead of using bearings, wouldn't it be better to return an x and a width (relative to the frame's border-box)?

@@ +1805,5 @@
> +   * Compute intrinsic ink metrics.
> +   */
> +  virtual void GetIntrinsicHBounds(gfxContext* aContext,
> +                                   nscoord&    leftBearing,
> +                                   nscoord&    rightBearing) const;

GetIntrinsicHorizontalBounds?
New patch. To measure intrinsic ink metrics, I trim whitespaces for all text children. That should be ok for MathML since in general there is only one single text node.

For nsBlock, I've essentially copied GetPrefWidth. For the other frames, I just do as if the children are layout on the same line. In table-width-3.html,

<math>
  <mtext><span>fff</span></mtext>
</math>

is slightly too short and similarly

          <math>
              <mtext><span>f</span></mtext>
              <mtext><span>f</span></mtext>
              <mtext><span>f</span></mtext>
          </math>

line wraps. However, perhaps these cases are not too serious and I can remove them.
Attachment #781982 - Attachment is obsolete: true
Attachment #781982 - Flags: feedback?(roc)
Attachment #804917 - Flags: feedback?(roc)
Attachment #804917 - Flags: feedback?(karlt)
Comment on attachment 804917 [details] [diff] [review]
Patch with GetIntrinsicHBounds - V2

Can TEXT_FORCE_TRIM_WHITESPACE be handled in the different patch?
That is an issue on its own. 

The base nsFrame method can just return what GetPrefWidth returns.
Assuming that frames add their child widths doesn't seem quite right for the
base class.

>+          currentKidX += kidX + kidWidth;

The distance between child frames in blocks depends on the child intrinsic
width (pref or min), not its horizontal ink bounds.

The nsBlockFrame implementation needs to use AddInlinePrefWidth() or similar
to calculate projected child frame x-positions.

The GetPrefWidth() implementation(s) for MathML frames can consider horizontal
ink bounds where appropriate.

>+   * This is similar to ComputeTightBounds, but used before reflow to compute
>+   * an intrinsic width.
>+   *
>+   * @param aContext a rendering context that can be used if we need
>+   * to do measurement
>+   * @param x        computed left coordinate of the tight bounding rectangle
>+   * @param width    computed intrinsic width of the tight bounding rectangle

This should specify the origin of the coordinate system for x.

>-  aDesiredSize.width = mBoundingMetrics.width;
>+  if (aPlaceOrigin) {
>+    aDesiredSize.width = mBoundingMetrics.width;
>+  } else {
>+    aDesiredSize.mBoundingMetrics.width = 
>+      std::max(mBoundingMetrics.width, mBoundingMetrics.rightBearing) -
>+      std::min(0, mBoundingMetrics.leftBearing);
>+    aDesiredSize.width = aDesiredSize.mBoundingMetrics.width;

Why is this necessary?
Doesn't PositionRowChildFrames() do this already?

(In reply to Frédéric Wang (:fredw) from comment #47)
>           <math>
>               <mtext><span>f</span></mtext>
>               <mtext><span>f</span></mtext>
>               <mtext><span>f</span></mtext>
>           </math>
> 
> line wraps. However, perhaps these cases are not too serious and I can
> remove them.

Perhaps this is due to the block frame calculations needing intrinsic widths.

I don't know much about the text frame trimmed offsets.
Attachment #804917 - Flags: feedback?(karlt) → feedback+
Comment on attachment 804917 [details] [diff] [review]
Patch with GetIntrinsicHBounds - V2

Review of attachment 804917 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +4027,5 @@
> +                                       nscoord& width) const
> +{
> +
> +  x = 0;
> +  width = 0;

I think we should just NS_ERROR here. This implementation is just wrong wherever we might use it.

::: layout/generic/nsIFrame.h
@@ +1815,5 @@
>    virtual nsRect ComputeTightBounds(gfxContext* aContext) const;
>  
>    /**
> +   * This is similar to ComputeTightBounds, but used before reflow to compute
> +   * an intrinsic width.

We still need to say more here.

The intrinsic pref-width is the width we would have if we lay out the element without taking any optional line breaks. So here I think this is like ComputeTightBounds assuming we lay out the element without taking any optional line breaks.

So maybe we should call this GetPrefWidthHorizontalBounds --- the horizontal bounds if we lay the frame out using the same layout as GetPrefWidth?

We should also list here the frame types which really implement this method.

@@ +1825,5 @@
> +   *
> +   */
> +  virtual void GetIntrinsicHorizontalBounds(gfxContext* aContext,
> +                                            nscoord& x,
> +                                            nscoord& width) const;

These parameter names should start with "a".

I think it might make more sense for these to be "aX" and "aXMost", since we'll want to use min/max most of the time to combine values from children.
Depends on: 919164
No longer depends on: 785956
Attached patch GetPrefWidthTightBounds - V3 (obsolete) — Splinter Review
Updated patch, which applies on top of James' attachment 808504 [details] [diff] [review].

I've removed the implementation of the new function for nsIFrame, and so I made the span inline-block in the tests and now all the tests pass. The bug may still happen for frames that don't implement the new function but that's probably a less serious that could be addressed later.

(In reply to Karl Tomlinson (:karlt) from comment #48)
> The distance between child frames in blocks depends on the child intrinsic
> width (pref or min), not its horizontal ink bounds.
> 
> The nsBlockFrame implementation needs to use AddInlinePrefWidth() or similar
> to calculate projected child frame x-positions.
> 

Right, I've fixed that. That's why our initial idea to just include the ink bounds in the intrinsic width as in attachment 772577 [details] [diff] [review] could not work...

> >-  aDesiredSize.width = mBoundingMetrics.width;
> >+  if (aPlaceOrigin) {
> >+    aDesiredSize.width = mBoundingMetrics.width;
> >+  } else {
> >+    aDesiredSize.mBoundingMetrics.width = 
> >+      std::max(mBoundingMetrics.width, mBoundingMetrics.rightBearing) -
> >+      std::min(0, mBoundingMetrics.leftBearing);
> >+    aDesiredSize.width = aDesiredSize.mBoundingMetrics.width;
> 
> Why is this necessary?
> Doesn't PositionRowChildFrames() do this already?

I don't remember exactly, but when I tried without that the intrinsic width was too small and when I tried to do that during aPlaceOrigin too the width was too large...
Attachment #780368 - Attachment is obsolete: true
Attachment #804917 - Attachment is obsolete: true
Attachment #804917 - Flags: feedback?(roc)
Attachment #808600 - Flags: feedback?(roc)
Attachment #808600 - Flags: feedback?(karlt)
Attached patch GetPrefWidthTightBounds - V3 (obsolete) — Splinter Review
Ooops, wrong patch, sorry.
Attachment #808600 - Attachment is obsolete: true
Attachment #808600 - Flags: feedback?(roc)
Attachment #808600 - Flags: feedback?(karlt)
Attachment #808614 - Flags: feedback?(roc)
Attachment #808614 - Flags: feedback?(karlt)
Attached patch GetPrefWidthTightBounds - V3 (obsolete) — Splinter Review
Attachment #808614 - Attachment is obsolete: true
Attachment #808614 - Flags: feedback?(roc)
Attachment #808614 - Flags: feedback?(karlt)
Attachment #808620 - Flags: feedback?(roc)
Attachment #808620 - Flags: feedback?(karlt)
Comment on attachment 808620 [details] [diff] [review]
GetPrefWidthTightBounds - V3

Review of attachment 808620 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good

::: layout/generic/nsBlockFrame.cpp
@@ +790,5 @@
> +                                      nscoord& aXMost)
> +{
> +  nsIFrame* firstInFlow = GetFirstContinuation();
> +  if (firstInFlow != this)
> +    return firstInFlow->GetPrefWidthTightBounds(aRenderingContext, aX, aXMost);

{} around body.

@@ +808,5 @@
> +        if (NS_SUCCEEDED(line->mFirstChild->
> +                         GetPrefWidthTightBounds(aRenderingContext,
> +                                                 childX, childXMost))) {
> +          aX = std::min(aX, data.currentLine + childX);
> +          aXMost = std::max(aXMost, data.currentLine + childXMost);

won't data.currentLine always be 0 here?

@@ +811,5 @@
> +          aX = std::min(aX, data.currentLine + childX);
> +          aXMost = std::max(aXMost, data.currentLine + childXMost);
> +        }
> +        data.currentLine = nsLayoutUtils::IntrinsicForContainer(aRenderingContext,
> +                        line->mFirstChild, nsLayoutUtils::PREF_WIDTH);

What's the point of doing this call? Won't data.currentLine be set to 0 by ForceBreak?

@@ +821,5 @@
> +          // percentage basis of 0 unconditionally would give strange
> +          // behavior for calc(10%-3px).
> +          const nsStyleCoord &indent = StyleText()->mTextIndent;
> +          if (indent.ConvertsToLength())
> +            data.currentLine += nsRuleNode::ComputeCoordPercentCalc(indent, 0);

{}

::: layout/generic/nsFrame.cpp
@@ +4069,5 @@
> +nsIFrame::GetPrefWidthTightBounds(nsRenderingContext* aContext,
> +                                  nscoord& aX,
> +                                  nscoord& aXMost)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

Is it possible to actually reach this? If it shouldn't be possible, then add NS_ERROR here so we get an assertion if someone comes up with a test that gets here.

::: layout/generic/nsTextFrame.cpp
@@ +2617,5 @@
>  }
>  
>  nsTextFrame::TrimmedOffsets
>  nsTextFrame::GetTrimmedOffsets(const nsTextFragment* aFrag,
> +                               bool aTrimAfter, bool postReflow)

aPostReflow

@@ +3260,5 @@
>    SetupJustificationSpacing();
>  }
>  
> +void
> +PropertyProvider::InitializeForMeasure(bool aTrimAfter)

Why this boolean parameter? Aren't we always passing true for it?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)
> won't data.currentLine always be 0 here?
> What's the point of doing this call? Won't data.currentLine be set to 0 by
> ForceBreak?

I expect so. I was not quite sure whether these would have other side effects on data than just modifying currentLine, so I just kept what was done in GetPrefWidth. I'm happy to simplify that if possible. So I'm guessing the second ForceBreak is not needed either here.

> ::: layout/generic/nsFrame.cpp
> @@ +4069,5 @@
> > +nsIFrame::GetPrefWidthTightBounds(nsRenderingContext* aContext,
> > +                                  nscoord& aX,
> > +                                  nscoord& aXMost)
> > +{
> > +  return NS_ERROR_NOT_IMPLEMENTED;
> 
> Is it possible to actually reach this? If it shouldn't be possible, then add
> NS_ERROR here so we get an assertion if someone comes up with a test that
> gets here.

Yes, for example <math><mtext><i>x</i></mtext></math>. IIRC, the HTML5 spec (or at least the RelaxNG schema used in the validator) allows any inline-block or inline HTML elements and this is used on our MDN demos. Currently I only implemented that for nsBlockFrame and nsTextFrame which are necessary for the standard case <math><mi>x</mi></math> (without foreign content). The bug may still show up for things like <i>x</i> but I think that's less serious and can be handled later. For example in MathJax, <math> elements are all wrapped in an inline-block span, which means that the bug with the standard <mi>x</mi> case appears virtually everywhere ; while things like <i>x</i> never shows up (MathJax does not use/support foreign HTML content yet)

nsMathMLContainerFrame is supposed to just ignore GetPrefWidthTightBounds if it fails and fallbacks to the metrics previously returned by nsLayoutUtils::IntrinsicForContainer.

Now, I think about it, nsBlockFrame::GetPrefWidthTightBounds should probably just returns the nsresult failure immediately if one of the child can not be measured.

> @@ +3260,5 @@
> >    SetupJustificationSpacing();
> >  }
> >  
> > +void
> > +PropertyProvider::InitializeForMeasure(bool aTrimAfter)
> 
> Why this boolean parameter? Aren't we always passing true for it?

Yes, I guess you're right. I think I just tried to be consistent with the InitializeForDisplay function, but this can be removed since we always trim the whitespace for MathML.

Thanks for the feedback. I expect come back to that bug early next week.
(In reply to Karl Tomlinson (:karlt) from comment #48)
> The GetPrefWidth() implementation(s) for MathML frames can consider
> horizontal
> ink bounds where appropriate.
> 
> Why is this necessary?
> Doesn't PositionRowChildFrames() do this already?

So I just had a quick look at the code, it seems to me that in nsMathMLContainerFrame::GetIntrinsicWidth, nsLayoutUtils::IntrinsicForContainer+GetPrefWidthTightBounds should be done only for foreign children and otherwise call nsMathMLContainerFrame::MeasureForWidth on MathML children. Then the call to MeasureForWidth will use the correct child metrics. Then the returned value desiredSize.width should actually include the left&right bearings. Do you agree?
(In reply to Frédéric Wang (:fredw) from comment #55)

Token frames don't include overflow (left/right bearings) in their frame bounds or intrinsic width, and GetIntrinsicWidth() on the parent on a token frame doesn't have a good measure for the left and right bearing.  To get left and right bearing, something similar to GetPrefWidthTightBounds() is necessary for mathml frames, or at least nsMathMLContainerFrames.

(I guess another option might be to always include overflow in mathml frame bounds and intrinsic widths, and offset the frame positions appropriately with the left bearing, but I'm not sure which frames prefer to ignore this overflow.)

Something like MeasureForWidth(), returning logical and ink bounds, would be tidier and more efficient than adding a separate GetPrefWidthTightBounds() for mathml frames, but MeasureForWidth() on its own doesn't call SaveReflowAndBoundingMetricsFor() on the children, expecting that to already be done. 

Perhaps replace GetIntrinsicWidth(), with something like GetIntrinsicWidthMetrics(), returning width, left and right bearing, perhaps even in nsBoundingMetrics or nsHTMLReflowMetrics structures.

Looks like the comment in nsMathMLContainerFrame.h on MeasureForWidth may be out of date for msqrt and menclose.
Comment on attachment 808620 [details] [diff] [review]
GetPrefWidthTightBounds - V3

>+  nsIFrame* firstInFlow = GetFirstContinuation();

Just as a heads-up, this was changed to FirstContinuation by bug 919318.
Attached patch GetPrefWidthTightBounds - V4 (obsolete) — Splinter Review
OK, I spent some time today to address the feedback comments. The determination of ink metrics for foreign children and mo/mroot/mfenced can probably still be refined later, but I think this is now ready for review.
Attachment #808620 - Attachment is obsolete: true
Attachment #808620 - Flags: feedback?(roc)
Attachment #808620 - Flags: feedback?(karlt)
Attachment #811688 - Flags: review?(roc)
Attachment #811688 - Flags: review?(karlt)
(In reply to Frédéric Wang (:fredw) from comment #58)
> Created attachment 811688 [details] [diff] [review]
> GetPrefWidthTightBounds - V4
> 
> OK, I spent some time today to address the feedback comments. The
> determination of ink metrics for foreign children and mo/mroot/mfenced can
> probably still be refined later, but I think this is now ready for review.

I am re-spinning my builds for today with your new patch.
Comment on attachment 811688 [details] [diff] [review]
GetPrefWidthTightBounds - V4

> nsMathMLContainerFrame::GetMinWidth(nsRenderingContext *aRenderingContext)

> nsMathMLContainerFrame::GetPrefWidth(nsRenderingContext *aRenderingContext)

>+  GetIntrinsicWidthMetrics(aRenderingContext, desiredSize);
>+  nsBoundingMetrics bm = desiredSize.mBoundingMetrics;

This should just return desiredSize.width here, without the overflow, because
that is the width that the frame will occupy.

However, that won't be right because of FixInterFrameSpacing(), so leave
this as is, and please add a comment explaining that this is compensating for
FixInterFrameSpacing().

>-    nsHTMLReflowMetrics desiredSize;
>-    ReflowError(*aRenderingContext, desiredSize);
>-    return desiredSize.width;
>+    ReflowError(*aRenderingContext, aDesiredSize);
>   }

Still should return early here.

>+    if (IsForeignChild(childFrame) ||
>+        childFrame->GetType() == nsGkAtoms::tableOuterFrame) {

>+    } else {
>+      nsIMathMLFrame* mathMLFrame = do_QueryFrame(childFrame);

Only nsMathMLContainerFrame implements GetIntrinsicWidthMetrics() properly;
not all nsIMathMLFrame.h frames.
So here it would be better to use
nsContainerFrame * containerFrame = do_QueryFrame(childFrame)
and test if (containerFrame).

That might actually be the better solution in
nsMathMLContainerFrame::ReflowChild() too, for consistency, and nsMathMLmtableOuterFrame::Reflow() wouldn't need to set up an nsBoundingMetrics
with meaningless data.

Otherwise, the MathML changes look good, thanks, and I'll assume roc reviewed the rest.
Attachment #811688 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #60)
> nsContainerFrame * containerFrame = do_QueryFrame(childFrame)

I mean "nsMathMLContainerFrame* containerFrame".
(In reply to Karl Tomlinson (:karlt) from comment #61)
> (In reply to Karl Tomlinson (:karlt) from comment #60)
> > nsContainerFrame * containerFrame = do_QueryFrame(childFrame)
> 
> I mean "nsMathMLContainerFrame* containerFrame".

Can I use do_QueryFrame to convert to nsMathMLContainerFrame*? I thought that would only be possible for e.g. frames like nsIMathMLFrame that implements an interface.
Attached patch GetPrefWidthTightBounds - V5 (obsolete) — Splinter Review
> That might actually be the better solution in nsMathMLContainerFrame::ReflowChild() too, for consistency, and nsMathMLmtableOuterFrame::Reflow() wouldn't need to set up an nsBoundingMetrics with meaningless data.

I'm not sure why you meant here, but replacing the IsForeignObject by !containerFrame in nsMathMLContainerFrame::ReflowChild and removing the mBoundingMetrics setting in nsMathMLmtableOuterFrame::Reflow leads to reftest failures, so I did not do that.
Attachment #811688 - Attachment is obsolete: true
Attachment #814210 - Flags: review?(karlt)
Comment on attachment 814210 [details] [diff] [review]
GetPrefWidthTightBounds - V5

Just need NS_QUERYFRAME_ENTRY(nsMathMLContainerFrame), as in nsInlineFrame:
http://hg.mozilla.org/mozilla-central/annotate/1ab6b9a0939b/layout/generic/nsInlineFrame.cpp#l42
Attachment #814210 - Flags: review?(karlt) → review+
https://tbpl.mozilla.org/?tree=Try&rev=3657ac9c8aed

So it seems that the new reftest is failing on other platforms. Any idea why?
Flags: needinfo?
Flags: needinfo?
Comment on attachment 814210 [details] [diff] [review]
GetPrefWidthTightBounds - V5

I don't know what is causing the failure.

I suggest trying some different fonts with more slope on the italic f, to try to reproduce.

>+++ b/layout/mathml/nsIMathMLFrame.h

>+  virtual void GetIntrinsicWidthMetrics(nsRenderingContext* aRenderingContext,
>+                                        nsHTMLReflowMetrics& aDesiredSize) {};

Can you remove this please, now that it is not used.
Attached patch GetPrefWidthTightBounds - V6 (obsolete) — Splinter Review
Attachment #814210 - Attachment is obsolete: true
Attached file testcase table-width-3.html (obsolete) —
@Karl: I've refresh the patch to address comment 68. I've executed the test again and they still fail on Windows 7,8 and Mac: https://tbpl.mozilla.org/?tree=Try&rev=4936ebb1bcc3. I've tried on Windows 7 and all the testcases work correctly for me. I have not tried with other fonts yet.
Attachment #8334583 - Attachment is obsolete: true
So I've tried attachment 8335148 [details] with various fonts on Linux and Windows 7. The linebreaking never happens. I see that with some fonts like Latin Modern, the ascent/descent is very large but that's unrelated to that patch. In some cases (font, zoom), there is a small one pixel error that can make the f's overlap the white frame and this could explain the small failures (however that does not make linebreaking happen). With some very special fonts (like Terminal or MS Serif, which don't seem to be vector fonts) the f's clearly goes outside the white frame (but again, that does not make linebreaking happen so I suspect the metrics of the fonts is not quite correct).

So I'm not sure where the test failures due to the linebreaking can happen. Once Jacques can confirm the bug is fixed on Mac then I suggest:
1) Try to install fonts / use Web fonts on the test server (still not sure how to do that)
2) or otherwise mark the reftest "fails" on the relevant platforms
Looks the same to me in your try_build as it does in the current release (which is to say, still not correct).
Flags: needinfo?(distler)
(In reply to distler from comment #75)
> Created attachment 8335167 [details]
> Testcase in Frédéric's try_build
> 
> Looks the same to me in your try_build as it does in the current release
> (which is to say, still not correct).

OK, I just realized the issue with the Cl_j^\pm... This could well be a different issue (than the one I was trying to fix at least, which was Karl's reduced test case attachment 306114 [details]) perhaps it's related to bug 433064 or similar. Can you please tell me what you get for attachment 8335148 [details] and attachment 306114 [details]?
Thanks Jacques. That looks better, but the linebreaking still happens in some cases.
There have been some unexplained issues apparently with Mac font metrics in the past: bug 423601, bug 654811, bug 757673.

The patches here at least provide a framework to get this right, so please file new bugs for the remaining issues, and go ahead and land what is here with necessary "fails" annotations on the tests.
Blocks: 941607
Blocks: 941611
I've opened bugs 941607 and bug 941611 for the remaining issues.

The results (without the fails-if conditions):
https://tbpl.mozilla.org/?tree=Try&rev=4936ebb1bcc3

I've just launched new tests (with fails-if conditions):
https://tbpl.mozilla.org/?tree=Try&rev=d53ca13d9767
Attached patch Patch (Final Version) (obsolete) — Splinter Review
Attachment #772577 - Attachment is obsolete: true
Attachment #8334566 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 8336041 [details] [diff] [review]
Patch (Final Version)

> layout/generic/nsIFrame.h
>+  virtual nsresult GetPrefWidthTightBounds(nsRenderingContext* aContext,
>+                                           nscoord& aX,
>+                                           nscoord& aXMost);

I think we generally prefer out-params to be pointers rather than
by reference.

And it seems to me that 'nsresult' is the wrong type for the return
value.  In nsBlockFrame::GetPrefWidthTightBounds you call it also
for inlines which will end up in:

>+nsIFrame::GetPrefWidthTightBounds(nsRenderingContext* aContext,
>+                                  nscoord& aX,
>+                                  nscoord& aXMost)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}

which will make NS_ENSURE_SUCCESS return (in the else-block).
Is that your intention, or am I missing something?

If you intend to only call GetPrefWidthTightBounds on block and text
frames that implement it, then the return type should be 'void' and
nsIFrame should MOZ_ASSERT that it's not called on other frame types.

If on the other hand it's OK to call it on any frame then the return
type should be 'bool' and nsIFrame should return false.

It looks like you want the latter.  And please document what the
return value means in nsIFrame.h in that case.


> layout/generic/nsTextFrame.cpp
>-  if (GetStateBits() & TEXT_START_OF_LINE) {
>+  if (!aPostReflow || GetStateBits() & TEXT_START_OF_LINE) {
> ...
>-  if (aTrimAfter && (GetStateBits() & TEXT_END_OF_LINE)) {
>+  if (aTrimAfter && (!aPostReflow || GetStateBits() & TEXT_END_OF_LINE)) {

I'd prefer parenthesis around the bit expressions.  I know it's not
needed above but it makes the code less error prone to change.


> layout/generic/nsTextFrame.h
>+                 bool aTrimAfter, bool postReflow = true);

aPostReflow
https://hg.mozilla.org/integration/fx-team/rev/fccb1956961e
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Backed out for Android reftest failures.
https://hg.mozilla.org/integration/fx-team/rev/e776a51ffc1d

https://tbpl.mozilla.org/php/getParsedLog.php?id=30905388&tree=Fx-Team
Flags: wanted1.9.0.x+
Flags: in-testsuite+
Flags: blocking1.9-
Whiteboard: [fixed-in-fx-team]
Attached patch bug415413.diffSplinter Review
GetPrefWidthTightBounds is used in nsMathMLContainerFrame::GetIntrinsicWidthMetrics to determine the preferred width of MathML token elements, including the ink metrics. In general, these are just block frames with text frame children. In some rare cases when one mixes HTML in MathML like

<math><mtext><span>f</span></mtext></math>

then the GetPrefWidthTightBounds will end up being called with inline frames. In that case, GetPrefWidthTightBounds will return immediately with a failure and nsMathMLContainerFrame::GetIntrinsicWidthMetrics will just be the value of nsLayoutUtils::IntrinsicForContainer. This might not provide perfect width in some cases (for example if the f is in italic). So I used a NS_ERROR_NOT_IMPLEMENTED assert to show that it is not implemented yet and leave room for further improvements. Do you want me to use bool values instead?

Here is a patch that addresses the other comments.
Attachment #8336041 - Attachment is obsolete: true
Attachment #8336669 - Flags: feedback?(matspal)
Blocks: 942084
Comment on attachment 8336669 [details] [diff] [review]
bug415413.diff

Thanks for addressing my nits.  I'm fine with returning nsresult for now
if you want to improve things progressively.
Attachment #8336669 - Flags: feedback?(matspal) → feedback+
https://hg.mozilla.org/mozilla-central/rev/f1e9448e639d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Besides the follow-up bugs, the "Preferred width wrong for <input> in <mtext> in MathML" testcase is still reproducible. 28.0a1 (2013-11-27), Win 7 x64
(In reply to Paul Silaghi, QA [:pauly] from comment #91)
> Besides the follow-up bugs, the "Preferred width wrong for <input> in
> <mtext> in MathML" testcase is still reproducible. 28.0a1 (2013-11-27), Win
> 7 x64

<input> has display=inline in theory it may not work (bug 942084), even if I have never seen the problem on Linux. You are testing on Windows and I think Dave uses a Mac so that's consistent with the fact that bug 941611 is specific to these platforms. I'll add a reference on bug 942084.
Depends on: 970710
You need to log in before you can comment on or make changes to this bug.