Closed Bug 414277 (scale-stretchy) Opened 16 years ago Closed 14 years ago

scale stretchy operators when there is no glyph of suitable size

Categories

(Core :: MathML, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b5

People

(Reporter: karlt, Assigned: fredw)

References

(Blocks 2 open bugs)

Details

Attachments

(7 files, 35 obsolete files)

3.73 KB, application/xhtml+xml
Details
2.30 KB, application/xhtml+xml
Details
4.35 KB, application/xhtml+xml
Details
782 bytes, application/xhtml+xml
Details
401.37 KB, image/png
Details
24.15 KB, patch
karlt
: review+
dbaron
: approval2.0+
Details | Diff | Splinter Review
11.52 KB, patch
karlt
: review+
Details | Diff | Splinter Review
For some stretchy grouping operators, the meaning (context) of the symbol depends on the size of the symbol.  If the font doesn't provide a glyph of suitable size, then we should scale the nearest-sized glyph.

This could be done either by changing the font-size, scaling by the same factor in each direction), which would work well for ∑, or by changing the transformation matrix to scale less in the non-stretchy direction, which might look better for parentheses and maybe radicals.

Doing this would improve MathML rendering for those that don't have STIX fonts installed.
With respect to stretching "large operators" like ∑, the MathML Spec,

   http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter3.html#id.3.2.5.8.2

says that these operators are supposed to have stretchy="true". As pointed out in the comments to bug 363240, TeX doesn't do that.  But the default size of these "largeop" operators in TeX is considerably larger (at least for display equations) than the current behaviour in Gecko.

For most purposes, the TeX behaviour works well enough. (It should be remembered that TeX's behaviour was determined in an era when scalable fonts didn't exist. Stretching a parenthesis with an extender glyph was one thing; stretching a summation sign would have required running Metafont on the fly.)

I'd be happy enough if the largeop operators were rendered in a larger font-size (as TeX does) in display equations.
Blocks: 416140
Blocks: 416065
Blocks: 491370
Attached patch First patch (obsolete) — Splinter Review
	Attachment 399257 [details] [diff] is a simple patch that forces a scale transform when the Stretch function is not able to find a glyph / set of glyphs of suitable size. However I don't know exactly when this scale is actually needed and I should study more carefuly how the Stretch function works.

	Attachement 399259 is a screenshot of the MathML Torture Test with the font.mathfont-family empty (right hand side = patch applied). In this case, one can see that the patch improves stretching of braces, radicals and integrals.
Comment on attachment 399259 [details]
Screenshot of MathML Torture Test with/without the patch applied

This is looking promising.
Definitely looking promising Frédéric!
The operator dictionary (in mathfont.properties) specifies whether each operator stretches by default and whether that is in a horizontal or vertical direction.
(Really the direction and default stretchiness properties should probably be orthogonal, though it looks like direction can currently only be specified when stretchy at the moment.)
Apparently the operator dictionary is not being considered before stretching.
This would prevent ',' from stretching at all (unless explicitly specified), and would constrain the direction of stretch for arrows and union.

(The broken arrows are an unrelated issue.  It could probably be improved by checking that all glyphs come from the same font but that's difficult to determine until we switch from nsIRenderingContext to gfxFontGroup at least.)
> Apparently the operator dictionary is not being considered before stretching.
> This would prevent ',' from stretching at all (unless explicitly specified),
> and would constrain the direction of stretch for arrows and union.

For the comma, I think the problem comes from nsMathMLmfenceFrame.cpp that explicitly specify a vertical stretch. I have not check yet what happens for horizontal arrows and large operators.
Attached image stretching integral symbols (obsolete) —
	I easily fixed case 5 (large op stretched horizontally) by applying scale only when mDirection has been set to vertical/horizontal by the Stretch function (I thought it was always the case).
	I added other integral symbols in case 2 and got a weird effect: the symbols are not stretched (see Attachment 399717 [details]) but we can see the scale applied when we select them.
Attachment #399257 - Attachment is obsolete: true
Attachment #399688 - Attachment is obsolete: true
Attachment #399690 - Attachment is obsolete: true
Attachment #399717 - Attachment is obsolete: true
Attached file testcase (obsolete) —
I've tried the patch with current trunk and it seems the weird effect described in comment 13 disappeared. Attachment 400542 [details] [diff] fixes the problem of large op stretched horizontally (test 5) as indicated in this comment. I also added some integral symbols in mathfont.properties, so that all these symbols are now stretched vertically (test 2).

Apart from the issue of symbols that should not be stretched vertically, it seems that the vertical position is not correct (test 2).
Attachment #400542 - Attachment is obsolete: true
Attachment #400545 - Attachment is obsolete: true
	I updated the patch so that when a MathMLChar is scaled vertically, its ascent/descent is set to those of aContainerSize. This fixes the problem of vertical alignment in test 2. The next step is to investigate why symbols are stretched vertically in tests 3 and 4...
	
	BTW, it seems that the current patch solves issues in other bugs:
- Bug 416065: tilde and check. https://bug416065.bugzilla.mozilla.org/attachment.cgi?id=301824
- Bug 301919: underbrace and overbrace. https://bug416140.bugzilla.mozilla.org/attachment.cgi?id=301919
- Bug 301824: angle brackets. https://eyeasme.com/Joe/MathML/angle_brackets_bug.html
Attached patch Fix rendering of tests 3 & 4 (obsolete) — Splinter Review
Attachment #400585 - Attachment is obsolete: true
Attached file testcase largeop (obsolete) —
Attached image Screenshot testcase largeop (obsolete) —
Now the problems of the testcase are fixed, let's turn our attention to the case of largeop. I've attached a new testcase and a screenshot with three versions (from left to right):

1) no patch applied
2) patch applied
3) patch applied + preserve aspect ratio

