Last Comment Bug 584332 - Improve lookup of char variants for large operators
: Improve lookup of char variants for large operators
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Frédéric Wang (:fredw)
:
Mentors:
Depends on:
Blocks: asana-math 518330
  Show dependency treegraph
 
Reported: 2010-08-04 05:38 PDT by Frédéric Wang (:fredw)
Modified: 2011-06-23 14:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (527 bytes, application/xml)
2010-08-25 21:15 PDT, Karl Tomlinson (:karlt)
no flags Details
Always start the lookup of largeop variants at size=1 (5.96 KB, patch)
2010-08-27 06:43 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
apply change of comment 1 (1.13 KB, patch)
2010-09-01 13:17 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review

Description Frédéric Wang (:fredw) 2010-08-04 05:38:50 PDT
I open this bug after the discussion of bug 414277.
IIUC, here is what happens in nsMathMLChar::StretchEnumContext::TryVariants:

1) For largeopOnly, we use size=2 (if it exists) or size=1.
2) For largeop, we start at size=2 (if it exists) or size=1. Then we continue until we find the best size (and then we use it) or reach the end of the list of variants.

For the best size, I suggested to use NS_STRETCH_LARGER but I guess this won't be needed. Apparently, it behaves almost the same way as NS_STRETCH_LARGEOP in IsSizeBetter/IsSizeOK.

For the starting size of largeop, the present code use size=1 for STIX fonts but would use size=2 for Asana Math (bug 407439). It seems that it would be best to always use size=1, which is more consistent with our SQRT(2) heuristic.

> 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.

So what we would like for stretchy largeops is to use the largest variant found, even if we don't have IsSizeOK. But I think this "while" loop is only looking variants in one font. So we have to do something to find the largest variant among all the fonts?
Comment 1 Karl Tomlinson (:karlt) 2010-08-04 15:48:53 PDT
I wonder whether changing the test to the following might work better:

