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?

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.

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.

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)

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.

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.

(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?

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

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

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

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.

I filed Bug 666754 on the issues in comment 3.