Last Comment Bug 415413 - Incorrect widths and heights of MathML with italics in table cells
: Incorrect widths and heights of MathML with italics in table cells
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: P3 normal with 3 votes (vote)
: mozilla28
Assigned To: Frédéric Wang (:fredw)
:
:
Mentors:
: 456358 (view as bug list)
Depends on: 363240 919164 970710
Blocks: mathml-in-mathjax 942084 941607 941611
  Show dependency treegraph
 
Reported: 2008-02-03 00:46 PST by distler
Modified: 2014-02-12 05:47 PST (History)
19 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (9.55 KB, application/xhtml+xml)
2008-02-03 00:47 PST, distler
no flags Details
correct rendering (23.29 KB, image/png)
2008-02-03 00:48 PST, distler
no flags Details
Rendering in current nightly build (33.21 KB, image/png)
2008-02-03 00:49 PST, distler
no flags Details
italic widths testcase (816 bytes, application/xml)
2008-02-27 13:42 PST, Karl Tomlinson (:karlt)
no flags Details
screenshot of italic widths testcase (7.12 KB, image/png)
2008-02-27 13:46 PST, Karl Tomlinson (:karlt)
no flags Details
Shows, how math is incorrectly rendered in a table cell (22.93 KB, image/png)
2008-03-26 12:44 PDT, Vladislav
no flags Details
Preferred width wrong for <input> in <mtext> in MathML (1.61 KB, text/html)
2012-05-16 20:50 PDT, dbarton
no flags Details
Patch V1 (9.12 KB, patch)
2012-11-29 11:43 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Screenshot of Dave's testcase (13.34 KB, image/png)
2012-11-30 09:21 PST, Frédéric Wang (:fredw)
no flags Details
Patch V2 (12.07 KB, patch)
2012-12-03 02:02 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Updated screenshot of Jacques's testcase (12.87 KB, image/png)
2013-05-23 07:11 PDT, Frédéric Wang (:fredw)
no flags Details
Patch V3 (12.08 KB, patch)
2013-05-23 08:16 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V4 (12.53 KB, patch)
2013-05-25 14:07 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V5 (17.22 KB, patch)
2013-07-09 03:16 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch using ComputeTightBounds - V1 (6.56 KB, patch)
2013-07-24 06:31 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch with GetIntrinsicHBounds - V1 (12.74 KB, patch)
2013-07-26 15:38 PDT, Frédéric Wang (:fredw)
karlt: feedback+
Details | Diff | Splinter Review
Patch with GetIntrinsicHBounds - V2 (22.68 KB, patch)
2013-09-14 11:17 PDT, Frédéric Wang (:fredw)
karlt: feedback+
Details | Diff | Splinter Review
GetPrefWidthTightBounds - V3 (22.68 KB, patch)
2013-09-23 08:21 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
GetPrefWidthTightBounds - V3 (2.42 KB, patch)
2013-09-23 08:40 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
GetPrefWidthTightBounds - V3 (23.24 KB, patch)
2013-09-23 08:53 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
GetPrefWidthTightBounds - V4 (36.03 KB, patch)
2013-09-29 07:12 PDT, Frédéric Wang (:fredw)
karlt: review-
roc: review+
Details | Diff | Splinter Review
GetPrefWidthTightBounds - V5 (36.73 KB, patch)
2013-10-07 13:38 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
GetPrefWidthTightBounds - V6 (36.48 KB, patch)
2013-11-19 08:28 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
testcase table-width-3.html (1.53 KB, text/html)
2013-11-19 08:51 PST, Frédéric Wang (:fredw)
no flags Details
testcase table-width-3.html (1.59 KB, text/html)
2013-11-19 23:47 PST, Frédéric Wang (:fredw)
no flags Details
Testcase in Frédéric's try_build (30.86 KB, image/png)
2013-11-20 00:28 PST, distler
no flags Details
Testcase 306114 in Frederic's try_build (17.88 KB, image/png)
2013-11-20 08:23 PST, distler
no flags Details
Testcase 8335148 in Frederic's try_build (21.18 KB, image/png)
2013-11-20 08:24 PST, distler
no flags Details
Patch (Final Version) (36.54 KB, patch)
2013-11-21 06:21 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
bug415413.diff (36.57 KB, patch)
2013-11-22 02:02 PST, Frédéric Wang (:fredw)
mats: feedback+
Details | Diff | Splinter Review

