Closed Bug 1002526 Opened 6 years ago Closed 5 years ago

inline MathML does not play well with font inflation

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jfkthame, Assigned: fredw)

References

Details

Attachments

(6 files, 20 obsolete files)

45.48 KB, patch
Details | Diff | Splinter Review
35.42 KB, patch
Details | Diff | Splinter Review
3.62 KB, patch
Details | Diff | Splinter Review
8.49 KB, patch
jkitch
: review+
Details | Diff | Splinter Review
4.09 KB, patch
Details | Diff | Splinter Review
789 bytes, patch
fredw
: review+
cbook
: checkin+
Details | Diff | Splinter Review
View a page that uses inline MathML within a normal body paragraph, such as http://people.mozilla.org/~jkew/inlinemath/test.html, with font inflation active.

E.g. with a normal window in desktop Firefox, set font.size.inflation.emPerLine to 25 (and reload the page). Then vary the width of the window: this will cause the body text size to vary dynamically in order to maintain the specified emPerLine.

Note how parts of the MathML content - in particular, most of the operators - do NOT get inflation applied to them, and so as the window gets wider (and the text larger) these symbols look ever more insignificant.

The parens and square root sign also don't get inflation applied; instead, they are treated as stretchy by the MathML code, which results in poor appearance at large sizes: a "stretched" parenthesis or radical is not the same as a font-inflated one would be.
Attached patch 1002526-font-inflation.patch (obsolete) — Splinter Review
Just a quick patch to try to pass font inflation to nsMathMLChar. mfenced is not handled yet.

I have not tested much, but it looks like the vertical centering of +, =, - does not work well.
Blocks: 775060
Assignee: nobody → fred.wang
Blocks: 1004057
This is the counterpart of attachment 574215 [details] [diff] [review] for MathML frames.

(it applies on top of the patches for bug 961365)
Attachment #8413842 - Attachment is obsolete: true
This one handles the font inflation for nsMathMLChar's. I realized that SetFontFamily does not actually make use of the font metrics, so I don't need to store the font inflation on the nsMathMLChar's. The case of the <mfenced> element is still not handled for now.
Attachment #8418658 - Flags: review-
Since bug 961365 is going to take a bit more time than expected, I've refreshed the patches to apply on top of current trunk.

https://tbpl.mozilla.org/?tree=Try&rev=c76318ed26f6
Attachment #8428748 - Flags: review?(karlt)
Attachment #8428751 - Flags: review?(karlt)
Comment on attachment 8428748 [details] [diff] [review]
Part 1 - Pass the font inflation parameter to nsLayoutUtils::GetFontMetricsFor* methods

I'm not familiar with the font-inflation algorithm, sorry, so I don't know whether inflation should be calculated for individual tokens or for the entire equation.
Attachment #8428748 - Flags: review?(karlt) → review?(dbaron)
Attachment #8428751 - Flags: review?(karlt) → review+
Attachment #8418656 - Attachment is obsolete: true
Attachment #8418658 - Attachment is obsolete: true
Comment on attachment 8428748 [details] [diff] [review]
Part 1 - Pass the font inflation parameter to nsLayoutUtils::GetFontMetricsFor* methods

> I'm not familiar with the font-inflation algorithm, sorry, so I don't know whether inflation should be calculated for individual tokens or for the entire equation.

Since David Baron is busy, I'm passing review to Robert or Jonathan so that we can make progress on math fonts on FirefoxOS/Android.
Attachment #8428748 - Flags: review?(roc)
Attachment #8428748 - Flags: review?(jfkthame)
Attachment #8428748 - Flags: review?(dbaron)
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #7)
> Comment on attachment 8428748 [details] [diff] [review]
> Part 1 - Pass the font inflation parameter to
> nsLayoutUtils::GetFontMetricsFor* methods
> 
> I'm not familiar with the font-inflation algorithm, sorry, so I don't know
> whether inflation should be calculated for individual tokens or for the
> entire equation.

