User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008052912 Firefox/3.0 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008052912 Firefox/3.0 The recommended markup for primes (and such) is as an explicit superscript: <msup><mi>x</mi><mo>&#x2032;</mo></msup>. However, the corresponding glyphs in Unicode fonts are already raised and scaled. As a result, such "pre-scripted" glyphs are rendered too small and too high. Reproducible: Always Steps to Reproduce: Point your out-of-the-box, STIXBeta-powered Firefox3 to http://www.w3.org/Math/testsuite/mml2-testsuite/Topics/Primes/primes1.xml and compare the first x-prime to its sample rendering. Actual Results: The prime is too small and too high. Expected Results: The prime should be rendered larger and lower. For further details and discussions, see http://groups.google.com/group/mozilla.dev.tech.mathml/browse_thread/thread/066a97ae5456d41a#

If you agree on this, close #140439.

http://groups.google.com/group/mozilla.dev.tech.mathml/browse_thread/thread/049fed9fd3618f84#

Making this blocking MathJax: https://github.com/mathjax/MathJax/issues/268 I suspect this is another bug that could be better handled by the refactoring suggested in bug 114365.

Mass change: setting priority to 2 for bugs that would be nice fix if Gecko's MathML support is enabled by default in MathJax but that are not in my opinion strictly required or for which a workaround could be written in the MathJax code.

I'll work on this. Do you have any suggestions on how to implement this?

Thanks James. Some comments: - I think bug 140439 should be WONTFIX or DUPLICATE of this one. The recommended MathML markup for primes is to use <msup><mi>f</mi><mo>&#x2032;</mo></msup> so we should focus on this case rather than adding some hacks for <mrow><mi>f</mi><mo>&#x2032;</mo></mrow>. Test 28 of the MathML torture test uses the recommended markup. - The prime glyph in MathJax fonts is already big so we don't need to worry about these fonts. (BTW, MathJax MathML parser has some hacks to always add an msup when it is missing) - In the future we want to target Open Type MATH fonts (see bug 407059 for a list of fonts). The appropriate Open Type feature for glyph variants in sup/sup scripts is "ssty". This table does not seem available in STIX/Asana but you can find them in Cambria Math, Neo Euler, Latin Modern etc Open one of these fonts in fontforge, select the U+2032 glyph, right click and select "glyph info". There is a "variant" section that contains the "ssty" table. For example for Neo Euler the glyphs listed are "minute.ssty1" for the first level and "minute.ssty2" for the second level. (In general, there could be many variants: script, script of script and so on but I guess it would be enough to focus of the first level here). - In STIX-Regular.odt (STIX-Word distribution), there is a "substitutions" section in the glyph info dialog, that refers to the table "ss03" and the glyph "uni2032.var". This is available as a CSS feature in Gecko (see https://developer.mozilla.org/en-US/docs/Web/CSS/font-feature-settings), for example try: <math style="font-family: STIX;"> <msup><mi>x</mi><mo>&#x2032;</mo></msup> <msup><mi>x</mi><mo style="-moz-font-feature-settings: 'ss03'">&#x2032;</mo></msup> </math> However, I would rather wait that the STIX consortium integrates an ssty table rather than adding some specific hack for the STIX fonts (also at the moment we rely on the STIX-General distribution) So the preferred method would be to add support for 'ssty' so that -moz-font-feature-settings: 'ssty'; (or with an explicit value: -moz-font-feature-settings: 'ssty' 1;) and -moz-font-feature-settings: 'ssty' 2; would allow to select the variant for level 1 and 2. Then you can just modify the rule in mathml.css: msub > :not(:first-child), msup > :not(:first-child), msubsup > :not(:first-child), mmultiscripts > :not(:first-child) { -moz-script-level: +1; -moz-font-feature-settings: 'ssty'; } If you want level 2, I guess the best would just be to make the -moz-font-feature-settings level value depend on the moz-script-level in the CSS code (as you did with -moz-mathvariant vs text-style / text-weight). You could also add more complex CSS rules (but note the case of MathML embedded in SVG/HTML itself embedded in MathML) or otherwise use some state bit on text frames as you did for the mathvariant bug. I don't know at all how -moz-font-feature-settings is implemented. But the 'ssty' feature seems very close to 'ss0x' so I guess it won't be too hard to adapt. Another method would be to maintain our own table for fonts and glyph substitutions and do something similar to your mathvariant work. However, note that variant glyphs don't have any Unicode code points so you'll have to find how to access them via glyph names with our font libraries (rather than just doing unicode code point changes). This method seems to be duplicating the 'ss0x' implementation and I don't think we want to keep maintaining private tables, so I won't really recommend that option.