Description distler 2008-02-03 00:46:21 PST
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.
Comment 1 distler 2008-02-03 00:47:28 PST
Created attachment 301094 [details]
testcase
Comment 2 distler 2008-02-03 00:48:23 PST
Created attachment 301095 [details]
correct rendering
Comment 3 distler 2008-02-03 00:49:26 PST
Created attachment 301096 [details]
Rendering in current nightly build
Comment 4 distler 2008-02-03 00:55:06 PST
Dunno if this bug is related to bug 363240. Either way, I thought it would be good to have a testcase.
Comment 5 distler 2008-02-03 01:07:08 PST
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. 
Comment 6 Ria Klaassen (not reading all bugmail) 2008-02-03 12:01:40 PST
Could be a regression from Bug 300030 (regressed between 7-12-2006 and 14-12-2006).
Comment 7 Karl Tomlinson (:karlt) 2008-02-07 18:52:37 PST
A fix for bug 363240 should fix this.

Comment 8 Karl Tomlinson (:karlt) 2008-02-27 13:42:40 PST
Created attachment 306114 [details]
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.
Comment 9 Karl Tomlinson (:karlt) 2008-02-27 13:46:00 PST
Created attachment 306117 [details]
screenshot of italic widths testcase
Comment 10 Karl Tomlinson (:karlt) 2008-02-27 13:49:14 PST
Looks like we'll need the GetPrefTightInkHBounds API of bug 363240 comment 61.
(or change the spacing of italic characters)
Comment 11 Jean-Marc Desperrier 2008-03-17 03:02:20 PDT
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).

Comment 12 Bill Gianopoulos [:WG9s] 2008-03-17 03:25:35 PDT
That is the case  the remaining work-in-prograss patch on bug 363240 fixes.
Comment 13 Karl Tomlinson (:karlt) 2008-03-17 05:12:18 PDT
(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].
Comment 14 Jean-Marc Desperrier 2008-03-17 05:44:22 PDT
(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.


Comment 15 Damon Sicore (:damons) 2008-03-19 15:48:51 PDT
Marking wanted1.9.0x+ per discussion with Karl.  Roc, if you disagree, please comment and change.  
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-03-21 19:01:15 PDT
I'd make it wanted-next, but this is fine.
Comment 17 Vladislav 2008-03-26 12:38:07 PDT
I have a <table> with <math><frac bevelled="true"> in it. The width is calculated, as it is bevelled="falce".
Comment 18 Vladislav 2008-03-26 12:44:50 PDT
Created attachment 311841 [details]
Shows, how math is incorrectly rendered in a table cell

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 19 Karl Tomlinson (:karlt) 2008-03-26 20:44:57 PDT
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).
Comment 20 Karl Tomlinson (:karlt) 2008-09-23 17:10:12 PDT
*** Bug 456358 has been marked as a duplicate of this bug. ***
Comment 21 Frédéric Wang (:fredw) 2010-12-26 06:48:42 PST
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)
Comment 22 dbarton 2012-05-16 20:50:37 PDT
Created attachment 624638 [details]
Preferred width wrong for <input> in <mtext> in MathML