if (largeopOnly || (largeop && !haveBetter)
    IsSizeBetter(...

http://hg.mozilla.org/mozilla-central/annotate/3166fda7d54d/layout/mathml/nsMathMLChar.cpp#l1266

There is appeal to keeping consistent fonts, so I'm not so keen on searching all fonts unnecessarily.
Comment 2 Karl Tomlinson (:karlt) 2010-08-04 15:51:33 PDT
There is actually another issue involved here:

The stretch height is calculated from the base size of characters.
For largeop operators that base size should probably be the same as the largeoponly size.
However, currently we do no stretching to determine the base size.

This affects, for example, the size of parentheses around and expression containing a largeop(only).

Not sure how important that is.
Comment 3 Karl Tomlinson (:karlt) 2010-08-25 21:15:55 PDT
Created attachment 469331 [details]
testcase

There are two issues here:

1. The parentheses are not as large as the integral.
   Not sure that they would look better if they were, but stretching
   rules seem to say that they should be. (comment 2)

2. The stretchy="true" largeop is using a transformation to achieve the
   largeop look, instead of using the large glyph as used for the
   stretchy="false" largeop.  (comment 0)
Comment 4 Frédéric Wang (:fredw) 2010-08-27 06:43:49 PDT
Created attachment 469857 [details] [diff] [review]
Always start the lookup of largeop variants at size=1

With this patch, we now use the starting size=1. For integrals of STIX fonts, I've removed the glyphs of STIXGeneral which are not really larger than the base size.
Comment 5 Frédéric Wang (:fredw) 2010-08-27 06:45:02 PDT
Thanks for the testcase. Actually, the second issue seems less important to me, because largeops are now non-stretchy by default.

> I wonder whether changing the test to the following might work better:

This suggestion seems to fix this second issue in the testcase. However, I think it is only going to check size=1 whereas we also want to check the size>1 for stretchy largeop?

> There is appeal to keeping consistent fonts, so I'm not so keen on searching
> all fonts unnecessarily.

But I believe that we are already checking all the fonts when we are not doing largeopOnly?
Maybe we can do something specific and stop at the first font having glyph variants. I suppose it can be done by adding something just before the last instruction of nsMathMLChar::StretchEnumContext::ResolverCallback. If we have glyph variants, we can choose the largest one and return PR_FALSE, instead of moving to the next font.

http://hg.mozilla.org/mozilla-central/annotate/3166fda7d54d/layout/mathml/nsMathMLChar.cpp#l1479

I think that it would solve the second issue and make a more consistent use of fonts for stretchy characters.
Comment 6 Karl Tomlinson (:karlt) 2010-08-29 16:21:36 PDT
(In reply to comment #5)
> This suggestion seems to fix this second issue in the testcase. However, I
> think it is only going to check size=1 whereas we also want to check the size>1
> for stretchy largeop?

Yes, we also want to check size>1.  It looks like that should happen?

> > There is appeal to keeping consistent fonts, so I'm not so keen on searching
> > all fonts unnecessarily.
> 
> But I believe that we are already checking all the fonts when we are not doing
> largeopOnly?

I guess it depends on what IsSizeOK returns.
It looks like that should return true in the testcase here?

> Maybe we can do something specific and stop at the first font having glyph
> variants. I suppose it can be done by adding something just before the last
> instruction of nsMathMLChar::StretchEnumContext::ResolverCallback. If we have
> glyph variants, we can choose the largest one and return PR_FALSE, instead of
> moving to the next font.

I don't understand choosing the "largest" variant.
Don't we just want the first larger-than-normal variant?
Do you have a different situation in mind?
Comment 7 Frédéric Wang (:fredw) 2010-08-30 13:59:14 PDT
> Yes, we also want to check size>1.  It looks like that should happen?

Sorry, I was thinking that the break line 1297 was made just after having set haveBetter to true. Now, I understand better your proposal and indeed the sizes > 1 are also tried.

> I guess it depends on what IsSizeOK returns.
> It looks like that should return true in the testcase here?
> 
> I don't understand choosing the "largest" variant.
> Don't we just want the first larger-than-normal variant?
> Do you have a different situation in mind?

IIUC, when a large enough variant is available (as in the testcase), IsSizeOK should return true. But when the target size is too large, IsSizeOK will return false ; if we can't build by parts, then we try the other fonts. I don't know if avoiding to pick stretchy chars from different fonts is so important. However, a possible workaround could be to use a variant (if there is one) rather than trying other fonts. In that case, the target size is larger than all the variants, so the natural choice is the "largest" variant.
Comment 8 Karl Tomlinson (:karlt) 2010-08-30 15:12:45 PDT
(In reply to comment #7)
> IIUC, when a large enough variant is available (as in the testcase), IsSizeOK
> should return true. But when the target size is too large, IsSizeOK will return
> false ; if we can't build by parts, then we try the other fonts. I don't know
> if avoiding to pick stretchy chars from different fonts is so important.

This is a situation where searching other fonts could be helpful, so, yes, I don't think avoiding other fonts is important here.  (My comment in comment 1, was to indicate that I wasn't keen on adding fallback to other fonts in additional situations.)
Comment 9 Frédéric Wang (:fredw) 2010-09-01 13:17:49 PDT
Created attachment 471250 [details] [diff] [review]
apply change of comment 1

The patch makes the change proposed in comment 1. For a stretchy largeop when no font has a large enough variant,  it is possible not to make the best choice (consider the case where we test font1 and font2 in that order and the largest variant of font2 is smaller than the largest variant of font1. Then the choice of variant for font1 is discarded when we test font2). But again, I don't think it is really important because it is a degenerated case where we don't have a large enough variant.
Comment 10 Karl Tomlinson (:karlt) 2010-09-02 19:34:22 PDT
Comment on attachment 471250 [details] [diff] [review]
apply change of comment 1

(In reply to comment #9)
>                                              For a stretchy largeop when
> no font has a large enough variant,  it is possible not to make the best choice
> (consider the case where we test font1 and font2 in that order and the largest
> variant of font2 is smaller than the largest variant of font1. Then the choice
> of variant for font1 is discarded when we test font2).

I see what you mean.  Thanks for noticing that.
I thinking probably we should just leave this part for now.

If issue 1 in comment 3 is fixed by say doing an initial stretch with largeoponly, then issue 2 will resolve itself because the target size for the largeop/stretchy stretch will be at least the largeoponly size.
Comment 12 Frédéric Wang (:fredw) 2011-06-23 11:29:36 PDT
I think the patch pushed does not fix the issues of comment 3. It's only a starting point which was needed for the Asana font support.
Comment 13 Karl Tomlinson (:karlt) 2011-06-23 14:36:54 PDT
I filed Bug 666754 on the issues in comment 3.

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