Closed
Bug 1002526
Opened 11 years ago
Closed 10 years ago
inline MathML does not play well with font inflation
Categories
(Core :: MathML, defect)
Core
MathML
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jfkthame, Assigned: fredw)
References
Details
Attachments
(6 files, 20 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jkitch
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
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.
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Blocks: font-inflation
Assignee | ||
Comment 2•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
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-
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8428748 -
Flags: review?(karlt)
Assignee | ||
Updated•10 years ago
|
Attachment #8428751 -
Flags: review?(karlt)
Comment 7•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8428751 -
Flags: review?(karlt) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8418656 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8418658 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
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)
Reporter | ||
Comment 9•10 years ago
|
||
(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?
Assignee | ||
Comment 10•10 years ago
|
||
(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)
That sounds reasonable.
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
(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)
Comment 17•10 years ago
|
||
(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)
Assignee | ||
Comment 18•10 years ago
|
||
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)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8437768 -
Flags: review?(karlt)
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8437769 -
Flags: review?(karlt)
Comment 21•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8428751 -
Attachment is obsolete: false
Assignee | ||
Updated•10 years ago
|
Attachment #8428748 -
Attachment is obsolete: false
Assignee | ||
Comment 23•10 years ago
|
||
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 24•10 years ago
|
||
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 25•10 years ago
|
||
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 26•10 years ago
|
||
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 27•10 years ago
|
||
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.
Comment 28•10 years ago
|
||
(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.
Assignee | ||
Comment 29•10 years ago
|
||
(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.
Assignee | ||
Comment 30•10 years ago
|
||
(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 31•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8437767 -
Attachment is obsolete: true
Comment 32•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8428748 -
Attachment is obsolete: true
Attachment #8428751 -
Attachment is obsolete: true
Attachment #8437768 -
Attachment is obsolete: true
Attachment #8437769 -
Attachment is obsolete: true
Assignee | ||
Comment 34•10 years ago
|
||
Assignee | ||
Comment 35•10 years ago
|
||
I tried to refresh the patches but now the font inflation is badly computed for token elements (other than nsMathMLChar).
Comment 36•10 years ago
|
||
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.
Assignee | ||
Comment 37•10 years ago
|
||
I just edited the commit line in James' patch.
Attachment #8511478 -
Attachment is obsolete: true
Attachment #8511498 -
Flags: review?(roc)
Assignee | ||
Comment 38•10 years ago
|
||
Mmh, I'm no longer able to run reftests locally. I'll try again later.
Attachment #8511499 -
Flags: review?(jkitch.bug)
Comment 39•10 years ago
|
||
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)
Assignee | ||
Comment 40•10 years ago
|
||
It seems that there are merge conflicts. I need to refresh the patches.
Flags: needinfo?(fred.wang)
Assignee | ||
Comment 41•10 years ago
|
||
Attachment #8511018 -
Attachment is obsolete: true
Assignee | ||
Comment 42•10 years ago
|
||
Attachment #8511019 -
Attachment is obsolete: true
Assignee | ||
Comment 43•10 years ago
|
||
(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.
Assignee | ||
Comment 44•10 years ago
|
||
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)
Assignee | ||
Comment 45•10 years ago
|
||
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 46•10 years ago
|
||
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+
Assignee | ||
Comment 47•10 years ago
|
||
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...
Assignee | ||
Comment 48•10 years ago
|
||
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+
Comment 50•10 years ago
|
||
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.
Assignee | ||
Comment 51•10 years ago
|
||
(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.
Comment 52•10 years ago
|
||
(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.
Assignee | ||
Comment 53•10 years ago
|
||
Attachment #8511498 -
Attachment is obsolete: true
Assignee | ||
Comment 54•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8511599 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8514053 -
Flags: review?(roc)
Assignee | ||
Comment 55•10 years ago
|
||
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-
Comment 57•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8517951 -
Flags: review?(jkitch.bug) → review+
Comment 58•10 years ago
|
||
Just make it clear which part 4 you want checked in.
Assignee | ||
Updated•10 years ago
|
Attachment #8517690 -
Attachment is obsolete: true
Assignee | ||
Comment 59•10 years ago
|
||
Assignee | ||
Comment 60•10 years ago
|
||
Is attachment 8514053 [details] [diff] [review] acceptable with additional comments as explained in comment 57?
Flags: needinfo?(roc)
Yes.
Flags: needinfo?(roc)
Attachment #8514053 -
Flags: review- → review+
Assignee | ||
Comment 62•10 years ago
|
||
Attachment #8514053 -
Attachment is obsolete: true
Assignee | ||
Comment 63•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d1bbb2b2ce4b
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f08e0aaea3f0
Keywords: checkin-needed
Comment 64•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5066b8517b92
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5b1b5040f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea143ae5de5
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e6b7e19361
https://hg.mozilla.org/integration/mozilla-inbound/rev/11d34328f49b
Keywords: checkin-needed
Comment 65•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5066b8517b92
https://hg.mozilla.org/mozilla-central/rev/0f5b1b5040f3
https://hg.mozilla.org/mozilla-central/rev/2ea143ae5de5
https://hg.mozilla.org/mozilla-central/rev/e1e6b7e19361
https://hg.mozilla.org/mozilla-central/rev/11d34328f49b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 66•10 years ago
|
||
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)
Assignee | ||
Comment 67•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8523510 -
Flags: checkin?
Comment 68•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fe169447c6eb
Just the one marked checkin?, thank you.
Keywords: checkin-needed
Comment 69•10 years ago
|
||
Keywords: checkin-needed
Updated•10 years ago
|
Attachment #8523510 -
Flags: checkin? → checkin+
Comment 70•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•