This seems like basically the same bug.
Comment 23 Frédéric Wang (:fredw) 2012-11-28 15:30:04 PST
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?
Comment 24 Karl Tomlinson (:karlt) 2012-11-28 15:56:42 PST
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.
Comment 25 Frédéric Wang (:fredw) 2012-11-29 11:43:01 PST
Created attachment 686720 [details] [diff] [review]
Patch V1

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?
Comment 26 Karl Tomlinson (:karlt) 2012-11-29 12:06:42 PST
(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().
Comment 27 Frédéric Wang (:fredw) 2012-11-29 12:13:04 PST
(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?
Comment 28 Karl Tomlinson (:karlt) 2012-11-29 12:25:27 PST
(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.
Comment 29 Frédéric Wang (:fredw) 2012-11-30 08:25:39 PST
Mass change: setting priority to 3 for bugs preventing Gecko's Native MathML to be enabled by default in MathJax.
Comment 30 Frédéric Wang (:fredw) 2012-11-30 09:21:26 PST
Created attachment 687137 [details]
Screenshot of Dave's testcase

(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.
Comment 31 Frédéric Wang (:fredw) 2012-12-03 02:02:03 PST
Created attachment 687683 [details] [diff] [review]
Patch V2

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.
Comment 32 Frédéric Wang (:fredw) 2013-05-23 07:11:28 PDT
Created attachment 753272 [details]
Updated screenshot of Jacques's testcase
Comment 33 Frédéric Wang (:fredw) 2013-05-23 08:16:41 PDT
Created attachment 753303 [details] [diff] [review]
Patch V3

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...
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-05-24 09:59:02 PDT
(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).
Comment 35 Frédéric Wang (:fredw) 2013-05-25 14:07:56 PDT
Created attachment 754164 [details] [diff] [review]
Patch V4

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.
Comment 36 Frédéric Wang (:fredw) 2013-07-09 03:16:04 PDT
Created attachment 772577 [details] [diff] [review]
Patch V5

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.
Comment 37 Frédéric Wang (:fredw) 2013-07-24 06:31:33 PDT
Created attachment 780368 [details] [diff] [review]
Patch using ComputeTightBounds - V1

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.
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-07-24 16:47:29 PDT
(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).
Comment 39 Karl Tomlinson (:karlt) 2013-07-24 17:09:04 PDT
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().
Comment 40 Frédéric Wang (:fredw) 2013-07-24 22:06:31 PDT
(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?
Comment 41 Karl Tomlinson (:karlt) 2013-07-25 14:25:18 PDT
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.
Comment 42 Frédéric Wang (:fredw) 2013-07-26 15:38:22 PDT
Created attachment 781982 [details] [diff] [review]
Patch with GetIntrinsicHBounds - V1

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
Comment 43 Karl Tomlinson (:karlt) 2013-07-28 21:57:44 PDT
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.
Comment 44 Frédéric Wang (:fredw) 2013-09-10 00:11:05 PDT
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)
Comment 45 Frédéric Wang (:fredw) 2013-09-10 02:19:21 PDT
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 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-09-13 17:35:14 PDT
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?
Comment 47 Frédéric Wang (:fredw) 2013-09-14 11:17:31 PDT
Created attachment 804917 [details] [diff] [review]
Patch with GetIntrinsicHBounds - V2

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.
Comment 48 Karl Tomlinson (:karlt) 2013-09-19 22:54:43 PDT
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.
Comment 49 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-09-20 03:46:01 PDT
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.
Comment 50 Frédéric Wang (:fredw) 2013-09-23 08:21:23 PDT
Created attachment 808600 [details] [diff] [review]
GetPrefWidthTightBounds - V3

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...
Comment 51 Frédéric Wang (:fredw) 2013-09-23 08:40:56 PDT
Created attachment 808614 [details] [diff] [review]
GetPrefWidthTightBounds - V3

Ooops, wrong patch, sorry.
Comment 52 Frédéric Wang (:fredw) 2013-09-23 08:53:09 PDT
Created attachment 808620 [details] [diff] [review]
GetPrefWidthTightBounds - V3
Comment 53 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-09-23 15:14:20 PDT
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?
Comment 54 Frédéric Wang (:fredw) 2013-09-24 01:29:11 PDT
(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.
Comment 55 Frédéric Wang (:fredw) 2013-09-25 01:19:42 PDT
(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?
Comment 56 Karl Tomlinson (:karlt) 2013-09-25 15:33:12 PDT
(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 57 Bill Gianopoulos [:WG9s] 2013-09-26 16:14:20 PDT
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.
Comment 58 Frédéric Wang (:fredw) 2013-09-29 07:12:15 PDT
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.
Comment 59 Bill Gianopoulos [:WG9s] 2013-09-29 12:39:35 PDT
(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 60 Karl Tomlinson (:karlt) 2013-10-03 21:54:23 PDT
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.
Comment 61 Karl Tomlinson (:karlt) 2013-10-03 21:56:36 PDT
(In reply to Karl Tomlinson (:karlt) from comment #60)
> nsContainerFrame * containerFrame = do_QueryFrame(childFrame)

I mean "nsMathMLContainerFrame* containerFrame".
Comment 62 Frédéric Wang (:fredw) 2013-10-06 08:52:03 PDT
(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.
Comment 63 Karl Tomlinson (:karlt) 2013-10-06 15:39:11 PDT
nsMathMLContainerFrame will need NS_DECL_QUERYFRAME_TARGET and NS_QUERYFRAME_ENTRY.
See for example nsInlineFrame:
http://hg.mozilla.org/mozilla-central/annotate/1ab6b9a0939b/layout/generic/nsInlineFrame.h#l38
http://hg.mozilla.org/mozilla-central/annotate/1ab6b9a0939b/layout/generic/nsInlineFrame.cpp#l42
Comment 64 Frédéric Wang (:fredw) 2013-10-07 13:38:57 PDT
Created attachment 814210 [details] [diff] [review]
GetPrefWidthTightBounds - V5

> 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.
Comment 65 Frédéric Wang (:fredw) 2013-10-07 13:42:11 PDT
https://tbpl.mozilla.org/?tree=Try&rev=796261eb6909
Comment 66 Karl Tomlinson (:karlt) 2013-10-07 15:47:12 PDT
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
Comment 67 Frédéric Wang (:fredw) 2013-10-08 12:26:41 PDT
https://tbpl.mozilla.org/?tree=Try&rev=3657ac9c8aed

So it seems that the new reftest is failing on other platforms. Any idea why?
Comment 68 Karl Tomlinson (:karlt) 2013-10-08 14:16:28 PDT
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.
Comment 69 Frédéric Wang (:fredw) 2013-11-19 08:28:48 PST
Created attachment 8334566 [details] [diff] [review]
GetPrefWidthTightBounds - V6
Comment 70 Frédéric Wang (:fredw) 2013-11-19 08:51:06 PST
Created attachment 8334583 [details]
testcase table-width-3.html
Comment 71 Frédéric Wang (:fredw) 2013-11-19 08:55:34 PST
@Jacques: can you give a try to http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/fred.wang@free.fr-4936ebb1bcc3/try-macosx64/ ?
Comment 72 Frédéric Wang (:fredw) 2013-11-19 08:59:34 PST
@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.
Comment 73 Frédéric Wang (:fredw) 2013-11-19 23:47:56 PST
Created attachment 8335148 [details]
testcase table-width-3.html
Comment 74 Frédéric Wang (:fredw) 2013-11-20 00:11:40 PST
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
Comment 75 distler 2013-11-20 00:28:57 PST
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).
Comment 76 Frédéric Wang (:fredw) 2013-11-20 00:57:44 PST
(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]?
Comment 77 distler 2013-11-20 08:23:03 PST
Created attachment 8335332 [details]
Testcase 306114 in Frederic's try_build
Comment 78 distler 2013-11-20 08:24:21 PST
Created attachment 8335333 [details]
Testcase 8335148 in Frederic's try_build
Comment 79 Frédéric Wang (:fredw) 2013-11-20 14:18:19 PST
Thanks Jacques. That looks better, but the linebreaking still happens in some cases.
Comment 80 Karl Tomlinson (:karlt) 2013-11-20 18:43:19 PST
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.
Comment 81 Frédéric Wang (:fredw) 2013-11-21 06:19:18 PST
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
Comment 82 Frédéric Wang (:fredw) 2013-11-21 06:21:04 PST
Created attachment 8336041 [details] [diff] [review]
Patch (Final Version)
Comment 83 Mats Palmgren (:mats) 2013-11-21 08:53:35 PST
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
Comment 84 Ryan VanderMeulen [:RyanVM] 2013-11-21 08:59:21 PST
https://hg.mozilla.org/integration/fx-team/rev/fccb1956961e
Comment 85 Ryan VanderMeulen [:RyanVM] 2013-11-21 11:11:21 PST
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
Comment 86 Frédéric Wang (:fredw) 2013-11-22 02:02:26 PST
Created attachment 8336669 [details] [diff] [review]
bug415413.diff

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.
Comment 87 Mats Palmgren (:mats) 2013-11-22 11:11:50 PST
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.
Comment 88 Frédéric Wang (:fredw) 2013-11-23 01:48:46 PST
https://tbpl.mozilla.org/?tree=Try&rev=e73d9eddfedd
Comment 89 Ryan VanderMeulen [:RyanVM] 2013-11-25 06:26:30 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e9448e639d
Comment 90 Ryan VanderMeulen [:RyanVM] 2013-11-25 11:27:44 PST
https://hg.mozilla.org/mozilla-central/rev/f1e9448e639d
Comment 91 Paul Silaghi, QA [:pauly] 2013-11-28 04:46:08 PST
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
Comment 92 Frédéric Wang (:fredw) 2013-11-28 05:02:22 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.