I think we should calculate an inflation factor once for the entire equation (rather than on a per-token basis), and then apply it to all the descendant frames. Frédéric, does that seem feasible?
(In reply to Jonathan Kew (:jfkthame) from comment #9)
> I think we should calculate an inflation factor once for the entire equation
> (rather than on a per-token basis), and then apply it to all the descendant
> frames. Frédéric, does that seem feasible?

I don't know exactly what are the requirements for the inflation factor and when it should be updated. I imagine it would be possible to compute FontSizeInflationFor(this) on the frame of the <math> element and then use the AutomaticData mechanism to make this value available to the descendants:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsIMathMLFrame.h#100
Flags: needinfo?(karlt)
For an inline equation, should the factor depend on the paragraph rather than the equation?

(In reply to Frédéric Wang (:fredw) from comment #10)
> I don't know exactly what are the requirements for the inflation factor and
> when it should be updated.

I don't know either, but I suspect an inline equation should depend on the block for the paragraph in much the same way as a span does.  displaystyle equations are blocks so perhaps that will work our for those equations too.

> I imagine it would be possible to compute
> FontSizeInflationFor(this) on the frame of the <math> element and then use
> the AutomaticData mechanism to make this value available to the descendants:

Perhaps.  I can't remember how often that gets updated.
Flags: needinfo?(karlt)
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #12)
> > I imagine it would be possible to compute
> > FontSizeInflationFor(this) on the frame of the <math> element and then use
> > the AutomaticData mechanism to make this value available to the descendants:
> 
> Perhaps.  I can't remember how often that gets updated.

I tried that in attachment 8437039 [details] [diff] [review], and when I resize the window as described by Jonathan, the nsMathMLFrame::GetPresentationDataFrom seems indeed to be called to update the presentation data.

> I don't know either, but I suspect an inline equation should depend on the
> block for the paragraph in much the same way as a span does.  displaystyle
> equations are blocks so perhaps that will work our for those equations too.

nsMathMLFrame::GetPresentationDataFrom seems to always return 1 for the font inflation on the <math>.

As I see, nsLayoutUtils::FontSizeInflationFor will call nsLayoutUtils::InflationMinFontSizeFor. Then we fall on the "FIXME: The need to null-check here is sort of a bug, and might lead to incorrect results." with data=null and thus the minsize is 0 (btw I don't know if "f" should be used instead of aFrame here) and so nsLayoutUtils::FontSizeInflationInner returns 1. I'm not sure how the algorithm is supposed to work and how to handle the <math> case here.
Flags: needinfo?(karlt)
(In reply to Frédéric Wang (:fredw) from comment #16)
> I'm not sure how the algorithm is supposed to work and how to
> handle the <math> case here.

I suggest having a look at how the inflation is handled on a span, and/or asking the author or reviewer of the inflation changes where the algorithm is documented.
Flags: needinfo?(karlt)
So as I read, nsInlineFrame only contains the font size inflation during reflow. I'm not sure when the value is available exactly, but computing that during the initial setup of presentation data does not work.

Here is a patch to handle the case of inline math. I suspect the case of block math / mtd will be slightly more work, since they don't implement nsIMathMLFrame and the computation of the size inflation value might be different. Perhaps that can be done in a separate bug.
Attachment #8428748 - Attachment is obsolete: true
Attachment #8428751 - Attachment is obsolete: true
Attachment #8437039 - Attachment is obsolete: true
Attachment #8437040 - Attachment is obsolete: true
Attachment #8437041 - Attachment is obsolete: true
Attachment #8428748 - Flags: review?(roc)
Attachment #8428748 - Flags: review?(jfkthame)
Attachment #8437767 - Flags: review?(karlt)
Attachment #8437769 - Flags: review?(karlt)
I don't know about inflation so I can't review this unless I'm told how this works.

Does the text code get its inflation factor passed in from it's container?
i.e. How does this work for spans?

It looks like FontSizeInflationFor() searches for an ancestor frame returning true for IsContainerForFontSizeInflation().

If so, I assume each frame usually uses this to get the factor, like the early approach in this bug.  The factor is determined in the ancestor frame.  Is there a good reason to do things differently for MathML?

But if each frame is meant to use FontSizeInflationFor(), they why doesn't GetFontMetricsForFrame() do that for us?
Flags: needinfo?(fred.wang)
I think the font-size inflation is already computed for normal text in MathML. The problem here is with special uses of GetFontMetricsForFrame in the MathML code, which require the font inflation param. That's probably best to wait David's reply on that.
Flags: needinfo?(fred.wang)
Attachment #8428751 - Attachment is obsolete: false
Attachment #8428748 - Attachment is obsolete: false
Comment on attachment 8428748 [details] [diff] [review]
Part 1 - Pass the font inflation parameter to nsLayoutUtils::GetFontMetricsFor* methods

@David, what would be the appropriate approach to get the font size inflation value for the nsLayoutUtils::GetFontMetricsForFrame calls in the MathML code? I initially wrote this simple patch calling FontSizeInflationFor, but I'm not sure that's so easy or otherwise it would probably have been done directly in bug 627842...

Attachment 8437767 [details] [diff] tries instead to get the font inflation parameter once for the whole <math>. However, the inflation seems to already be available for normal text frames in MathML, so there is maybe a way to get it another way?
Attachment #8428748 - Flags: feedback?(dbaron)
Comment on attachment 8428748 [details] [diff] [review]
Part 1 - Pass the font inflation parameter to nsLayoutUtils::GetFontMetricsFor* methods

(In reply to Frédéric Wang (:fredw) from comment #23)
> @David, what would be the appropriate approach to get the font size
> inflation value for the nsLayoutUtils::GetFontMetricsForFrame calls in the
> MathML code?

It's certainly a reasonable approach.  Although I somewhat suspect that for MathML it's particularly important that the entire math expression use the same font inflation.  FontSizeInflationFor seems *likely* to yield that result, although not guaranteed.  (It's possible that you want to make it so that it's guaranteed to yield that result.)

That assumes that it isn't possible to do explicit sizing (e.g., 'width') within a MathML expression.  (I don't think you can, but maybe I'm wrong.)

> I initially wrote this simple patch calling
> FontSizeInflationFor, but I'm not sure that's so easy or otherwise it would
> probably have been done directly in bug 627842...

I remember why I didn't do this for MathML then.  I might have been worried about different parts of the MathML getting different sizes, which I think might (maybe?) have been more of a problem with the original implementation than the way it ended up.

I might have just been unsure about whether we wanted font inflation for MathML at all.

> Attachment 8437767 [details] [diff] tries instead to get the font inflation
> parameter once for the whole <math>.

That also seems like a reasonable option, although it might be worth making FontSizeInflationFor return a consistent result to match (as I mentioned above).

> However, the inflation seems to already
> be available for normal text frames in MathML, so there is maybe a way to
> get it another way?

I'm not quite sure what you mean here.

Sorry for the delay getting back to you.
Attachment #8428748 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 8437768 [details] [diff] [review]
Part 1 - Pass the font inflation parameter to nsLayoutUtils::GetFontMetricsFor* methods

Of the users of GetFontMetricsForFrame, most call
FontSizeInflationFor(const nsIFrame*) and pass the result.

The exceptions that use the default inflation of 1.0 are:

  nsMeterFrame and nsProgressFrame, which were added after font-inflation was
  introduced.  Perhaps these should also use FontSizeInflationFor().

  accessible/base/TextAttrs.cpp, where sizes are not used.

  xul and mathml, which were not updated in
  https://hg.mozilla.org/mozilla-central/rev/ccabd715b232

It seems that ideally GetFontMetricsForFrame() would calculate the inflation
if not provided and 1.0, special, or cached inflation values can be provided
where required.  We would have the right behavior for MathML without any other changes.

However, this approach may be a little more efficient anyway than looking up the frame hierarchy all the time.
Attachment #8437768 - Flags: review?(karlt) → review+
Comment on attachment 8437767 [details] [diff] [review]
Part 0 - Add the font inflation parameter for inline formulas to the presentation data.

Does this bug only demonstrate for inline math?
Do you know math in block frames is not affected?
Attachment #8437767 - Flags: review?(karlt) → review+
Comment on attachment 8437769 [details] [diff] [review]
Part 2 - Pass the font inflation parameter to nsMathMLChar.

What was the reason for passing around the font inflation so that it can be
used in NormalizeDefaultFont, instead of applying the inflation to the size,
as in attachment 8428751 [details] [diff] [review]?  The previous approach seemed simpler.
(In reply to Karl Tomlinson (:karlt) from comment #26)
> Does this bug only demonstrate for inline math?

Oh, I forgot comment 18.  Does the approach of attachment 8428748 [details] [diff] [review] work better for block and mtd?  If so, then that would be better, I think.
(In reply to Karl Tomlinson (:karlt) from comment #28)
> (In reply to Karl Tomlinson (:karlt) from comment #26)
> > Does this bug only demonstrate for inline math?
> 
> Oh, I forgot comment 18.  Does the approach of attachment 8428748 [details] [diff] [review]
> [diff] [review] work better for block and mtd?  If so, then that would be
> better, I think.

Yes, as I recall the first method gave the best result.
(In reply to Karl Tomlinson (:karlt) from comment #27)
> Comment on attachment 8437769 [details] [diff] [review]
> Part 2 - Pass the font inflation parameter to nsMathMLChar.
> 
> What was the reason for passing around the font inflation so that it can be
> used in NormalizeDefaultFont, instead of applying the inflation to the size,
> as in attachment 8428751 [details] [diff] [review]?  The previous approach
> seemed simpler.

GetMetricsFor(font,...) is called twice. One in nsMathMLChar::StretchInternal and one in SetFontFamily. It seems to me that the font passed to SetFontFamily should also have the right size applied, so that the out parameter aFontGroup will still give the correct metrics. Since all the aFont parameter passed to SetFontFamily are all normalized by "NormalizeDefaultFont", putting the size correction in NormalizeDefaultFont seemed the right thing to do.

(In reply to Karl Tomlinson (:karlt) from comment #28)
> (In reply to Karl Tomlinson (:karlt) from comment #26)
> > Does this bug only demonstrate for inline math?

So as I recall this bug also demonstrates for block math. My second attempt didn't work very with block and mtd while I didn't have any problem with the first approach. According to David's reply, this first approach is reasonable but might not always work (I couldn't find a counter example). So at the moment, that's the one I'm going to suggest...

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #24)
> That assumes that it isn't possible to do explicit sizing (e.g., 'width')
> within a MathML expression.  (I don't think you can, but maybe I'm wrong.)

It's for example to use <mpadded> to do explicit sizing. I don't know which kind of problems this could give...
Flags: needinfo?(karlt)
Comment on attachment 8428748 [details] [diff] [review]
Part 1 - Pass the font inflation parameter to nsLayoutUtils::GetFontMetricsFor* methods

(In reply to Frédéric Wang (:fredw) from comment #30)
> (In reply to Karl Tomlinson (:karlt) from comment #28)
> > (In reply to Karl Tomlinson (:karlt) from comment #26)
> > > Does this bug only demonstrate for inline math?
> 
> So as I recall this bug also demonstrates for block math. My second attempt
> didn't work very with block and mtd while I didn't have any problem with the
> first approach. According to David's reply, this first approach is
> reasonable but might not always work (I couldn't find a counter example). So
> at the moment, that's the one I'm going to suggest...

OK.  Let's go with the first approach then.

I don't mind whether you call the parameter aInflation or aFontSizeInflation in the other patch, so adjust if you like.
Attachment #8428748 - Flags: review+
Flags: needinfo?(karlt)
Attachment #8437767 - Attachment is obsolete: true
Comment on attachment 8437769 [details] [diff] [review]
Part 2 - Pass the font inflation parameter to nsMathMLChar.

(In reply to Frédéric Wang (:fredw) from comment #30)
> GetMetricsFor(font,...) is called twice. One in
> nsMathMLChar::StretchInternal and one in SetFontFamily. It seems to me that
> the font passed to SetFontFamily should also have the right size applied, so
> that the out parameter aFontGroup will still give the correct metrics.
> Since all the aFont parameter passed to SetFontFamily are all normalized by
> "NormalizeDefaultFont",

Ah, thanks.  I was mis-assuming that StretchInternal was passing its nsFont to
the StretchEnumContext methods.
Attachment #8437769 - Flags: review?(karlt) → review+
Attachment #8428748 - Attachment is obsolete: true
Attachment #8428751 - Attachment is obsolete: true
Attachment #8437768 - Attachment is obsolete: true
Attachment #8437769 - Attachment is obsolete: true
I tried to refresh the patches but now the font inflation is badly computed for token elements (other than nsMathMLChar).
When we fetch the new font group in MathMLTextRunFactory, we/I previously assumed that the font inflation set earlier would not need to be reapplied.

This assumption was incorrect.
I just edited the commit line in James' patch.
Attachment #8511478 - Attachment is obsolete: true
Attachment #8511498 - Flags: review?(roc)
Mmh, I'm no longer able to run reftests locally. I'll try again later.
Attachment #8511499 - Flags: review?(jkitch.bug)
Frédéric,

I had troubles trying to apply your patch. Could you help me?

$ git log -1
commit a92f2bc08346913555845318ca8c532ba0b2bbdc
Author: ffxbld <none@none>
Date:   Sat Oct 25 03:19:28 2014 -0700

    No bug, Automated HPKP preload list update from host bld-linux64-spot-115 - a=hpkp-update
$ git apply /tmp/1002526-part1.patch                                                  
/tmp/1.patch:172: trailing whitespace.
                      float           aFontSizeInflation) 
/tmp/1.patch:185: trailing whitespace.
                      float           aFontSizeInflation) 
error: patch failed: layout/mathml/nsMathMLContainerFrame.cpp:47
error: layout/mathml/nsMathMLContainerFrame.cpp: patch does not apply
error: patch failed: layout/mathml/nsMathMLmencloseFrame.cpp:339
error: layout/mathml/nsMathMLmencloseFrame.cpp: patch does not apply
error: patch failed: layout/mathml/nsMathMLmfencedFrame.cpp:215
error: layout/mathml/nsMathMLmfencedFrame.cpp: patch does not apply
error: patch failed: layout/mathml/nsMathMLmfracFrame.cpp:209
error: layout/mathml/nsMathMLmfracFrame.cpp: patch does not apply
error: patch failed: layout/mathml/nsMathMLmmultiscriptsFrame.cpp:179
error: layout/mathml/nsMathMLmmultiscriptsFrame.cpp: patch does not apply
error: patch failed: layout/mathml/nsMathMLmoFrame.cpp:605
error: layout/mathml/nsMathMLmoFrame.cpp: patch does not apply
error: patch failed: layout/mathml/nsMathMLmrootFrame.cpp:219
error: layout/mathml/nsMathMLmrootFrame.cpp: patch does not apply
error: patch failed: layout/mathml/nsMathMLmtableFrame.cpp:850
error: layout/mathml/nsMathMLmtableFrame.cpp: patch does not apply
error: patch failed: layout/mathml/nsMathMLmunderoverFrame.cpp:390
error: layout/mathml/nsMathMLmunderoverFrame.cpp: patch does not apply
Flags: needinfo?(fred.wang)
It seems that there are merge conflicts. I need to refresh the patches.
Flags: needinfo?(fred.wang)
(In reply to Frédéric Wang (:fredw) from comment #38)
> Created attachment 8511499 [details] [diff] [review]
> Part 4 - Add a reftest for MathML and font inflation.
> 
> Mmh, I'm no longer able to run reftests locally. I'll try again later.

For the record, it seems to be bug 1089002.
OK, I finally could execute the reftest. I needed "pref" not "test-pref" in reftest.list
Attachment #8511499 - Attachment is obsolete: true
Attachment #8511499 - Flags: review?(jkitch.bug)
Attachment #8511526 - Flags: review?(jkitch.bug)
Comment on attachment 8511526 [details] [diff] [review]
Part 4 - Add a reftest for MathML and font inflation.

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

::: layout/reftests/mathml/font-inflation-1.html
@@ +38,5 @@
> +    </script>
> +  </head>
> +  <body>
> +
> +    <p>The text '<math><mrow id="ref"><mtext>
Comment on attachment 8511526 [details] [diff] [review]
Part 4 - Add a reftest for MathML and font inflation.

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

Perhaps break the really long line into 80-character segments.

Otherwise the test is working as expected, both with and without the other patches.
Attachment #8511526 - Flags: review?(jkitch.bug) → review+
Apparently my comment was last (maybe because of the very long line?). There is a problem on the try server (https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=543fcb7a6fc0) because they miss glyph for the Plane 1 italic B. So we'll need to use a custom font.

Moreover, from attachment 8511557 [details] it seems that there is a problem with <mtable> elements...
Attachment #8511526 - Attachment is obsolete: true
Comment on attachment 8511498 [details] [diff] [review]
Part 3 - Pass the font inflation parameter to MathMLTextRunFactory.

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

::: layout/generic/MathMLTextRunFactory.cpp
@@ +739,5 @@
> +    // We need to not run font.size through floats when it's large since
> +    // doing so would be lossy.  Fortunately, in such cases, aInflation is
> +    // guaranteed to be 1.0f.
> +    if (mFontInflation != 1.0f) {
> +      font.size = NSToCoordRound(font.size * mFontInflation);

Just make this unconditional.
Attachment #8511498 - Flags: review?(roc) → review+
The current behaviour of tables is for each table cell to have its own value for font inflation and for table metrics to be calculated with font inflation disabled.

This patch disables both of those features, forcing font inflation to be determined using the <math> ancestor's font inflation parameter and to always consider font inflation when determining metrics.

A disadvantage of this approach is that very wide tables will run off the edge of the screen.  (Setting style="word-wrap:normal" in <mtd> elements will allow overly long table cells to overflow, but may give misleading renderings until a proper line breaking solution is implemented.)

Further testing is needed to ensure this doesn't cause unpleasant regressions.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> ::: layout/generic/MathMLTextRunFactory.cpp
> @@ +739,5 @@
> > +    // We need to not run font.size through floats when it's large since
> > +    // doing so would be lossy.  Fortunately, in such cases, aInflation is
> > +    // guaranteed to be 1.0f.
> > +    if (mFontInflation != 1.0f) {
> > +      font.size = NSToCoordRound(font.size * mFontInflation);
> 
> Just make this unconditional.

@James: can you comment on why you need that?

(In reply to jkitch.bug@internode.on.net from comment #50)
> Created attachment 8514053 [details] [diff] [review]
> Part 5 - Force <math> element's font inflation in <mtd> descendants

That seems to work well after a quick test. Given that contrary to HTML, MathML tables are intended to do some 2D layout of mathematical subexpressions rather than merely organizing some data in rows and columns, it seems natural to apply the same inflation method as for mfrac, munderover etc So that seems a sensible approach.

Feedback from people at CICM regarding the Firefox OS math apps was that we can't zoom in (bug 830306) so having font inflation would help here. And of course, I expect we'll implement MathML line breaking which is important for mobile.
(In reply to Frédéric Wang (:fredw) from comment #51)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> > ::: layout/generic/MathMLTextRunFactory.cpp
> > @@ +739,5 @@
> > > +    // We need to not run font.size through floats when it's large since
> > > +    // doing so would be lossy.  Fortunately, in such cases, aInflation is
> > > +    // guaranteed to be 1.0f.
> > > +    if (mFontInflation != 1.0f) {
> > > +      font.size = NSToCoordRound(font.size * mFontInflation);
> > 
> > Just make this unconditional.
> 
> @James: can you comment on why you need that?
> 

It was copy/pasted from nsLayoutUtils::GetFontMetricsForStyleContext(), from which I adapted the font metric caching code.

https://hg.mozilla.org/mozilla-central/annotate/80e18ff7c7b2/layout/base/nsLayoutUtils.cpp#l3465

I cannot comment on whether it is likely to be an issue in MathML - I included it as it seemed to do no harm.
Attachment #8511599 - Attachment is obsolete: true
Attachment #8514053 - Flags: review?(roc)
This adds a test for mtable.

It also increases a bit the error tolerance since mac & android fail:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d1bbb2b2ce4b
Attachment #8517951 - Flags: review?(jkitch.bug)
Comment on attachment 8514053 [details] [diff] [review]
Part 5 - Force <math> element's font inflation in <mtd> descendants

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

::: layout/base/nsLayoutUtils.cpp
@@ +6728,5 @@
>           !(aFrame->GetStateBits() & NS_FRAME_IN_CONSTRAINED_HEIGHT) &&
>           // We also want to disable font inflation for containers that have
> +         // preformatted text (excluding MathML).
> +         (styleText->WhiteSpaceCanWrap(aFrame) ||
> +          aFrame->IsFrameOfType(nsIFrame::eMathML));

Why is this change needed?

@@ +7185,5 @@
>    // root's NCA's (nearest common ancestor of its inflatable
>    // descendants) width, we could probably disable inflation in
>    // fewer cases than we currently do.
> +  if (aFrame->IsContainerForFontSizeInflation() &&
> +      !aFrame->IsFrameOfType(nsIFrame::eMathML)) {

And this one?
Attachment #8514053 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #56)
> Comment on attachment 8514053 [details] [diff] [review]
> Part 5 - Force <math> element's font inflation in <mtd> descendants
> 
> Review of attachment 8514053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +6728,5 @@
> >           !(aFrame->GetStateBits() & NS_FRAME_IN_CONSTRAINED_HEIGHT) &&
> >           // We also want to disable font inflation for containers that have
> > +         // preformatted text (excluding MathML).
> > +         (styleText->WhiteSpaceCanWrap(aFrame) ||
> > +          aFrame->IsFrameOfType(nsIFrame::eMathML));
> 
> Why is this change needed?

https://hg.mozilla.org/mozilla-central/annotate/2114ef80f6ae/layout/mathml/mathml.css#l139
mathml.css specifies the css property nowrap, which this code interprets as preformatted text and disables font inflation.

An alternative solution is to change the value of the white-space property for mtd elements, but until bug 534962 implements proper linebreaking support, we may be rendering some rather misleading tables.

> 
> @@ +7185,5 @@
> >    // root's NCA's (nearest common ancestor of its inflatable
> >    // descendants) width, we could probably disable inflation in
> >    // fewer cases than we currently do.
> > +  if (aFrame->IsContainerForFontSizeInflation() &&
> > +      !aFrame->IsFrameOfType(nsIFrame::eMathML)) {
> 
> And this one?

By default, table metrics are calculated with font inflation disabled for shrink wrapping purposes.  If font inflation is applied after table metrics are calculated, the child elements would no linger fit within the table.  By preventing font inflation from being disabled, the table is now sized appropriately to host inflated children.


I realise is a hack, but Gecko's behaviour with tables and font inflation doesn't work very well (eg bug 707195 and bug 786382).  With a proper solution this patch may not be needed, but until then this patch changes mtable from always broken with font inflation to mostly working.
Attachment #8517951 - Flags: review?(jkitch.bug) → review+
Just make it clear which part 4 you want checked in.
Attachment #8517690 - Attachment is obsolete: true
Is attachment 8514053 [details] [diff] [review] acceptable with additional comments as explained in comment 57?
Flags: needinfo?(roc)
It seems verifySizes gets called twice - once for onload and once for MozReftestInvalidate.

I apologise for missing this earlier.
Attachment #8523510 - Flags: review?(fred.wang)
Comment on attachment 8523510 [details] [diff] [review]
Part 6: Reftest followup

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

Oops, it seems that I forgot to remove that.
Attachment #8523510 - Flags: review?(fred.wang) → review+
Attachment #8523510 - Flags: checkin?
Attachment #8523510 - Flags: checkin? → checkin+
You need to log in before you can comment on or make changes to this bug.