A surprisingly simple patch as it turns out that no changes are necessary to support "ssty" in Neo Euler. Note however it doesn't work with Cambria Math for which I have been unable to find an explanation (although I suspect it to be Harfbuzz related). I would appreciate your opinion on behaviour of the nsRuleNode changes before I get that part reviewed. The non-zero script level check is there to reduce the chances of overriding user set behaviour. I've verified that it works, but there are no automated tests as I have been unable to find a woff font with both ssty support and a license that looks compatible.

whitespace fixes

(In reply to James Kitchener (:jkitch) from comment #8) > A surprisingly simple patch as it turns out that no changes are necessary to > support "ssty" in Neo Euler. Note however it doesn't work with Cambria > Math for which I have been unable to find an explanation (although I suspect > it to be Harfbuzz related). The ‘ssty’ feature of Cambria Math (and propably other math fonts) has ‘math’ script tag only, unlike Neo Euler which has ‘latn’ and ‘DFLT’ tags as well, I suspect that the layout code is defaulting to the Latin script. I think the MathML text should be laid out with ‘math’ script in general.

Thanks for the info, Khaled. @James: That sounds great. Can you investigate about how to set the 'math' script tag? I'm wondering if that would just be as easy as -moz-font-feature-settings: 'ssty', 'math';. Perhaps set -moz-font-feature-settings: 'math' on the <math> tag. Regarding the scriptlevel, three remarks: 1) Perhaps you can check that the scriptlevel is not set. For example SetFont contains eCSSUnit_Unset == scriptLevelValue->GetUnit(). However, since the scriptlevel is only set in the MathML code, I think your idea to just ignore 0 will be fine in general. There is the edge case of mixed content like <math><msup><mi>x</mi><mtext>FOREIGN HTML CONTENT</mtext></math> where the scriptlevel could be transmitted to the foreign content. Maybe just add mtext > *, annotation-xml > * { -moz-script-level: 0 } to reset the script level. Again, I don't think that's too serious. 2) The (still non-public) Open Type spec says "This feature can have a parameter indicating the script level: 1 for simple subscripts and superscripts, 2 for second level subscripts and superscripts (that is, scripts on scripts), and so on. (Currently, only the first two alternates are used).". So it seems best to do feat.mValue = (aScriptLevel > 1 ? 2 : aScriptLevel) for now. 3) I haven't verified the details, but be aware of the "cheesy" hack: http://dxr.mozilla.org/mozilla-central/source/content/mathml/content/src/nsMathMLElement.cpp#544 Regarding the test, someone said somewhere else that it is possible to use your woff fonts on the Mozilla test servers. I suggest to use font-forge to create your own dummy font with only three distinct glyphs (say 'A', 'B', 'C'). The font would have the open type property that maps the glyph 'A' to 'B' in scriptlevel=1 and to 'C' in scriptlevel=2. Then you can do the obvious reftest like <math><msup><mtext>A</mtext><mtext>B</mtext></msup></math> vs <math><msup><mtext>A</mtext><mtext>A</mtext></msup></math> for the first level and similarly for other cases.

(In reply to Frédéric Wang (:fredw) from comment #11) > Thanks for the info, Khaled. > > @James: That sounds great. Can you investigate about how to set the 'math' > script tag? I'm wondering if that would just be as easy as > -moz-font-feature-settings: 'ssty', 'math';. Perhaps set > -moz-font-feature-settings: 'math' on the <math> tag. I don’t think there is a way to set script from CSS. > Regarding the scriptlevel, three remarks: > > 1) Perhaps you can check that the scriptlevel is not set. For example > SetFont contains eCSSUnit_Unset == scriptLevelValue->GetUnit(). However, > since the scriptlevel is only set in the MathML code, I think your idea to > just ignore 0 will be fine in general. There is the edge case of mixed > content like <math><msup><mi>x</mi><mtext>FOREIGN HTML > CONTENT</mtext></math> where the scriptlevel could be transmitted to the > foreign content. Maybe just add mtext > *, annotation-xml > * { > -moz-script-level: 0 } to reset the script level. Again, I don't think > that's too serious. I think “-moz-font-feature-settings: 'ssty' off;” would be cleaner.

(In reply to Khaled Hosny from comment #12) > > I don’t think there is a way to set script from CSS. I'm wondering if it is worth implementing a -moz-font-scripts property or just ask the font library to force the 'math' script somewhere in the MathML text frames. > I think “-moz-font-feature-settings: 'ssty' off;” would be cleaner. James' code currently sets the feature tag value of 'ssty' to the current -moz-script-level when it is nonzero and ignores the value specified by the user (or the one in the MathML stylesheet). So the problem is really what to do when the user sets his own 'ssty' value, but I'm not sure it will really happen.

(In reply to Frédéric Wang (:fredw) from comment #11) > I suggest to use font-forge to > create your own dummy font with only three distinct glyphs (say 'A', 'B', > 'C'). The font would have the open type property that maps the glyph 'A' to > 'B' in scriptlevel=1 and to 'C' in scriptlevel=2. http://hg.mozilla.org/mozilla-central/annotate/f2adb62d07eb/layout/reftests/fonts/mark-generate.py makes some simple fonts.

To set the ssty mapping, try to see if you can use some member here: http://fontforge.org/python.html#Glyph or otherwise do it "by hand" with the fontforge menu. Perhaps setting the ‘math’ script can be postponed to a separate bug if that requires significant change (e.g. implementing a new CSS property).

The appropriate value for the "ssty" font feature setting has a dependence on scriptlevel which I do not believe can be accomplished through CSS alone.

The included woff font contains hand drawn, abstract characters rather than the script in comment 14. It is admittedly ugly, but serves its purpose in demonstrating that ssty works. Supporting "math" scripts is left unimplemented per comment 15. I have looked into it, but implementing it appears to be non-trivial.

Comment on attachment 8343634 [details] [diff] [review] Part 1: RuleNode changes Review of attachment 8343634 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsRuleNode.cpp @@ +3623,5 @@ > + feat.mTag == (('s' << 24) | ('s' << 16) | ('t' << 8) | 'y')) { > + // The value of tag "ssty" is determined by -moz-script-level if the > + // script level is non-zero. Currently only the first two levels are > + // used, so the value is capped. > + feat.mValue = (aScriptLevel > 1 ? 2 : aScriptLevel); Now that I think again about this, the scriptlevel can be negative. Try <math><mstyle scriptlevel="-3"><mo>&#8242;</mo><mstyle></math> <math><mstyle scriptlevel="-2"><mo>&#8242;</mo><mstyle></math> <math><mstyle scriptlevel="-1"><mo>&#8242;</mo><mstyle></math> <math><mstyle><mo>&#8242;</mo><mstyle></math> <math><mstyle scriptlevel="1"><mo>&#8242;</mo><mstyle></math> <math><mstyle scriptlevel="2"><mo>&#8242;</mo><mstyle></math> <math><mstyle scriptlevel="3"><mo>&#8242;</mo><mstyle></math> So I think this should be done only when aScriptlevel > 0.

I think Jonathan and John should comment on whether having the treatment of 'ssty' depend on the scriptlevel like this is OK.

Comment on attachment 8343636 [details] [diff] [review] Part 2: MathML changes and test Review of attachment 8343636 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, that looks good to me. I think you should also: - test with the other script elements (mmultiscripts, msubsup, msub). The Open Type spec says that ssty is for both subscripts and superscripts. - test what happens for deeper level of nested script elements. ('A' should map to 'C' for scriptlevel > 2 too) - test for several scriptlevel values (especially negative ones) set via <mstyle>.

Now with more tests.

Fixed newlines

> I think Jonathan and John should comment on whether having the treatment of > 'ssty' depend on the scriptlevel like this is OK. If possible, I'd prefer to avoid putting MathML-specific hacks like this into the generic feature-handling code in nsRuleNode, and potentially overriding the feature value specified in font-feature-settings. Is there some way the mathml code could directly set the feature (and value) that it wants when handling <msup>, etc., rather than doing it via CSS, which doesn't appear to have all the information needed to choose the right value?

> If possible, I'd prefer to avoid putting MathML-specific hacks like this > into the generic feature-handling code in nsRuleNode, and potentially > overriding the feature value specified in font-feature-settings. I think the reasoning here is: 1) 'ssty' is typically used in advanced math layout. In normal HTML, someone will probably just write "x′" and won't bother with "x<sup style='-moz-font-feature-settings: 'ssty'>′</sup>". For more complex equations, MathML is likely to be used. 2) Even if for some reason someones uses the 'ssty' feature, then the scriptlevel value outside MathML is generally zero so the user setting won't be overriden. The only case I see is when one uses foreign content in MathML or tries to set 'ssty' in MathML itself. 3) Even if the the feature is set from the MathML layout code, it will eventually need to override any user-specified value so we don't win much by doing that. and perhaps more importantly 4) Historically, setting text and CSS properties from the MathML layout has been the source of rendering bugs and security issues so the idea is to clean up some code and move some stuff to the CSS or text system rather than the other way around. This is why roc moved the scriptlevel code to the CSS module a long time ago or why Eshan moved the collapse-whitespace code into the nsTextFrame. This is also what James did for mathvariant and there are other ongoing refactoring work like that (for examples to fix bug 832800, bug 527201 or bug 491384). > > Is there some way the mathml code could directly set the feature (and value) > that it wants when handling <msup>, etc., rather than doing it via CSS, > which doesn't appear to have all the information needed to choose the right > value? It seemed to me that relying on the CSS scriptlevel was the right choice since it already corresponds to the nesting of structure, it is available to the style system and we don't need to bother with computing that again. Also, this will work better with user-specified scriptlevel. However, scriptlevel is also automatically increased by other elements like mfrac and for example it's not clear to me whether the prime in <math><mfrac><msup><mi>x</mi><mo>′</mo></msup></mfrac></math> should have ssty=2 or ssty=1?

How about we have nsTextFrame override the fontFeatureSettings value of the nsFont when we get the font? I think we can do this just by modifying nsLayoutUtils::GetFontMetricsForStyleContext to modify 'font''s fontFeatureSettings based on StyleFont()'s scriptlevel. This lets us add an ssty feature if there isn't already one, avoiding any overriding if there's already an ssty feature.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25) > How about we have nsTextFrame override the fontFeatureSettings value of the > nsFont when we get the font? I think we can do this just by modifying > nsLayoutUtils::GetFontMetricsForStyleContext to modify 'font''s > fontFeatureSettings based on StyleFont()'s scriptlevel. This lets us add an > ssty feature if there isn't already one, avoiding any overriding if there's > already an ssty feature. I like this, except that I don't think it handles examples like <math><mstyle scriptlevel="-1"><mo><msup>x&prime;</msup></mo></mstyle></math> where, as I understand it, the prime's scriptlevel will be zero, but we still need to apply 'ssty' to it to get a properly-sized glyph. AFAICS, we need the MathML code to have some way to communicate the fact that the prime is (super|sub)scripted - and therefore should have the feature applied - separately from the actual value of scriptlevel. Maybe an additional bool mIsMathScript on nsFont? It looks like we currently have some dead space in that struct anyhow. (Should we pack the bools there into bits, or is speed of access more important than space?)

(In reply to Jonathan Kew (:jfkthame) from comment #26) > AFAICS, we need the MathML code to have some way to communicate the fact > that the prime is (super|sub)scripted - and therefore should have the > feature applied - separately from the actual value of scriptlevel. ...which the CSS approach was achieving via "-moz-font-feature-settings: 'ssty'", but that didn't feel like a clean way to implement this, as it would overwrite any attempt by a user to explicitly control the feature setting.

Comment on attachment 8344129 [details] [diff] [review] Part 2: MathML changes and test Review of attachment 8344129 [details] [diff] [review]: ----------------------------------------------------------------- > I like this, except that I don't think it handles examples like > > <math><mstyle scriptlevel="-1"><mo><msup>x&prime;</msup></mo></mstyle></math> > >where, as I understand it, the prime's scriptlevel will be zero, but we still need to apply 'ssty' to it to get a properly-sized glyph. So I guess you mean <math><mstyle scriptlevel="-1"><msup><mi>x</mi><mo>&prime;</mo></msup></mstyle></math> So yes, the scriptlevel is 0 here. I was expecting that the ssty was to correct for the scriptlevel but it seems it is rather to correct for the scriptlevel change aFont->mScriptLevel - aParentFont->mScriptLevel (although this formula is probably still not always correct in general). This also means that some of the tests here are incorrect. Perhaps we should defined how do you determine the ssty value first, since the Open Type MATH spec is not really clear to me. For example on the following <mo> primes? 1) <math><msup><mi>x</mi><mo>&prime;</mo></msup></math> 2) <math><msub><mi>x</mi><mo>&prime;</mo></msub></math> 3) <math><msup><mi>x</mi><msup><mi>y</mi><mo>&prime;</mo></msup></msup></math> 4) <math><msup><msup><mi>x</mi><mo>&prime;</mo></msup><mo>&prime;</mo></msup></math> 5) <math><msub><mi>x</mi><msup><mi>y</mi><mo>&prime;</mo></msup></msub></math> 6) <math><msub><msup><mi>x</mi><mo>&prime;</mo></msup><mo>&prime;</mo></msub></math>