I think the third case is good but I don't know how to determine when aspect ratio should be preserved for a largeop (for instance, it should not be preserved for integral).
(In reply to comment #1)
> With respect to stretching "large operators" like ∑, the MathML Spec,
> 
>    http://www.w3.org/TR/2003/REC-MathML2-20031021/chapter3.html#id.3.2.5.8.2
> 
> says that these operators are supposed to have stretchy="true". As pointed out
> in the comments to bug 363240, TeX doesn't do that.  But the default size of
> these "largeop" operators in TeX is considerably larger (at least for display
> equations) than the current behaviour in Gecko.
> 
> For most purposes, the TeX behaviour works well enough. (It should be
> remembered that TeX's behaviour was determined in an era when scalable fonts
> didn't exist. Stretching a parenthesis with an extender glyph was one thing;
> stretching a summation sign would have required running Metafont on the fly.)
> 
> I'd be happy enough if the largeop operators were rendered in a larger
> font-size (as TeX does) in display equations.

I opened bug 518330 for this issue.
It seems that the Math WG is going to remove the requirement that large operators stretch vertically, so we can make symbols like sum nonstretchy:

http://lists.w3.org/Archives/Public/www-math/2009Sep/

However, we still need to deal with this case when the user explicitly ask to stretch the symbol. I agree with what is indicated in comment 10 : direction and default strechiness properties should be orthogonal. Hence I suggest to change mathfonts.properties in this way:

- "strechy" takes values true or false
- add a new attribute "direction" that takes at least values "vertical" and "horizontal".

	Then I would put the default value "both" to "direction". This would be the case where we want to preserve aspect ratio (sum, union...). We can even add a value "diagonal" for instance for diagonal arrows (BTW, this could be implemented using vertical/horizontal streching + rotation matrix). Finally, I think the syntax could be simplified for boolean properties: rather than "stretchy:true accent:true", just write "stretchy accent" (this is the syntax used in the MathML 3 REC). I guess it's worth opening a new bug to fix these issues first.
Blocks: 518330
The WG has not really answered yet. However I've already prepared an improved syntax for mathfont.properties, that it is worth doing anyway (see bug 519126)
(In reply to comment #27)
> However, we still need to deal with this case when the user explicitly ask to
> stretch the symbol. I agree with what is indicated in comment 10 : direction
> and default strechiness properties should be orthogonal. Hence I suggest to
> change mathfonts.properties in this way:
> 
> - "strechy" takes values true or false
> - add a new attribute "direction" that takes at least values "vertical" and
> "horizontal".

Sounds good.  It's not what the spec says as you pointed out very clearly,
thank you: http://lists.w3.org/Archives/Public/www-math/2009Sep/0021.html

Though these changes to mathfonts.properties seem to be appropriate for
producing the behavior that I think the spec authors expected.  There is a
hint of that in "but some renderers may be able to stretch *certain*
characters, such as diagonal arrows, in both directions independently"
(emphasis added).

> Then I would put the default value "both" to "direction". This would be the
> case where we want to preserve aspect ratio (sum, union...).

I'm happy with a default value "both" for stretching in *either* direction.
(We don't do mtd for both at once yet.)

However, "both" isn't really what's needed for sum and union.
These look best if they preserve aspect ratio, but they shouldn't stretch
horizontally just because their limits are wide, for example.
I'd prefer that "both" meant that they stretch horizontally and/or vertically
depending on their parent element.

Another tricky issue is making GetMaxWidth() work with operators that expand
to arbitrary sizes, while preserving aspect ratio.  (Really we need a
different layout algorithm, but I don't have a suitable algorithm in mind, so
let's not try to do that now.)

How about this for operators with largeop?:

* When only stretched as a largeop, increase the font size by a factor.
  Would a factor of 2 be suitable?  Or less maybe.  I haven't looked at
  textbooks for fonts to see what they do.

  This would preserve aspect ratio.  I think font size may work better with
  measuring and font hinting than a transformation, but if I'm wrong and a
  transformation works just as well then that's fine too.

* When the largeop is also stretchy, first increase font size (or symmetric
  transformation) by the largeop factor, then stretch by (additional)
  transformation only in the direction of the stretch.

This could be a reasonable compromise for having sum and union looking
reasonable in their largeop default non-stretchy state.  (If stretchy="true"
is specified and they stretch considerably, then they won't look their best, but there will be some extra width to balance things out a bit.)  Hopefully largeop integrals wouldn't look too bad with only a fixed extra thickness.
> I'm happy with a default value "both" for stretching in *either* direction.
> (We don't do mtd for both at once yet.)
> 
> However, "both" isn't really what's needed for sum and union.
> These look best if they preserve aspect ratio, but they shouldn't stretch
> horizontally just because their limits are wide, for example.
> I'd prefer that "both" meant that they stretch horizontally and/or vertically
> depending on their parent element.
> 
> Another tricky issue is making GetMaxWidth() work with operators that expand
> to arbitrary sizes, while preserving aspect ratio.  (Really we need a
> different layout algorithm, but I don't have a suitable algorithm in mind, so
> let's not try to do that now.)
> 

Yes, actually I changed my mind meanwhile and I agree with you for the default value. Anyway if in order to display large operators we are going to use font size rather than stretching (as TeX & MathML3 Operator Dictionary do) then we do not really need to preserve aspect ratio in that case.

> * When only stretched as a largeop, increase the font size by a factor.
>   Would a factor of 2 be suitable?  Or less maybe.  I haven't looked at
>   textbooks for fonts to see what they do.

I guess it's worth asking on mozilla.dev.tech.mathml what people expect. I'm still not really sure about what TeX do for integral symbols. Also, the factor may depend on whether the formula is in displaystyle="true" or not.

>   This would preserve aspect ratio.  I think font size may work better with
>   measuring and font hinting than a transformation, but if I'm wrong and a
>   transformation works just as well then that's fine too.

Is the transformation matrix changed before the curves composing the glyph are drawn? Or is the glyph drawn first and then the scale applied? I believe the former happens and in this case the glyph is rendered with the same quality as if we increase font size. At the moment I can't really imagine what would be best for measuring. In a first time, I'll start using what I've already made for scaling and we'll see later if we should rather use font size.
(In reply to comment #30)
> > * When only stretched as a largeop, increase the font size by a factor.
> >   Would a factor of 2 be suitable?  Or less maybe.  I haven't looked at
> >   textbooks for fonts to see what they do.
> 
> I guess it's worth asking on mozilla.dev.tech.mathml what people expect.

These were discussed a bit in this thread, where Alvaro suggested "twice as
big".  I'm not sure if that was height or area, or whether it is even meant to
be precise.  It probably even depends on the operator, so the value will
probably be a compromise.

http://groups.google.com/group/mozilla.dev.tech.mathml/browse_frm/thread/39a3ac4e6bc34e50#

No harm in asking more specifically in the newsgroup if you like.

> I'm still not really sure about what TeX do for integral symbols.

There will be different preferences from different people here.

Check out bug 363240 comment 21 and subsequent.

It sounds like the MathML WG did some research and decided to suggest all
largeop operators default non-stretchy.
http://lists.w3.org/Archives/Public/www-math/2009Sep/0015.html

I would follow that suggestion, as eventually people will probably refer to
the upcoming suggested operator dictionary and realize that they should
explicitly specify stretchy="true" if they want that behavior.

>                                                   Also, the factor
> may depend on whether the formula is in displaystyle="true" or not.

Largeop should have no effect when displaystyle="false".
http://www.w3.org/TR/MathML3/chapter3.html#id.3.2.5.2

> 
> >   This would preserve aspect ratio.  I think font size may work better with
> >   measuring and font hinting than a transformation, but if I'm wrong and a
> >   transformation works just as well then that's fine too.
> 
> Is the transformation matrix changed before the curves composing the glyph are
> drawn? Or is the glyph drawn first and then the scale applied? I believe the
> former happens and in this case the glyph is rendered with the same quality as
> if we increase font size.

Yes, I think you are right.

> At the moment I can't really imagine what would be best for measuring.

Getting the bounding metrics is different: the glyph is first drawn and
measured, then the scale is applied:

http://hg.mozilla.org/mozilla-central/annotate/8a350f5584d4/gfx/thebes/src/gfxFont.cpp#l1089

But the bounds probably do not need to be that accurate.

> In a first time, I'll start using what I've already made
> for scaling and we'll see later if we should rather use font size.

The transformation will probably be fine.
Depends on: 519126
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Depends on: nsStretchDirection
No longer depends on: 519126
Attached patch patch (5th version) (obsolete) — Splinter Review
not many changes... just an update to be compatible with the patches from the bugs this depends on.
Attachment #399689 - Attachment is obsolete: true
Attachment #400544 - Attachment is obsolete: true
Attachment #400587 - Attachment is obsolete: true
Attachment #400736 - Attachment is obsolete: true
Attachment #401745 - Attachment is obsolete: true
Attachment #401746 - Attachment is obsolete: true
Attached file Testcase largeop (obsolete) —
Largeops with displaystyle="true"
Attachment #399259 - Attachment is obsolete: true
It is looking very impressive Frédéric. I, for one, appreciate the work you are doing here. Keep it up. :)
Attached patch patch (6th version) (obsolete) — Splinter Review
Attachment #422713 - Attachment is obsolete: true
Attached file Testcase largeop
The largeop testcase was not correct. It seems that the union was considered as "infix" (so not a largeop according to our dictionary). I added an mrow around the right hand side of the equality and the union is now displayed larger.
Attachment #422714 - Attachment is obsolete: true
Attached file Testcase integral
Attached file Testcase root
Attached image Screenshot largeop (obsolete) —
Attached image Screenshot integral (obsolete) —
Attached image Screenshot root (obsolete) —
Attached patch patch (7th version) (obsolete) — Splinter Review
Attachment #423261 - Attachment is obsolete: true
The current version of the patch is able to apply the scale transform to any kind of nsMathMLChar (drawn with a "normal" char, a single glyph or a composite char). This improves size matching since we can first try to find the nearest size and then apply scaling to correct the error (see below the case of the roots). Also, it will be possible to add new transforms such that negative scaling (used to mirror operators in bug 208309) or rotation (for the diagonal arrows). BTW, I've made some tests to apply rotation but I think we should postpone this feature and open a separate bug for that (anyway, it will really be useful/testable only when bug is 236963 fixed). I think we should concentrate on the "refresh" bug I mention in comment 13 and below.

Largeops now have the behaviour described in comment 29. The increase factor is currently 2 (maybe it's too large?) and only apply when displaystyle=true (maybe we should also apply a factor when displaystyle=false?). The default behaviour is now changed to stretchy=false for largeop (as discussed previously) but it is possible for the user to modify it (see testcase "integral").
	
Without the patch, root symbols were drawn either too small (no STIX font) or too large (apparently, like TeX does in the MathML torture test). Scaling enables to correct the error. Note however a bug with the mroot when another frame overlap the sqrt char. I suspect to be an instance of the "refresh" bug.
	
As you might have understand, my main problem is this "refresh" bug: the operator is drawn again without the transforms applied. I first thought the member of nsMathMLChar that describes the transform parameter was reinitialized but it seems to come from elsewhere. I'm not able to explain it, but what I've noticed so far:
- it happens more often when STIX fonts are disabled
- it can appear / be fixed when you select/drag the char with your mouse.
- it can be fixed when you scroll the window with up/down keys (push the char outside and move back).
- it can appear when you scroll the window with page up/down keys.
- sometimes a part of the char is repaint correctly
	
Some examples: in the last test of attachment 422715 [details], the overbrace covered the b's but became smaller when I took the screenshot. In general, the torture test can be affected by the bug when you scroll with page up/down keys. See also this page with many sum symbols: http://www.cs.au.dk/~fwang/riemannian_geometry/exercise_4_12.xhtml.
Attached file bug parenthesis
Attached image screenshot bug parenthesis (obsolete) —
I've isolated a case where the bug happens with a parenthesis (attachment 423646 [details]). Screenshot is attachment 423647 [details]. From left to right:
- with STIX fonts
- without STIX fonts, obtained by selecting the line of the "x0)" with the mouse.
- without STIX fonts, just after the page is loaded.
(In reply to comment #48)

Also, using printf in the function of nsMathMLChar that displays the char shows that the mScaleY value does not change and consequently I think the parenthesis is displayed elsewhere.
Blocks: 208309
I've just tried to print the test pages and the scale correction is always well-applied in the ps files. So I still think there is something in the refresh system that produces the bug I describes in comment 45 (BTW, I've also noticed moving another window in front of the char and then moving it out can also change the scale), i.e. that redisplay the operator without the scale. However I've no idea about how the bug precisely comes from. I'm curious to know whether other people can see this bug?
Maybe something that might be helpful is to add some logging to the appropriate nsThebesRenderingContext::DrawString to log the current transformations (and maybe the string also) to check that this gets called at the expected times and that the expected transformations are on the context.
(In reply to comment #51)
> Maybe something that might be helpful is to add some logging to the appropriate
> nsThebesRenderingContext::DrawString to log the current transformations (and
> maybe the string also) to check that this gets called at the expected times and
> that the expected transformations are on the context.

I've added some logging and tested attachment 423646 [details]. When the function is called with ')', the transformation is a scale along the Y-axis as expected. I've tested both nsThebesRenderingContext::GetCurrentTransform and nsThebesRenderingContext::mThebes->CurrentMatrix() and got the same result.
What platform are you testing on, Frédéric?
There's chance that the wrong glyph is being pulled from a cache in cairo or similar.  Do you see sometimes the wrong glyph changing to the right glyph and sometimes right glyph to wrong glyph?
(In reply to comment #53)
> What platform are you testing on, Frédéric?

Hardware: x86
OS: Linux

> There's chance that the wrong glyph is being pulled from a cache in cairo or
> similar.  Do you see sometimes the wrong glyph changing to the right glyph and
> sometimes right glyph to wrong glyph?

I'm not sure to understand what you mean by right/wrong glyph, but yes I see
the wrong size changing to right size and conversely. For the attachment 423646 [details]
with font.mathfont-family empty, I have:

- by default the parenthesis is not scaled.
- moving another window over the parenthesis and then moving this window out,
  the parenthesis is scaled correctly.
- focusing the browser's window makes the parenthesis come back to its 
  non-scaled size.
- and I can repeat the operation indefinitely
I've just tested with a recent build again and I can no longer produce the "refresh" bug. I don't know if some changes on the trunk have fixed it or if it's because I've upgraded some system libraries...

I think the patch will be ready for a first review once bug 524275 is fixed. Given the fact that it gives many improvements, I would appreciate if it is integrated in Firefox 3.7. However, I think some tests would also probably needed since it makes many changes to the rendering of operators (for example some problems could arise from our operator dictionary, we should fix bug 534970 and complete carefully the entries)... I wonder if there is a way to make binaries available, so that people can provide feedback.
(In reply to comment #55)
> 534970 and complete carefully the entries)... I wonder if there is a way to
> make binaries available, so that people can provide feedback.

You could do tryserver builds.

I do Windows and Linux builds that include the patches here and from bug 542275 (among other pending MathML and SVG patches) more or less daily, which I make available at http://www.wg9s.com/mozilla/firefox/
(In reply to comment #55)
> I've just tested with a recent build again and I can no longer produce the
> "refresh" bug.

Might be bug 542605.

> I wonder if there is a way to
> make binaries available, so that people can provide feedback.

Yes, tryserver: https://wiki.mozilla.org/Build:TryServer

I recommend "push to try", which requires an hg account with "Level 1" commit access.  I can vouch for you for that or for "Level 2" if you like.
Depends on: 542605
Depends on: 562347
Thanks! I've opened bug 562347.
Attached patch patch (8th version) (obsolete) — Splinter Review
This new version adds only minor changes with respect to the previous patch.
The modifications to the operator dictionary have been removed and are now available in a separate patch: attachment 442458 [details] [diff] [review].
Attachment #423544 - Attachment is obsolete: true
Attached patch Patch (9th version) (obsolete) — Splinter Review
I've replaced the largeop factor: sqrt(2) in place of 2.
Attachment #442464 - Attachment is obsolete: true
Depends on: LookupOperator
Alias: scale-stretchy
Blocks: 464592
For the MathML browser testing site at
https://eyeasme.com/Joe/MathML/MathML_browser_test
the operators were manually set for a larger size.

A copy of this site with without operators being set larger is at
https://eyeasme.com/Joe/MathML/MathML_browser_test_next

The image is from this website.
The first formula in a row is from TeX, the second from Firefox 3.5
and the last for the latest trunk release.

Using the latest trunk build:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.3a5pre) Gecko/20100513 Minefield/3.7a5pre

The operators scale well for large size fonts, but not small font sizes:
The font size is 24pt for "Test 1" and it looks great
The font size is 12pt for "Sophmore's Dream" and looks OK
The font size is 8pt for "Sphere Volume" but the integrals look bad.

Perhaps the algorithm for scaling should take the size of the font into account.
Forgot to mention that the second formula shows the old manual setting of the operators to a larger size using FF 3.5 and the last formula in a row uses the new default operator scaling.
(In reply to comment #64)
> https://build.mozilla.org/tryserver-builds/fred.w...@free.fr-try-08c872a8d318/

https://build.mozilla.org/tryserver-builds/fred.wang@free.fr-try-08c872a8d318/ 

(In reply to comment #63)
> Forgot to mention that the second formula shows the old manual setting of the
> operators to a larger size using FF 3.5 and the last formula in a row uses the
> new default operator scaling.

Thank you for your comment, Joe. I don't think the issue with the size of largeop is related to the font size. The scale is relative to the base size, so it is automatically adapted. Rather, I guess the problem is that integrals are normally drawn larger than other largeops:

http://en.wikipedia.org/wiki/Sum_rule_in_integration

Hence I suggest adding a property "integral" in our operator dictionary and use a factor of 2 instead of sqrt(2) for largeop with that property.
Attached file Bug tilde (obsolete) —
I've also found a new bug with tilde. I suspect to be related to the negative bearings:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmoverFrame.cpp#362
Attached image OverBar touches overscript (obsolete) —
Using
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.3a5pre) Gecko/20100517 Minefield/3.7a5pre
on
https://eyeasme.com/Joe/MathML/MathML_browser_test_next
using STIX Beta font 
notice that the overbar touches overscript in the De Morgan's law test.
Attached image strange gaps in uparrow (obsolete) —
Same setup as in Comment #67.
The uparrow gets stretched but "Gaps" occur.
Joe, IIUC the issues in comments 67 and 68 are not related to Fred's build or this bug, but are a regression from some other change that has been made since Firefox 3.5.
They deserve a separate bug report for diagnosis of the change that caused the problem.
Karl, thank you, you are correct these regressions do NOT seem to be because of this bug.  
I have filed bug 567090 for bug found in Comment #67 and bug 567091 for the bug in Comment #68
Attached patch Patch v. 10 (obsolete) — Splinter Review
Attachment #422715 - Attachment is obsolete: true
Attachment #423540 - Attachment is obsolete: true
Attachment #423541 - Attachment is obsolete: true
Attachment #423542 - Attachment is obsolete: true
Attachment #423647 - Attachment is obsolete: true
Attachment #442724 - Attachment is obsolete: true
Attachment #445151 - Attachment is obsolete: true
Attachment #445772 - Attachment is obsolete: true
Attachment #445800 - Attachment is obsolete: true
Attachment #445803 - Attachment is obsolete: true
(In reply to comment #66)
> Created an attachment (id=445772) [details]
> Bug tilde
> 
> I've also found a new bug with tilde. I suspect to be related to the negative
> bearings:
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmoverFrame.cpp#362

This is not related to the patch of this bug either. I've reported bug 567718.
Attached patch Patch v. 11 (obsolete) — Splinter Review
just fixing an ASSERT... No error found on the TryServer, but I don't know why no build is produced for Mac.

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/fred.wang@free.fr-2c8ac775ab39
Attachment #447042 - Attachment is obsolete: true
Comment on attachment 447147 [details] [diff] [review]
Patch v. 11

There does not seem to be much protest against the TryServer builds, so let's ask for a first review...
Attachment #447147 - Flags: review?(karlt)
Attached image Triple integral looks strange (obsolete) —
The latest TryServer build (24-May-2010) looks good on a Windows Vista machine.
I tried it on
https://www.eyeasme.com/Joe/MathML/MathML_browser_test_next
and it looks good, except for the triple integral in the sphere volume derivation.
Single integrals look good in the sphere volume derivation.
> except for the triple integral in the sphere volume derivation.

It seems to be a remaining instance of the "refresh" bug. Adding support for the STIXIntegrals* fonts fixes the issue for me.
(In reply to comment #76)

> It seems to be a remaining instance of the "refresh" bug. Adding support for
> the STIXIntegrals* fonts fixes the issue for me.

OK, does this mean that the default settings for the "font.mathfont-family" preference will need to be changed for this bug to be fixed?  I am assuming that is what you meant by "Adding support for the STIXIntegrals* fonts".
Depends on: 569195
> OK, does this mean that the default settings for the "font.mathfont-family"
> preference will need to be changed for this bug to be fixed?  I am assuming
> that is what you meant by "Adding support for the STIXIntegrals* fonts".

Yes, but it also requires a bit more of work, not configurable by the user. See bug 569195.
Comment on attachment 447147 [details] [diff] [review]
Patch v. 11

It looks like leaving the code to set mDirection to NS_STRETCH_DIRECTION_UNSUPPORTED in nsMathMLChar::Stretch() after a stretch
http://hg.mozilla.org/mozilla-central/annotate/5b3604a3cfbe/layout/mathml/nsMathMLChar.cpp#l1745

may confuse some callers.
e.g.
http://hg.mozilla.org/mozilla-central/annotate/5b3604a3cfbe/layout/mathml/nsMathMLmoFrame.cpp#l793
http://hg.mozilla.org/mozilla-central/annotate/5b3604a3cfbe/layout/mathml/nsMathMLmfencedFrame.cpp#l499

>-  // quick return if there is nothing special about this char
>-  if (!mGlyphTable ||
>-      (aStretchDirection != direction &&
>-       aStretchDirection != NS_STRETCH_DIRECTION_DEFAULT) ||
>-      (aStretchHint & ~NS_STRETCH_MAXWIDTH) == NS_STRETCH_NONE) {
>-    return NS_OK;
>-  }
>-
>-  // if no specified direction, attempt to stretch in our preferred direction
>-  if (aStretchDirection == NS_STRETCH_DIRECTION_DEFAULT) {
>-    aStretchDirection = direction;
>-  }
>-
>   // see if this is a particular largeop or largeopOnly request
>   PRBool largeop = (NS_STRETCH_LARGEOP & aStretchHint) != 0;
>   PRBool stretchy = (NS_STRETCH_VARIABLE_MASK & aStretchHint) != 0;
>   PRBool largeopOnly = largeop && !stretchy;
> 
>   PRBool isVertical = (direction == NS_STRETCH_DIRECTION_VERTICAL);
> 
>   nscoord targetSize =
>     isVertical ? aContainerSize.ascent + aContainerSize.descent
>     : aContainerSize.rightBearing - aContainerSize.leftBearing;
> 
>+  mScaleY = mScaleX = 1.0;
>+  mTrueAscent = aDesiredStretchSize.ascent;
>+
>+  // quick return if there is nothing special about this char
>+  if ((aStretchDirection != direction &&
>+       aStretchDirection != NS_STRETCH_DIRECTION_DEFAULT) ||
>+      (aStretchHint & ~NS_STRETCH_MAXWIDTH) == NS_STRETCH_NONE) {
>+    return NS_OK;
>+  }
>+
>+  // if no specified direction, attempt to stretch in our preferred direction
>+  if (aStretchDirection == NS_STRETCH_DIRECTION_DEFAULT) {
>+    aStretchDirection = direction;
>+  }

Was there a reason for changing the order here?
(The old order made more sense to me.)

>+  // ensure a minimum targetSize in the case of a largeop in display mode
>+  if (largeop && (targetSize < largeopFactor * charSize)) {
>+    targetSize = largeopFactor * charSize;
>+  }
>+

I think it makes sense to have target largeop factor when we don't have font
support for the large variants.  However, these can only be heuristics to
guess what will be suitable.

As quirky as our "choose the largest size less than or equal to 2"
implementation is, it does provide a mechanism for the font to choose the
appropriate size and aspect-ratio changes, according to the particular
character.  If we have font support for a largeop, then I don't think we
should apply our own general heuristics as well.

>+      scale = float(largeopFactor *
>+                    (initialSize.ascent + initialSize.descent)) /
>+        (aDesiredStretchSize.ascent + aDesiredStretchSize.descent);

float() is not necessary here because largeopFactor is float, so
(initialSize.ascent + initialSize.descent) gets converted to float for the '*'
operator.

>+  // set the origin of the coordinate system to the center of the rectangle,
>+  // before applying any transform.
>+  aRenderingContext.Translate(r.x + dx, r.y + dy);
>+
>+  // scale the rectangle.
>+  aRenderingContext.Scale(mScaleX, mScaleY);
>+
>+  // set the origin of the coordinate system to the top left corner of the
>+  // bounding rectangle.
>+  dx /= mScaleX; dy /= mScaleY;
>+  aRenderingContext.Translate(-dx, -dy);

It would be simpler to just have a single Translate(r.x, r.y) followed by the
Scale(mScaleX, mScaleY).  Would that work fine?

>-    aRenderingContext.DrawString(mData.get(), len, mRect.x + aPt.x,
>-                                 mRect.y + aPt.y + mBoundingMetrics.ascent);
>+    aRenderingContext.PushState();
>+    nsRect r = mRect + aPt;
>+    ApplyTransforms(aRenderingContext, r);
>+    aRenderingContext.DrawString(mData.get(), len, 0, mTrueAscent);
>+    aRenderingContext.PopState();

Can a single PushState(); nsRect r = mRect + aPt;
ApplyTransforms(aRenderingContext, r); sequence be used around this set of
conditionals (instead of having the code repeated)?

>+  // mTrueAscent is the actual ascent of the char, i.e. not taking into account
>+  // the transform applied.
>+  nscoord            mTrueAscent;

What do you think about calling this "mUnscaledAscent"?

>-    if (NS_MATHML_IS_DISPLAYSTYLE(aPresentationData.flags) &&
>-        NS_MATHML_OPERATOR_IS_LARGEOP(aFlags)) {
>+    if (NS_MATHML_OPERATOR_IS_INTEGRAL(aFlags) ||
>+        (NS_MATHML_IS_DISPLAYSTYLE(aPresentationData.flags) &&
>+         NS_MATHML_OPERATOR_IS_LARGEOP(aFlags))) {
>       stretchHint = NS_STRETCH_LARGEOP; // (largeopOnly, not mask!)

This doesn't seem quite right to me.  I thought integrals would keep their
normal size when displaystyle="false".  Similarly, I would expect them to keep
their normal size when the author specifies largeop="false".
(In reply to comment #79)
> (From update of attachment 447147 [details] [diff] [review])
> It looks like leaving the code to set mDirection to
> NS_STRETCH_DIRECTION_UNSUPPORTED in nsMathMLChar::Stretch() after a stretch
> http://hg.mozilla.org/mozilla-central/annotate/5b3604a3cfbe/layout/mathml/nsMathMLChar.cpp#l1745
> 
> may confuse some callers.
> e.g.
> http://hg.mozilla.org/mozilla-central/annotate/5b3604a3cfbe/layout/mathml/nsMathMLmoFrame.cpp#l793
> http://hg.mozilla.org/mozilla-central/annotate/5b3604a3cfbe/layout/mathml/nsMathMLmfencedFrame.cpp#l499

Yes. Anyway, it's weird to keep direction = NS_STRETCH_DIRECTION_DEFAULT when we do actually stretch with the scaling method. I think I'm going to use a member mDrawNormal instead.

> I think it makes sense to have target largeop factor when we don't have font
> support for the large variants.  However, these can only be heuristics to
> guess what will be suitable.
> 
> As quirky as our "choose the largest size less than or equal to 2"
> implementation is, it does provide a mechanism for the font to choose the
> appropriate size and aspect-ratio changes, according to the particular
> character.  If we have font support for a largeop, then I don't think we
> should apply our own general heuristics as well.
 
I believe you're right. At least for the integral: it renders better with the STIX variants alone rather than the STIX variant + scale (because the aspect-ratio of the base char is used).

> It would be simpler to just have a single Translate(r.x, r.y) followed by the
> Scale(mScaleX, mScaleY).  Would that work fine?

I think the composition of transforms would be the same. The reason why I did that is to prepare other transforms which are best understood when having the origin at the center of the bounding box (mirroring and rotation). See for example the patch of bug 208309.

> Can a single PushState(); nsRect r = mRect + aPt;
> ApplyTransforms(aRenderingContext, r); sequence be used around this set of
> conditionals (instead of having the code repeated)?

I'm not sure whether they are all using the same nsRect.

> What do you think about calling this "mUnscaledAscent"?

Yes, it's fine (except if we add a rotation later).

> This doesn't seem quite right to me.  I thought integrals would keep their
> normal size when displaystyle="false".  Similarly, I would expect them to keep
> their normal size when the author specifies largeop="false".

As I said elsewhere, I think what I do is wrong. My intention was that integrals are drawn larger even when displaystyle="false". But I'm not sure.
Attached patch Patch v12 (obsolete) — Splinter Review
Here is an updated version.

However, the integrals are drawn too small on Joe's test, when displaystyle=false:

Sphere volume
https://eyeasme.com/Joe/MathML/MathML_browser_test_next
(In reply to comment #81)
> However, the integrals are drawn too small on Joe's test, when
> displaystyle=false:
> 
> Sphere volume
> https://eyeasme.com/Joe/MathML/MathML_browser_test_next

However it looks ok in his HTML5 version of the test:

https://eyeasme.com/Joe/MathML/HTML5_MathML_browser_test.html
I know why.

In the HTML5 version 
https://eyeasme.com/Joe/MathML/HTML5_MathML_browser_test.html
the operators that have limits (Sums, Products, Integrals) have their 
font sizes set larger.

While in this (XHTML 1.1 plus MathML 2.0) version
https://eyeasme.com/Joe/MathML/MathML_browser_test_next
no operators are set to larger sizes

On of the goals of this bug is to automatically produce
aesthetically pleasing equations.  Operators with limits
generally look better when they are larger than the surrounding 
characters.  When this bug is fixed, the browser will have some
heuristics on when and how much to increase the size of some 
operators.
(In reply to comment #83)
> On of the goals of this bug is to automatically produce
> aesthetically pleasing equations.  Operators with limits
> generally look better when they are larger than the surrounding 
> characters.  When this bug is fixed, the browser will have some
> heuristics on when and how much to increase the size of some 
> operators.

The spec says:

"Specifies whether the operator is considered a largeop operator, that is, whether it should be drawn larger than normal when displaystyle="true" (similar to using TEX's \displaystyle). Examples of large operators include &int;  and &prod;."

"TEX's \displaystyle, \textstyle, \scriptstyle, and \scriptscriptstyle correspond to displaystyle and scriptlevel  as "true" and "0", "false" and "0", "false" and "1", and "false" and "2", respectively. Thus, math's display="block" corresponds to \displaystyle, while display="inline" corresponds to \textstyle."

In the "sphere volume" test, displaystyle=false so the largeops need not be drawn larger. However I was wondering if there should be a special case for integrals? What is TeX doing?

The factors I was considering to use as well as the start size:

-----------------------------------------------
| largeop | integral | width | height |  size |
|----------------------------------------------
|   no    |   no     |   1   |   1    |   0   |
|----------------------------------------------
|  yes    |   no     | sqrt2 | sqrt2  |   2   |
-----------------------------------------------
|  no     |   yes    |   1   | sqrt2  |   1   |
-----------------------------------------------
|  yes    |   yes    | sqrt2 |    2   |   2   |
-----------------------------------------------
(In reply to comment #84)

Your ideas below look good.

> The factors I was considering to use as well as the start size:
> 
> -----------------------------------------------
> | largeop | integral | width | height |  size |
> |----------------------------------------------
> |   no    |   no     |   1   |   1    |   0   |
> |----------------------------------------------
> |  yes    |   no     | sqrt2 | sqrt2  |   2   |
> -----------------------------------------------
> |  no     |   yes    |   1   | sqrt2  |   1   |
> -----------------------------------------------
> |  yes    |   yes    | sqrt2 |    2   |   2   |
> -----------------------------------------------


I think that the "Sphere Volume" derivation should be in mode="display".

I have altered 
https://www.eyeasme.com/Joe/MathML/MathML_browser_test_next
to show this.  

The triple integral 
 <mo>&#x222D;<!-- &iiint; --></mo>
does not get set to the correct size, I have posted an image showing this.

TEX is the gold standard for mathematics representation.
The problem I have with default TEX is that it crams the lines too close (see the TEX rendering of the "Sphere Volume" derivation).  
FireFox MathML "display" mode does the opposite and spreads the lines too 
far apart. 

I am using the latest Windows trunk build, with the final STIX fonts installed
after the STIX Beta fonts (Beta fonts NOT uninstalled).
Here is the image to go along with Comment #85.
> | largeop | integral | width | height |  size |

I meant display mode for the first column.

> I think that the "Sphere Volume" derivation should be in mode="display".

In your TeX source, you are using begin{align*} which I guess enables the display mode. So I think we don't need a special treatment for integrals in displaystyle=false i.e. what the patch currently does is correct.

> TEX is the gold standard for mathematics representation.
> The problem I have with default TEX is that it crams the lines too close (see
> the TEX rendering of the "Sphere Volume" derivation).  
> FireFox MathML "display" mode does the opposite and spreads the lines too 
> far apart. 

I'm not sure what is the "right" spacing... It is a separate issue anyway.

> The triple integral 
>  <mo>&#x222D;<!-- &iiint; --></mo>
> does not get set to the correct size, I have posted an image showing this.
> I am using the latest Windows trunk build, with the final STIX fonts installed
> after the STIX Beta fonts (Beta fonts NOT uninstalled).

Are you talking about a trunk build or Bill Gianopoulos's build? Normally, with the current set of patches (including those of bug 534970) this issue is fixed as said in comment 76.
This image shows what I see on my machine.  As far as this bug is concerned things look OK.  I can see no more changes needed for this bug. 

Thank you! for the job well done.
Comment on attachment 450446 [details] [diff] [review]
Patch v12

For the remaining issues:

> It would be simpler to just have a single Translate(r.x, r.y) followed by the
> Scale(mScaleX, mScaleY).  Would that work fine?

Do you really want I do this change, given that I'll probably change back in subsequent work s.t. bug 208309?

> Can a single PushState(); nsRect r = mRect + aPt;
> ApplyTransforms(aRenderingContext, r); sequence be used around this set of
> conditionals (instead of having the code repeated)?

I think if I replace the return statement by a else, I can factor out PushState(); and PopState().
For the rectangle, most blocks are using mRect + aPt, but one is nsRect(aPt.x, aPt.y, mRect.width, mRect.height) so I'm not sure it would be relevant to factor out the other things.
Attachment #450446 - Flags: review?(karlt)
Attachment #448289 - Attachment is obsolete: true
Attachment #450805 - Attachment is obsolete: true
Attachment #447147 - Attachment is obsolete: true
Attachment #447147 - Flags: review?(karlt)
Comment on attachment 450446 [details] [diff] [review]
Patch v12

>   NS_MATHML_OPERATOR_MOVABLELIMITS      = 1<<9,
>   NS_MATHML_OPERATOR_SYMMETRIC          = 1<<10,
>+  NS_MATHML_OPERATOR_INTEGRAL           = 1<<11,
> 
>   // Additional bits not stored in the dictionary
>   NS_MATHML_OPERATOR_MINSIZE_ABSOLUTE   = 1<<11,
>   NS_MATHML_OPERATOR_MAXSIZE_ABSOLUTE   = 1<<12,
>   NS_MATHML_OPERATOR_LEFTSPACE_ATTR     = 1<<13,
>   NS_MATHML_OPERATOR_RIGHTSPACE_ATTR    = 1<<14
> };

I forgot to increase the shifts here.
I think the ApplyTransforms should be static and used in nsDisplayMathMLSelectionRect, nsDisplayMathMLCharBackground, nsDisplayMathMLCharDebug classes.
(In reply to comment #80)
> (In reply to comment #79)
> > It would be simpler to just have a single Translate(r.x, r.y) followed by the
> > Scale(mScaleX, mScaleY).  Would that work fine?
> 
> I think the composition of transforms would be the same. The reason why I did
> that is to prepare other transforms which are best understood when having the
> origin at the center of the bounding box (mirroring and rotation). See for
> example the patch of bug 208309.

(In reply to comment #89)
> Do you really want I do this change, given that I'll probably change back in
> subsequent work s.t. bug 208309?

For bug 208309, the mirroring can be done with a single Translate to the
right hand edge, followed by the Scale.

I think I prefer this approach.  There are fewer operations and
it avoids (minor) rounding issues with halving integer coords.

Things will get much more complicated with rotations, but I'm not sure that
translating to the middle is going to solve all the issues there, so I'd
prefer to leave that at this stage.

(In reply to comment #89)
> > Can a single PushState(); nsRect r = mRect + aPt;
> > ApplyTransforms(aRenderingContext, r); sequence be used around this set of
> > conditionals (instead of having the code repeated)?
> 
> I think if I replace the return statement by a else, I can factor out
> PushState(); and PopState().
> For the rectangle, most blocks are using mRect + aPt, but one is nsRect(aPt.x,
> aPt.y, mRect.width, mRect.height) so I'm not sure it would be relevant to
> factor out the other things.

That odd block is for composite chars which we don't use.

I'd be very happy if you'd like to remove the composite char code.

If you'd like to keep it, I don't think it can be working correctly as it is
now even, because it seems to be ignoring mRect.x/y as set on the parent.  It
seems that it should be passing aPt + mRect.TopLeft(), which would make it
consistent with the other blocks.

(In reply to comment #91)
> I think the ApplyTransforms should be static and used in
> nsDisplayMathMLSelectionRect, nsDisplayMathMLCharBackground,
> nsDisplayMathMLCharDebug classes.

This isn't necessary AFAIK.  I think these classes should just display as if
it were an unscaled regular variant or built-from-parts char.
Comment on attachment 450446 [details] [diff] [review]
Patch v12

If a largeop is stretchy and a variant is found, it looks like this patch will
squish the operator down to the requested size.

Should largeops be only grown (not shrunk) in size, perhaps?

We seem to have a problem with stretchy largeops not being large even without
this patch.  It looks like nsMathMLChar::StretchEnumContext::TryVariants is
not forcing use of the larger variant unless largeoponly.
(In reply to comment #93)
> (From update of attachment 450446 [details] [diff] [review])
> If a largeop is stretchy and a variant is found, it looks like this patch will
> squish the operator down to the requested size.
> 
> Should largeops be only grown (not shrunk) in size, perhaps?
> 
> We seem to have a problem with stretchy largeops not being large even without
> this patch.  It looks like nsMathMLChar::StretchEnumContext::TryVariants is
> not forcing use of the larger variant unless largeoponly.

The standard properties regarding large operators become "largeop" but not "stretchy" after bug 534970. The "largeop"+"stretchy" case is only possible if user explicitly requests it.

I propose the following behavior:
- largeopOnly: use size=1. A scale increasing by a factor SQRT(2) is used when this variant does not exist.
- largeop: start with size=0 and use the flag asking to stretch greater, which is used for the square root sign. Apply the scale only when it increases the size (so when the stretch fails).

Currently for largeops, size=2 is used when it exists. For Asana Math, this variant seems to be too large compared to STIX font or to the SQRT(2) heuristic.
(In reply to comment #94)
> I propose the following behavior:
> - largeopOnly: use size=1. A scale increasing by a factor SQRT(2) is used when
> this variant does not exist.

That sounds fine with me.

> - largeop: start with size=0 and use the flag asking to stretch greater, which
> is used for the square root sign.

I don't feel so good about this.  Consistency with largeopOnly makes more sense to me here.  Also NS_STRETCH_LARGER is not strictly greater but can be equal, I think.  Using NS_STRETCH_LARGER may make sense though.

Both of these are independent of this bug though.

> Apply the scale only when it increases the
> size (so when the stretch fails).

This part I think should be included in the patch here.
Not sure if this is part of this bug, but in
http://www.w3.org/Math/testsuite/mml2-testsuite/index.html
Presentation -> TablesAndMatrices -> mtable 
test mtableBsize2
fails.  The arrows fail to automatically grow to fill the 
space allowed.

Should I file a new bug to cover this?

I have altered 
https://www.eyeasme.com/Joe/MathML/MathML_browser_test_next
"Cichoń's Diagram" to depend on this bug being fixed.
(In reply to comment #96)
I'm going to answer in bug 557086.
The "refresh" bug observed here is most likely bug 518172.
Depends on: 518172
(In reply to comment #98)
> The "refresh" bug observed here is most likely bug 518172.

I think there are actually two "refresh" bugs. The first happened only on Linux but has been fixed by the upgrade of Cairo. It remains some problems with radical symbols and characters drawn with the "normal drawing" (using the terminology of the source code). These remaining problems happen on other platforms. For instance Joe saw it for the triple integral (which disappeared when we use the glyph from STIX fonts).
Attached patch Patch v13Splinter Review
Just an update to address the comments above. The patch is untested.
Attachment #450446 - Attachment is obsolete: true
Attachment #450446 - Flags: review?(karlt)
Comment on attachment 461929 [details] [diff] [review]
Patch v13

The remaining "refresh" bug is gone after the resolution of bug 518172.
Attachment #461929 - Flags: review?(karlt)
Comment on attachment 461929 [details] [diff] [review]
Patch v13

Thanks very much!
Attachment #461929 - Flags: review?(karlt) → review+
This bug depends on bug 534970 (as well as bug 524275), right?

Is anything else needed?
(In reply to comment #103)
> This bug depends on bug 534970 (as well as bug 524275), right?
> 
> Is anything else needed?

Yes, this bug must be pushed together with the 6 patches of bug 534970 and the patch of bug 524275.
Comment on attachment 461929 [details] [diff] [review]
Patch v13

Requesting approval2.0.
This significantly improves the MathML rendering for users without STIX fonts installed (and even improves rendering a little with STIX fonts installed).
Effects are limited to MathML.  Some more details are at http://groups.google.com/group/mozilla.dev.tech.mathml/browse_thread/thread/59d7a24aa44771b1
Attachment #461929 - Flags: approval2.0?
Comment on attachment 461929 [details] [diff] [review]
Patch v13

No tests?
Blocks: 516292
(In reply to comment #106)
> Comment on attachment 461929 [details] [diff] [review]
> Patch v13
> 
> No tests?

If you mean reftests, I think it would be difficult for this bug.
Why would it be difficult? Is it not possible to create an SVG reference image with similar properties to the stretched glyphs here? Or do all the test machines have the STIX font and therefore we can't test this fallback codepath?

Why can't we just ship the STIX font as part of Firefox?
(In reply to comment #108)
> Why would it be difficult? Is it not possible to create an SVG reference image
> with similar properties to the stretched glyphs here? Or do all the test
> machines have the STIX font and therefore we can't test this fallback codepath?
> 
> Why can't we just ship the STIX font as part of Firefox?

Bug 295193 covers supplying STIX fonts.  I'm not sure about Fedora 12 but I don't think any of the test machines have STIX fonts.

The difficulty with reftests and fonts is variations in rendering (which won't be matched by SVG).

Frédéric, maybe a reftest could check that a stretchy operator does not stretch more than a couple of pixels too far?
Perhaps two mathematical expressions with the same absolute position, equivalent but one with a stretchy operator and the other stretchy=false.  The stetchy=false expression has background the same color as foreground and a border of two or three pixels.  The stretchy=true expression has the same foreground color but no (transparent) background.  The reference only has the stretchy=false expression to check that the stretchy=true expression does not extend beyond the border of the stretchy=false expression.

Perhaps a reftest could also check that a stretchy operator does stretch, though this would be a bit harder I guess.  Maybe a visible stretchy operator and the rest of the expression is tall but made invisible by making the foreground color the same as the background.  I think it would need to be a != reftest, which reduces the usefulness, but could test that the stretchy attribute has some effect.
OK, I think I can do the second reftest. But for the first one, I'm not sure to understand why you want the stretchy=true expression to be close to the stretch=false? A priori, stretchy=true can be much more larger than the stretch=false.
For vertical stretching, I'm expecting a stretchy operator to not stretch (more than a few pixels) more than the tallest unstretched character.

I probably didn't make it clear but I imagined the stretchy=false and =true expressions to be in the same test file.  The stretchy=false expression and its background/border would form a block to "black"-out the stretchy=true expression unless it stretched beyond the border.
Attached patch add reftests (obsolete) — Splinter Review
I'm not sure that it is exactly what you expected, but I've done something similar. There are two kinds of superpositions:

  1) the stretchy operator is covered by a red rectangle slightly longer than the expected size (== test)
  2) the stretchy operator is covered by a green rectangle slightly shorter than the expected size (!= test)

I've made these tests for both vertical and horizontal stretching. Moreover, I've written a != test for largeop stretching i.e. to check that a largeop is really drawn larger in display mode.

I've used triple arrows and quadruple integral, for which stretching fails without this patch (whatever the fonts installed).
That looks great, thanks, better than what I had in mind.
The patch seems to be missing the files for scale-stretchy-4/5 though.
Do you have those?
Attached patch add reftestsSplinter Review
oops, sorry.
Attachment #464827 - Attachment is obsolete: true
Attachment #465143 - Flags: review?(karlt)
Comment on attachment 465143 [details] [diff] [review]
add reftests

(No approval necessary.)
Attachment #465143 - Flags: review?(karlt) → review+
http://hg.mozilla.org/mozilla-central/rev/ebe66e959255
http://hg.mozilla.org/mozilla-central/rev/853a2178341d
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Not bad, looks like it does on paper :)

Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100819 Minefield/4.0b5pre ID:20100819224140
Blocks: 454155
Depends on: 605605
You need to log in before you can comment on or make changes to this bug.