In TeX we apply ssty=1 to script font, and ssty=2 to scriptscript font, I’m not sure what is the equivalent in MathML, but I guess it should be similar to how TeX decides which font to use.

For MathML prime purposes, I think we should set ssty=1 when scriptlevel <= 1, and ssty=2 when scriptlevel >= 2. This should apply regardless of how the scriptlevel is reached, as its main purpose is essentially a form of optical sizing (in plain TeX, the equivalent is the use of 7pt and 5pt design sizes of CMR, when the default text size is 10pt). For most glyphs, it would make most sense to use ssty=0 (i.e. turn the feature off) when scriptlevel <= 0, and only apply an ssty substitution when scriptlevel >= 1. But for the prime (and any others like it) where the default glyph is "pre-superscripted", we'll need to apply ssty=1 *whenever* it is used in a math sub/superscript (even if scriptlevel had previously been decremented below zero) as this also applies a vertical shift and reduced font size, and so we need the ssty feature (with a minimum value of 1) to "undo" the pre-superscripting of the default glyph.

(In reply to Khaled Hosny from comment #29) > In TeX we apply ssty=1 to script font, and ssty=2 to scriptscript font, I’m > not sure what is the equivalent in MathML, but I guess it should be similar > to how TeX decides which font to use. From the MathML specification: "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." If we want to take displaystyle into account, then we could rely on the work of bug 838506. Then the nsTextFrame could do what Robert suggested in comment 25, using the CSS properties for displaystyle and scriptlevel. However, it seems to me that we also want to apply the correction when scriptlevel is > 2 (and actually the larger ssty values should probably be used when they exist in the font, at least it seems to be what the "so on" in Microsoft document suggest). Note that per the spec <msup> and similar tags 'increments scriptlevel by 1, and sets displaystyle to "false", within superscript'. Also the recommended markup for primes and similar operators is to use them in a script element. In both cases <math><msup><mi>x</mi><mo>&prime;</mo></msup></math> and <math display="block"><msup><mi>x</mi><mo>&prime;</mo></msup></math> the prime is \scriptstyle. So just considering the scriptlevel might be enough to do the job in usual cases.

(In reply to Frédéric Wang (:fredw) from comment #31) > So just considering the scriptlevel might be > enough to do the job in usual cases. In the usual cases, yes. The corner case where this breaks down is when scriptlevel has been decremented to a value < 0 somewhere outside the element where the prime occurs, so that the increment done by <msup> doesn't actually result in a positive scriptlevel - yet we still need ssty applied to the prime.

This version of the patch determines the appropriate SSTY level by walking up the frame tree and counting the number of sub/supscript etc ancestors (with a check to exclude base frames). It is then passed onto the appropriate text run factory which sets the font feature setting, if there isn't already a user set value for ssty. The disadvantage of this approach is that every line container has its ancestors examined, regardless of MathML influence. I experimented with setting a frame flag to limit the number of line containers examined, but encountered difficulties in ensuring that the flag propagates recursively to all children. (nsMathVariantTextRunFactory was generalised to nsMathMLTextRunFactory and now performs the ssty setting as well. To do otherwise is a large duplication of code).

Three main differences. 1. The implementation in part 1 ignores scriptlevel when determining the appropriate ssty setting, so tests for this were added. 2. ssty-2 contains dynamic tests, ensuring that the ssty setting is correct when nodes are added/relocated. 3. The font now supports a D character which uses the same glyph as A, but without the ssty substitutions.

I had another go at setting a flag to limit the times when we check for script ancestry, and was unable to replicate the problems I had earlier. The implementation uses TransmitAutomaticData(), which you mentioned on IIRC was somewhat buggy. Would a SetInitialChildList()/AppendFrames()/InsertFrames() approach be better? Note that the flag is never unset. The consequence of this is that relocated nodes will still be examined for script ancestry, but it will not cause false ssty applications.

(In reply to James Kitchener (:jkitch) from comment #35) > The implementation uses TransmitAutomaticData(), which you mentioned on IIRC > was somewhat buggy. Would a > SetInitialChildList()/AppendFrames()/InsertFrames() approach be better? I think TransmitAutomaticData is fine is most case. See bug 750169 and Karl's comment on issues involving maction. Also, see 838509 for related ideas about mstyle.

Comment on attachment 8351694 [details] [diff] [review] Part 1: TextFrame and MathML Factory changes Review of attachment 8351694 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/MathMLTextRunFactory.cpp @@ +535,5 @@ > + } > + if (!found) { > + gfxFontFeature settingSSTY; > + settingSSTY.mTag = TRUETYPE_TAG('s','s','t','y'); > + settingSSTY.mValue = mSSTYLevel > 1 ? 2 : mSSTYLevel; So IIUC, if the ssty level is 0 and the mathvariant is set (perhaps implicitly via a single mi), this will end up setting settingSSTY.mValue to 0. The Open Type MATH document mentions the values 1 for scripts and 2 for scripts of scripts ; I don't know whether 0 is a valid value. If not, then you should not append settingSSTY in that case. @@ +536,5 @@ > + if (!found) { > + gfxFontFeature settingSSTY; > + settingSSTY.mTag = TRUETYPE_TAG('s','s','t','y'); > + settingSSTY.mValue = mSSTYLevel > 1 ? 2 : mSSTYLevel; > + // Currently only the first two levels are used, so the value is capped. This comment should be above the settingSSTY.mValue value, I guess.

Comment on attachment 8351735 [details] [diff] [review] Part 3: Use a flag to limit script ancestry checking Review of attachment 8351735 [details] [diff] [review]: ----------------------------------------------------------------- I'm wondering if we should also consider scripts in munderover/munder/mover and mroot...

(In reply to Frédéric Wang (:fredw) from comment #38) > I'm wondering if we should also consider scripts in munderover/munder/mover > and mroot... I think so.

Comment on attachment 8351695 [details] [diff] [review] Part 2: tests Review of attachment 8351695 [details] [diff] [review]: ----------------------------------------------------------------- > I had another go at setting a flag to limit the times when we check for script ancestry, and was unable to replicate the problems I had earlier. IIRC, <mstyle> and <math> are not really optimized and just relayout everything if you set/remove an attribute/child, so your test might not really verify the propagation. In particular, this is what is likely to happen when you move the "foo" element in ssty-2. I suggest to split ssty-2 into several <math> elements (each one in its own <p> paragraph) and to only do one add/remove operation per <math> to limit side effects. You can create new mo elements with createElementNS using the MathML namespace. I suspect you'll be able to reproduce some refresh bug with maction, as I said in comment 36. However, this is not too serious for now and can be handled in bug 750169. ::: layout/reftests/mathml/ssty-1.html @@ +97,5 @@ > + <p> > + > + <!-- Automatically set ssty ignores scriptlevel --> > + <math> > + <mstyle style="font-family: 'ssty';" scriptlevel=-3> Here, below and in the ref, can you use explicit double quotes around attribute values, just to be consistent. @@ +279,5 @@ > + <mstyle style="font-family: 'ssty';"> > + <msup> > + <mo>A</mo> > + <msup> > + <mo style='-moz-font-feature-settings: "ssty" 0'>A</mo> Again to be consistent, use double quotes for the style attribute and simple quote for the ssty value. As I said, I don't know if 0 is a valid ssty value...

(In reply to Frédéric Wang (:fredw) from comment #40) > As I said, I don't know if 0 is a valid ssty value... According to http://www.w3.org/TR/css3-fonts/#propdef-font-feature-settings is a valide value and it simply disables the feature.

It would be good to check test 28 of the MathML torture test https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test With the patch, the triple U+2032 primes should have the expected size with at least Neo Euler. I don't know if the apostrophe ' has a ssty value, but I suspect we will eventually want to only keep the first form with U+2032, or even better only use a single U+2034 triple prime. There are other related issues to consider: - U+2032 has rspace=2 while ' has not space around it (I'm not sure why...) - Other primes are missing from the operator dictionary (http://lists.w3.org/Archives/Public/www-math/2014Jan/0001.html) - We should probably do as in TeX and don't add spacing around operators used as scripts (bug 877563)

(In reply to Frédéric Wang (:fredw) from comment #40) > Comment on attachment 8351695 [details] [diff] [review] > > + <mstyle style="font-family: 'ssty';" scriptlevel=-3> > @@ +279,5 @@ > > + <mstyle style="font-family: 'ssty';"> This looks odd; surely 'ssty' should be a value for -moz-font-feature-settings, not font-family.

(In reply to Khaled Hosny from comment #41) > According to http://www.w3.org/TR/css3-fonts/#propdef-font-feature-settings > is a valide value and it simply disables the feature. OK, thanks for the info. (In reply to Frédéric Wang (:fredw) from comment #37) > So IIUC, if the ssty level is 0 and the mathvariant is set (perhaps > implicitly via a single mi), this will end up setting settingSSTY.mValue to > 0. The Open Type MATH document mentions the values 1 for scripts and 2 for > scripts of scripts ; I don't know whether 0 is a valid value. If not, then > you should not append settingSSTY in that case. Never mind, I just realized that you do a if (mSSTYLevel) in nsMathMLTextRunFactory::RebuildTextRun. the sstyLevel variable is not used in that function.

(In reply to Jonathan Kew (:jfkthame) from comment #43) > This looks odd; surely 'ssty' should be a value for > -moz-font-feature-settings, not font-family. I also got confused, but this is the @font-face name. Perhaps another name should be used.

(In reply to Frédéric Wang (:fredw) from comment #45) > (In reply to Jonathan Kew (:jfkthame) from comment #43) > > > This looks odd; surely 'ssty' should be a value for > > -moz-font-feature-settings, not font-family. > > I also got confused, but this is the @font-face name. Perhaps another name > should be used. Oh! Sorry, I didn't actually read the patch - it was just a drive-by comment when I noticed this in your review. Yes, I think using a different family name would be helpful.

(In reply to Frédéric Wang (:fredw) from comment #40) > I had another go at setting a flag to limit the times when we check for script ancestry, and was unable to replicate the problems I had earlier. I just realized that while updating tests for bug 838509, but I think dynamic reftests require a special syntax with reftest-wait and MozReftestInvalidate to ensure the screenshot is taken after the dynamic change. See my changes to your mathvariant patch: https://bugzilla.mozilla.org/attachment.cgi?id=8356553&action=diff#a/layout/reftests/mathml/mathvariant-5.html_sec3

This patch has two chief changes, both of which are utilised in part 2. 1. A function has been added to return the natural scriptlevel increment set on a child frame provided as argument. 2. TransmitAutomaticData() now sets a flag on appropriate children (and all of their descendants) to inform them when the scriptlevel has been incremented. Note that this patch now depends on bug 838506 as the two touch the same code (and the latter fixes a bug in the test suite).

When nsIFrame flag NS_FRAME_MATHML_SCRIPT_DESCENDANT has been set, the code walks up the frame tree to reconstruct the scriptlevel. It differs from CSS scriptlevel in that it does not include all scriptlevel changes (specifically user set ones). It then passes the script level onto a MathMLTextRunFactory (Previously MathVariantTextRunFactory, but now performs two functions to avoid a large amount of code duplication), which sets the ssty font feature level based on an algorithm which depends on scriptlevel and scriptsizemultiplier. If the user has already set the ssty font feature, no action is taken. Strictly speaking, the NS_FRAME_MATHML_SCRIPT_DESCENDANT is optional and is designed to avoid unnecessary reconstructions of scriptlevel when it has never changed. fredw: Your part of the review is confirming that the algorithm used to generate the font feature value is acceptable.

ssty-1 handles mmultiscripts et al. and scriptlevel changes ssty-2 handles mroot, mfrac with possible displaystyle and munderover et al. with possible accent/accentunder. ssty-3 tests the effect of scriptsizemultiplier on ssty font feature settings. ssty-4 handles dynamic tests and includes the testcase from bug 958914

(In reply to Frédéric Wang (:fredw) from comment #42) > It would be good to check test 28 of the MathML torture test > https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/ > MathML_Torture_Test > With the patch, the triple U+2032 primes should have the expected size with > at least Neo Euler. I don't know if the apostrophe ' has a ssty value, but I > suspect we will eventually want to only keep the first form with U+2032, or > even better only use a single U+2034 triple prime. > With Neo Euler, the U+2032 primes render properly with ssty font feature applied. No change to the apostrophe case. > There are other related issues to consider: > - U+2032 has rspace=2 while ' has not space around it (I'm not sure why...) > - Other primes are missing from the operator dictionary > (http://lists.w3.org/Archives/Public/www-math/2014Jan/0001.html) > - We should probably do as in TeX and don't add spacing around operators > used as scripts (bug 877563) These I have left to followup bugs, unless they are important enough to block landing.

(In reply to James Kitchener (:jkitch) from comment #51) > (In reply to Frédéric Wang (:fredw) from comment #42) > > It would be good to check test 28 of the MathML torture test > > https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/ > > MathML_Torture_Test > > With the patch, the triple U+2032 primes should have the expected size with > > at least Neo Euler. I don't know if the apostrophe ' has a ssty value, but I > > suspect we will eventually want to only keep the first form with U+2032, or > > even better only use a single U+2034 triple prime. > > > > With Neo Euler, the U+2032 primes render properly with ssty font feature > applied. No change to the apostrophe case. I've remove the triple apostrophes from the MathML torture test as the proper markup is with prime characters. > > > There are other related issues to consider: > > - U+2032 has rspace=2 while ' has not space around it (I'm not sure why...) > > - Other primes are missing from the operator dictionary > > (http://lists.w3.org/Archives/Public/www-math/2014Jan/0001.html) > > - We should probably do as in TeX and don't add spacing around operators > > used as scripts (bug 877563) > > These I have left to followup bugs, unless they are important enough to > block landing. Yes, issues in the operator dictionary have been reported to the MathML WG, let's wait their feedback before working on that. Bug 877563 has already its own bug.

Comment on attachment 8358915 [details] [diff] [review] Part 2: TextFrame and MathMLFactory Changes Review of attachment 8358915 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsTextFrame.cpp @@ +2004,5 @@ > + parent = parent->GetParent(); > + oldScriptLevel = sstyScriptLevel; > + } > + if (sstyScriptLevel) { > + anyMathMLStyling = true; I guess it is not really a problem, but it seems to me that anyMathMLStyling will also happen in your "overflow" case above.

Comment on attachment 8358912 [details] [diff] [review] Part 1: layout/mathml changes Review of attachment 8358912 [details] [diff] [review]: ----------------------------------------------------------------- OK, looks good to me. Just collapse the whitespace inside all these SetIncrement functions ScriptIncrement(nsIFrame* aFrame) => ScriptIncrement(nsIFrame* aFrame)

Rebased to take account of bug 838506 changes.

(In reply to Frédéric Wang (:fredw) from comment #53) > > I guess it is not really a problem, but it seems to me that anyMathMLStyling > will also happen in your "overflow" case above. That is intentional. Overflow detects when the recomputed scriptlevel increases past 255 and caps the result at that level. A larger cap can easily be supported by using a different type, but uint8_t was chosen as mScriptLevel is also 8 bit. In practice since the ssty level is capped at 2 it won't make any difference, unless a scriptsizemultiplier absurdly close to 1 is used.

Documentation notes: The ssty font feature setting is now set on the descendants of MathML elements that increase scriptlevel, other than a user specified scriptlevel attribute. E.g. in the case of a single <msup>, the font feature is applied to the second (script) child element, but not the first (base) child element. The particular value of the setting is a function of scriptlevel (excluding scriptlevel attribute changes) and scriptsizemultiplier. The algorithm used is admittedly arbitrary and may change in the future. User specified values of the ssty font feature setting override the default behaviour, so if web authors do not wish to be affected by the changes, they can add the following css to their code -moz-font-feature-settings: 'ssty' 0

(In reply to James Kitchener (:jkitch) from comment #57) > Documentation notes: Note that ssty glyphs that are stored under the OpenRype "math" script are not currently supported. (see bug 953385).

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0b1c4cf207a https://hg.mozilla.org/integration/mozilla-inbound/rev/10b9624cde2b https://hg.mozilla.org/integration/mozilla-inbound/rev/b595ecfe8117

https://hg.mozilla.org/mozilla-central/rev/a0b1c4cf207a https://hg.mozilla.org/mozilla-central/rev/10b9624cde2b https://hg.mozilla.org/mozilla-central/rev/b595ecfe8117

Reported to STIX: https://sourceforge.net/p/stixfonts/tracking/48/

I added a browser note on the font-feature-settings page: https://developer.mozilla.org/en-US/docs/Web/CSS/font-feature-settings#Browser_notes