Bug 114365 - (mathvariant) [MathML2.0] Support for the 'mathvariant' attribute
 (mathvariant) Summary: [MathML2.0] Support for the 'mathvariant' attribute
 Status: RESOLVED FIXED [documentation: see comments 112 and ... dev-doc-complete Core Components MathML (show other bugs) Trunk All All P4 normal with 7 votes (vote) mozilla28 James Kitchener (:jkitch) 162403 162405 162412 413115 449396 919164 945509 mathml-2 mathml-in-mathjax 923890 69409 cambria-math asana-math 518592 731667 838509 930504 941607 1043358 Show dependency tree / graph

Reported: 2001-12-09 22:30 PST by rbs
Modified: 2014-09-13 13:23 PDT (History)
21 users (show)
jruderman: sec‑review+
ryanvm: in‑testsuite+
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
 relnote-firefox: 28+

Attachments
fixup some CSS rules in mathml.css (1.86 KB, patch)
2005-09-05 00:22 PDT, rbs
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Part 0: CSS related changes (16.67 KB, patch)
2013-09-20 07:16 PDT, James Kitchener (:jkitch)
fred.wang: feedback+
Details | Diff | Splinter Review
Part 1: TextFrame and Textrun transformation changes (23.64 KB, patch)
2013-09-20 07:23 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 2: Parse the mathvariant, fontstyle and fontweight properties (7.03 KB, patch)
2013-09-20 07:24 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 3: MathML frame changes (14.17 KB, patch)
2013-09-20 07:26 PDT, James Kitchener (:jkitch)
fred.wang: feedback+
Details | Diff | Splinter Review
Part 4: Remoe legacy stuff (17.19 KB, patch)
2013-09-20 07:27 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 5: tests (10.16 KB, patch)
2013-09-20 07:29 PDT, James Kitchener (:jkitch)
fred.wang: feedback+
Details | Diff | Splinter Review
bug-114365-testjs.patch (2.78 KB, patch)
2013-10-14 19:59 PDT, Raniere Silva
no flags Details | Diff | Splinter Review
bug-114365-testjs.patch (2.87 KB, patch)
2013-10-15 07:08 PDT, Raniere Silva
no flags Details | Diff | Splinter Review
Part 0: CSS related changes (18.19 KB, patch)
2013-10-22 02:59 PDT, James Kitchener (:jkitch)
fred.wang: feedback+
Details | Diff | Splinter Review
Part 1: TextFrame and Textrun transformation changes (26.37 KB, patch)
2013-10-22 03:07 PDT, James Kitchener (:jkitch)
fred.wang: feedback+
Details | Diff | Splinter Review
Part 2: Parse the mathvariant, fontstyle and fontweight properties (7.38 KB, patch)
2013-10-22 03:10 PDT, James Kitchener (:jkitch)
fred.wang: review+
Details | Diff | Splinter Review
Part 3: MathML frame changes (14.34 KB, patch)
2013-10-22 03:12 PDT, James Kitchener (:jkitch)
fred.wang: review+
Details | Diff | Splinter Review
Part 4: Remove legacy stuff (31.54 KB, patch)
2013-10-22 03:17 PDT, James Kitchener (:jkitch)
fred.wang: review+
Details | Diff | Splinter Review
Part 5: tests (54.96 KB, patch)
2013-10-22 03:19 PDT, James Kitchener (:jkitch)
fred.wang: review+
Details | Diff | Splinter Review
Part 0: CSS related changes (18.19 KB, patch)
2013-10-25 05:58 PDT, James Kitchener (:jkitch)
cam: review+
Details | Diff | Splinter Review
Part 1: TextFrame and Textrun transformation changes (29.25 KB, patch)
2013-10-25 06:21 PDT, James Kitchener (:jkitch)
roc: review-
Details | Diff | Splinter Review
Part 2: Parse the mathvariant, fontstyle and fontweight properties (7.39 KB, patch)
2013-10-25 06:22 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 3: MathML frame changes (14.20 KB, patch)
2013-10-25 06:23 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 4: Remove legacy stuff (31.55 KB, patch)
2013-10-25 06:23 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 5: tests (50.41 KB, patch)
2013-10-25 06:24 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 0: CSS related changes (19.14 KB, patch)
2013-11-26 00:05 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 0: CSS related changes (18.99 KB, patch)
2013-11-26 00:09 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 1: TextFrame and Textrun transformation changes (37.43 KB, patch)
2013-11-26 00:20 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 0: CSS related changes (19.23 KB, patch)
2013-11-26 00:30 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 1: TextFrame and Textrun transformation changes (36.75 KB, patch)
2013-11-26 00:31 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 5: tests (54.89 KB, patch)
2013-11-26 00:36 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 0: CSS related changes (19.10 KB, patch)
2013-11-26 05:42 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 4: Remove legacy stuff (32.48 KB, patch)
2013-11-26 05:43 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 0: CSS related changes (21.45 KB, patch)
2013-11-29 00:36 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 3: MathML frame changes (14.08 KB, patch)
2013-11-29 00:37 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 1: TextFrame and Textrun transformation changes (37.45 KB, patch)
2013-11-30 16:36 PST, James Kitchener (:jkitch)
roc: review+
Details | Diff | Splinter Review
Part 1: TextFrame and Textrun transformation changes (37.44 KB, patch)
2013-12-01 05:05 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review
Part 5: tests (55.96 KB, patch)
2013-12-02 01:10 PST, James Kitchener (:jkitch)
no flags Details | Diff | Splinter Review

 rbs 2001-12-09 22:30:05 PST [See also 65951] MathML 2.0 has introduced a new attribute which amongst other things allows referencing characters in plane-1 (remember the artefact of a to get alpha). These characters are currently mapped to PUA assignments in Mozilla, and users can access them via their entity references. Since plane-1 characters break nearly all current applications (including James Clark's nsgmls which sits behind most validating XML parsers), |mathvariant| provides a work-around that some people find useful. Here is a screenshot: http://www.w3.org/Math/testsuite/Presentation/TokenElements/mi/mimathvariant13.html Another peculiarity of |mathvariant| attribute is that it is a flag for style invariant characters (bug 65951), so I guess this attribute needs to be supported at some stage. I am filing this bug to jot down some thougths as I was reading. Some values of |mathvariant| can be mapped directly to CSS in mathml.css, others require back-end code. (maybe the CSS rules should inlcude the |font| shorthand to reset everything first in order to meet the style invariant requirement). a -- can be mapped directly to CSS a -- ditto a -- ditto a -- ditto a -- CSS + code in GFX, specifically use [mathvariant='double-struck'] { font-family: -moz-double-struck /* then, detect this font-family in GFX and */ /* simply convert the character indices */ /* to the expected PUA code points */ } a -- ditto here + |font-weight: bold| a -- similar tweaking here and below a a a -- these can be mapped directly to CSS a a a a ab rbs 2002-02-15 02:13:10 PST I checked in a partial fix for (CSS rules) while I was doing the work to support MathML styling attributes. rbs 2002-08-08 18:57:24 PDT Possible plan of action: CSS === - add recognition of new -moz values for |font-variant|: font-variant: -moz-script | -moz-fraktur | -moz-double-struck (currently |font-variant| only supports |small-caps|). - add CSS rules in mathml.css: [mathvariant="double-struck"] { font-variant: -moz-double-struck; } [mathvariant="fraktur"], [mathvariant="bold-fraktur"] { font-variant: -moz-fraktur; } [mathvariant="script"], [mathvariant="bold-script"] { font-variant: -moz-script; } MathML ====== - merge mi/mtext/ms into a single token class: nsMathMLTokenFrame - make mo inherits from the token class - make the token class set a frame state bit in their child nsTextFrame (to flag that since the parent is MathML-related, the child might need special processing - see below). nsTextFrame / nsTextTranformer ============================== For platforms that support surrogate pairs, when font.variant has one of the new values, transform the text to the corresponding surrogate representation and let the font code deal with the rest. For platforms that don't support surrogate pairs, transform the text to the corresponding PUA values and let the font code deal with these. ucvmath ======= It might be necessary to setup converters that work with surrogate input. If not, the transformation stage would have to output PUA values. In fact, a first implementation could be entirely based on PUA values (i.e., the current converters are retained). Then, the migration to surrogate (on platforms that support it) could be another stage. issues ====== Since the text might already been using the surrogate representation of these plane-1 characters, it might be necessary to transform this text to the existing PUA values so that the font code can deal with them. With a quick test to see if the MathML frame state bit is set, a scan can be done to attempt to perform the transformation. This way, the penalty of the scan won't affect non-MathML frames. rbs 2002-08-12 19:36:39 PDT Filed the following sub-tasks: bug 162403: Extend conveter APIs to surrogate pairs bug 162405: Add support for font-variant: -moz-script | -moz-fraktur | -moz-double-struck bug 162412: Merge mi/mtext/ms into a single token class: nsMathMLTokenFrame rbs 2005-09-05 00:22:32 PDT Created attachment 194890 [details] [diff] [review] fixup some CSS rules in mathml.css The "font" shorthand doesn't play nice with the propagation of the scriptlevel. Using "font" required to use "font-size: inherit" as well, and it was initially thought that this would have recovered the size that "font" would have reset, but it actually means that the element inherits the size of the parent... that's not what is needed in a fraction, superscript, etc, as can be seen in this example: x a So, the fix now is to stay clear of "font" and manually reset all the font-xxx properties that have to be reset when mathvariant is specified. rbs 2005-09-05 20:53:07 PDT Comment on attachment 194890 [details] [diff] [review] fixup some CSS rules in mathml.css This one was checked in. Brad Bell 2007-12-13 06:30:48 PST The following mathvariant test does not display properly in Firefox 2.0.0.11: http://www.w3.org/Math/testsuite/testsuite/Presentation/TokenElements/mi/mimathvariant14.xml Checking for updates (just now) with this version results in a message that none are available. The About Box with this version yields the following information: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11 In addition, I just downloaded and re-installed the MathML fonts suggested for windows; i.e., http://web.mit.edu/is/topics/webpublishing/mathml/fonts-win.html  Karl Tomlinson (:karlt) 2008-01-20 13:46:33 PST http://www.w3.org/TR/2007/WD-MathML3-20071214/chapter3.html#presm.symbolchars "Processing applications that accept SMP characters are required to treat the corresponding BMP and attribute combinations identically." I guess this means that copying the text should provide the Plane 1 characters, and that might require modifying the content rather than performing a transformation for the TextRuns. Felix Breuer 2010-05-05 13:22:31 PDT Is there any progress on this? Supporting mathvariant would be helpful for use with applications targeting MathML 3.0 such as the popular MathJax: http://www.mathjax.org MathJax makes heavy use of mathvariant. email1990+bugzilla 2010-05-14 11:28:45 PDT Please fix this bug! Math is no fun without \mathbb, \mathcal and \mathfrac. Brad Bell 2011-11-15 14:55:11 PST (In reply to Brad Bell from comment #6) > The following mathvariant test does not display properly in Firefox 2.0.0.11: > http://www.w3.org/Math/testsuite/testsuite/Presentation/TokenElements/mi/ > mimathvariant14.xml > ... snip ... The web page for this test has changed to: http://www.w3.org/Math/testsuite/mml2-testsuite/Presentation/TokenElements/mi/mimathvariant13.xml Frédéric Wang (:fredw) 2011-11-15 15:29:35 PST (In reply to Brad Bell from comment #10) > The web page for this test has changed to: > http://www.w3.org/Math/testsuite/mml2-testsuite/Presentation/TokenElements/ > mi/mimathvariant13.xml You may want to try the build at http://www.wg9s.com/mozilla/firefox/ with MathJax fonts installed (bug 701758) Joe Java 2012-05-18 08:27:11 PDT Using latest nightly and all the suggested fonts at https://developer.mozilla.org/en/Mozilla_MathML_Project/Fonts the webpage for this test: http://www.w3.org/Math/testsuite/mml2-testsuite/Presentation/TokenElements/mi/mimathvariant13.xml displays correctly. Perhaps this bug should be marked as fixed. Frédéric Wang (:fredw) 2012-05-18 08:39:50 PDT Only the most important characters in double-struck, script or fraktur should now render correctly and only with MathJax fonts installed. Frédéric Wang (:fredw) 2012-08-02 11:06:58 PDT I got feedback on this bug again. Relying on existing CSS properties and adding new properties as suggested in bug 162405 does not seem the right approach. mathvariant is supposed to map existing characters to their corresponding bold, fraktur, sans-serif etc counterpart, if they exist. So maybe a better implementation is the following: - use a single CSS property -moz-mathvariant with values default, normal, bold, italic, bold-italic, double-struck, bold-fraktur, script, bold-script, fraktur, sans-serif, bold-sans-serif, sans-serif-italic, sans-serif-bold-italic, monospace. - map the mathvariant attribute to this property in nsMathMLElement - do the char remapping in layout/generic, in the files that handle the text I'm wondering if we could also do something in layout/generic to handle at the same time the cases of single x and token elements with leading/trailing spaces text , that would solve bugs like bug 518592, bug 770710, bug 527201 etc Of course this approach won't address the issue mentioned in comment 7. Karl Tomlinson (:karlt) 2012-08-02 14:45:39 PDT (In reply to Frédéric Wang (:fredw) from comment #14) > - do the char remapping in layout/generic, in the files that handle the text Have a look at nsTextRunTransformations, which does something similar. > I'm wondering if we could also do something in layout/generic to handle at > the same time the cases of single x and token elements with > leading/trailing spaces text , that would solve bugs like > bug 518592, bug 770710, bug 527201 etc I don't know exactly what would be involved here, but perhaps something similar could also make text frames return intrinsic widths (or perhaps frame bounds, even) that include glyph overflow to fix the problems with math content overflowing in matrices. Frédéric Wang (:fredw) 2012-08-02 22:56:21 PDT (In reply to Karl Tomlinson (:karlt) from comment #15) > > I'm wondering if we could also do something in layout/generic to handle at > > the same time the cases of single x and token elements with > > leading/trailing spaces text , that would solve bugs like > > bug 518592, bug 770710, bug 527201 etc > > I don't know exactly what would be involved here, but perhaps something > similar could also make text frames return intrinsic widths (or perhaps > frame bounds, even) that include glyph overflow to fix the problems with > math content overflowing in matrices. Interesting. My idea was to use two other -moz-mathvariant properties "math-text" (for non-mi token) and "math-identifier" which would be set in mathml.css by mtext, mo, ms, mn { -moz-mathvariant: math-text; } mi { -moz-mathvariant: math-identifier; } I'm not exactly sure about the priority between CSS rules applied in nsMathMLElement and mathml.css. So we may need to map the mathvariant attribute to -moz-mathvariant in mathml.css instead: [mathvariant="fraktur"] { -moz-mathvariant: fraktur; } etc And I don't remember exactly about the priority of CSS rules but we must do that in a way that the mathvariant rules override the rules for token elements (perhaps adding more rules). Now, in layout/generic we add MathML token elements specific stuff (leading/trailing space removal, computation of intrinsic widths) when -moz-mathvariant is not "default". When -moz-mathvariant is "math-identifier" we render the text as italic when the mi content is made of a single variant char. When -moz-mathvariant is not "default", "math-identifier" or "math-text" we perform the appropriate mathvariant transformation. If we can detect that the text frame type is MathML in layout/generic, one of the value "default" or "math-text" can be removed. Frédéric Wang (:fredw) 2012-08-04 03:11:38 PDT I forgot to say that we'll keep the [mathvariant] rule to reset the CSS font properties and won't use font rules for other mathvariant attributes. So in my opinion, mathvariant="bold" should only affect the characters for which a corresponding bold Unicode code point exists in contrast to "font-weight: bold" which puts all characters in bold. BTW, I think that a -moz-mathvariant property to distinguish MathML tokens will enable to fix bug 560100 without adding new CSS properties. In general, I expect that it will simplify nsMathMLTokenFrame a lot. Frédéric Wang (:fredw) 2012-11-30 08:32:54 PST 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. Frédéric Wang (:fredw) 2013-02-06 00:58:27 PST FYI, the Webkit bug: https://bugs.webkit.org/show_bug.cgi?id=108778 Frédéric Wang (:fredw) 2013-07-27 01:19:44 PDT @James Kitchener: that would definitely great if you could work on this bug. Here are my latest thoughts on this: 1) The TEXT_FORCE_TRIM_WHITESPACE flags are not correctly set by nsMathMLTokenFrame::ForceTrimChildTextFrames. See Patch V5 from bug 415413 for how it could be fixed and changed to a more general TEXT_IS_IN_TOKEN_MATHML flag. I guess the function could also be modified to set another flag TEXT_IS_IN_SINGLE_CHAR_MI when you are in an element with only a single char. 2) A -moz-mathvariant CSS property should be implemented. The mathvariant attribute will be mapped to that property in content/mathml/ as usual. It should be allowed on token elements and math/mstyle. 3) As Karl suggested, nsTextRunTransformations should be modified to do the mapping when TEXT_IS_IN_TOKEN_MATHML is set. Basic rules are: - if there is an explicit -moz-mathvariant value, use it. - otherwise single mi maps to "italic" ; multiple mi and other token elements to "normal". Use the TEXT_IS_IN_SINGLE_CHAR_MI as a help. - the mapping should only be done for characters where a transformation exists: latin/greek alphabet and digits (see https://en.wikipedia.org/wiki/Mathematical_alphanumeric_symbols) 4) You'll have to clean up the old CSS rules in mathml.css, the invariant char and mi code etc You can have a look at my experimental patches that I wrote some time ago. These are probably not quite correct but will give you an idea of the places to modify: https://github.com/fred-wang/MozillaCentralPatches/blob/master/mathml-token-1.diff https://github.com/fred-wang/MozillaCentralPatches/blob/master/mathml-token-2.diff https://github.com/fred-wang/MozillaCentralPatches/blob/master/mathml-token-3.diff https://github.com/fred-wang/MozillaCentralPatches/blob/master/mathml-token-4.diff https://github.com/fred-wang/MozillaCentralPatches/blob/master/mathml-token-5.diff Note that this is what the spec says, but there is also the question of whether we should keep using font-weight and font-style to emulate bold, italic and bolditalic. Indeed some people may not have fonts with characters from the math alpha num block and will see "missing char" symbols... James Kitchener (:jkitch) 2013-09-20 07:16:46 PDT Created attachment 807752 [details] [diff] [review] Part 0: CSS related changes Other than some slight rearranging, there has been little change since the experimental patches. One thing I am stuck on is having -moz-mathvariant override fontstyle and fontweight. layout/style/nsRuleNode.cpp appears to be a possible place to make the change, but I am unable to access both text (mathvariant) and font (fontstyle/fontweight) properties at the same time. James Kitchener (:jkitch) 2013-09-20 07:23:17 PDT Created attachment 807758 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Latin, Greek and number transforms are implemented. Arabic? ones are not. I've tested this code and it works properly, but I'm not sure if the algorithm is ideal. There are a large number of exceptions which results in a long switch statement. I'm not certain that the TEXT_IS_IN_SINGLE_CHAR_MI flag has an appropriate bit, but the reserve areas are already full and I couldn't find a user of 59. James Kitchener (:jkitch) 2013-09-20 07:24:46 PDT Created attachment 807759 [details] [diff] [review] Part 2: Parse the mathvariant, fontstyle and fontweight properties At most superficial changes since the experimental version. James Kitchener (:jkitch) 2013-09-20 07:26:26 PDT Created attachment 807760 [details] [diff] [review] Part 3: MathML frame changes James Kitchener (:jkitch) 2013-09-20 07:27:27 PDT Created attachment 807761 [details] [diff] [review] Part 4: Remoe legacy stuff Little change from the experimental patch. James Kitchener (:jkitch) 2013-09-20 07:29:16 PDT Created attachment 807763 [details] [diff] [review] Part 5: tests Tests 1 and 2 work, test 3 doesn't as the functionality is not yet implemented. What else should I be testing for? Frédéric Wang (:fredw) 2013-09-20 07:49:00 PDT Thank you James. That's great that you can take over my work on this! - (In reply to James Kitchener from comment #22) > Latin, Greek and number transforms are implemented. Arabic? ones are not. I think Arabic variants still don't have any unicode positions reserved... so let's ignore the arabic mathvariant for now. > I've tested this code and it works properly, but I'm not sure if the > algorithm is ideal. There are a large number of exceptions which results in > a long switch statement. Yes, unfortunately there are exceptions in these Unicode positions. The only thing I can think right now would be to rely on hash tables to remap these characters. But not sure if it's worth doing so, given that there are not so many exceptions. > I'm not certain that the TEXT_IS_IN_SINGLE_CHAR_MI flag has an appropriate > bit, but the reserve areas are already full and I couldn't find a user of 59. I think 59 is ok, per bug 785720 comment 18. Frédéric Wang (:fredw) 2013-09-20 07:57:05 PDT (In reply to James Kitchener from comment #25) > Created attachment 807761 [details] [diff] [review] > Part 4: Remoe legacy stuff > > Little change from the experimental patch. I think this is one of my old attempts for bug 415413. Could you please open a new bug that will block both bug 114365 and bug 415413? And extract from this patch the TEXT_FORCE_TRIM_WHITESPACE => TEXT_IS_IN_TOKEN_MATHML renaming + the changes to nsMathMLTokenFrame::ForceTrimChildTextFrames (renamed MarkTextFramesAsTokenMathML). These changes should be pretty trivial and thus easily accepted. Frédéric Wang (:fredw) 2013-09-20 08:50:31 PDT (In reply to James Kitchener from comment #26) > Created attachment 807763 [details] [diff] [review] > Part 5: tests > > Tests 1 and 2 work, test 3 doesn't as the functionality is not yet > implemented. > > What else should I be testing for? Some ideas: - default value of mathvariant for single-char mi and multi-char mi (tested by mi-mathvariant-1.xhtml) - single-char mi for unicode characters that are already italic (tested by mi-mathvariant-2.xhtml) - single-char mi for unicode characters for which there is no italic variant in Unicode (should not have any effect) - mathvariant on single-char mi (the mathvariant property should override the default italicness) - normal mathvariant mapping on mtext (mathvariant-1.html) - mathvariant exceptions on mtext (mathvariant-2.html) - mathvariant vs fontweight/fontstyle (mathvariant-3.html) - mathvariant on characters that are already in the Mathematical Alphanumeric Symbols or are exceptions (should not have any effect). - mathvariant on characters for which there is no equivalent mathvariant form in Unicode (should not have any effect) - mathvariant on multi-char token elements (should apply to all the characters) - mathvariant on mstyle (should apply to all token element descendants like single-char mi, mtext etc) Also, it might be possible that fixing this bug is all what we need to address the _moz- attribute issue in the html5lib test suite. So you may want to try Henri's patch from bug 527201. (In reply to James Kitchener from comment #21) > Created attachment 807752 [details] [diff] [review] > One thing I am stuck on is having -moz-mathvariant override fontstyle and > fontweight. layout/style/nsRuleNode.cpp appears to be a possible place to > make the change, but I am unable to access both text (mathvariant) and font > (fontstyle/fontweight) properties at the same time. I think it's not a too serious issue if you are not able to fix that. fontstyle/fontweight are deprecated and hopefully are not really used in combination with mathvariant. Also, people can set font-style and font-weight via CSS and in theory mathvariant should protect from style change too ; but this should not happen too often either. As comparison, MathJax or WebKit do not handle these cases very well. Frédéric Wang (:fredw) 2013-09-20 12:57:28 PDT Comment on attachment 807752 [details] [diff] [review] Part 0: CSS related changes Review of attachment 807752 [details] [diff] [review]: ----------------------------------------------------------------- Let's remove the Arabic mathvariant for now. What about making mathvariant a font property rather than a text property? I originally made it a text property since that looked more a character change than a style change, but I'm not quite sure what the rules are (BTW, you might want to check https://developer.mozilla.org/en-US/docs/Adding_a_new_style_property) Frédéric Wang (:fredw) 2013-09-20 13:02:51 PDT Comment on attachment 807763 [details] [diff] [review] Part 5: tests Review of attachment 807763 [details] [diff] [review]: ----------------------------------------------------------------- I haven't checked the details. You might want to check indentation and remove trailing whitespace. See also comment 29. ::: layout/reftests/mathml/mathvariant-2.html @@ +5,5 @@ > + > + > + [itex] > + > + h extra space in the attribute value Frédéric Wang (:fredw) 2013-09-20 13:19:42 PDT Comment on attachment 807760 [details] [diff] [review] Part 3: MathML frame changes Review of attachment 807760 [details] [diff] [review]: ----------------------------------------------------------------- That looks good to me. Some things to remember to remove: - in mathml.css, the _moz-math-font-style and mathvariant rules. - in mathfont.properties: the mathvariant remappings. Frédéric Wang (:fredw) 2013-09-20 23:53:46 PDT (In reply to James Kitchener from comment #26) > What else should I be testing for? Also, you could test dynamic changes: - adding/removing/modifying mathvariant on an mstyle/token element - modifying the text content of an mi element (i.e. changing single-char <=> multi-char). In particular, I'm wondering if TEXT_IS_IN_SINGLE_CHAR_MI is set/reset correctly in that case. Frédéric Wang (:fredw) 2013-09-22 04:06:26 PDT So strictly speaking, in MarkTextFramesAsTokenMathML you should only set TEXT_IS_IN_SINGLE_CHAR_MI when the nsBlockFrame child has only *one* nsTextFrame child and when this child has only one single char (modulo the whitespace trimming) James Kitchener (:jkitch) 2013-09-25 05:12:17 PDT (In reply to Frédéric Wang (:fredw) from comment #29) > Also, it might be possible that fixing this bug is all what we need to > address the _moz- attribute issue in the html5lib test suite. So you may > want to try Henri's patch from bug 527201. > I've tried a modified version of the testcase. _moz-math-columnalighn and _moz-math-rowalign are still present but there are no _moz- attributes for various combinations of fontweight, fontstyle and mathvariant. (My copy of the tree is a few days old, so it may have been fixed in the meantime). Frédéric Wang (:fredw) 2013-09-25 05:24:28 PDT (In reply to James Kitchener from comment #35) > (In reply to Frédéric Wang (:fredw) from comment #29) > I've tried a modified version of the testcase. > > _moz-math-columnalighn and _moz-math-rowalign are still present but there > are no _moz- attributes for various combinations of fontweight, fontstyle > and mathvariant. > > (My copy of the tree is a few days old, so it may have been fixed in the > meantime). Yes, these attributes will be removed when bug 731667 is fixed. I just meant that the html5lib test suite is likely to not contain any columnalign/rowalign attributes so perhaps it's not necessary to wait for bug 731667 to remove the workaround in parser_datreader.js (cf Henri's patch) and that your patch will be enough. BTW, your patch may also allow to pass test 32 of http://fred-wang.github.io/AcidTestsMathML/acid3/ Khaled Hosny 2013-09-28 05:09:43 PDT (In reply to Frédéric Wang (:fredw) from comment #27) > Thank you James. That's great that you can take over my work on this! > > - (In reply to James Kitchener from comment #22) > > Latin, Greek and number transforms are implemented. Arabic? ones are not. > > I think Arabic variants still don't have any unicode positions reserved... > so let's ignore the arabic mathvariant for now. They were added to Unicode 6.1, in the U+1EE00–U+1EEFF block. Frédéric Wang (:fredw) 2013-09-28 06:46:33 PDT (In reply to Khaled Hosny from comment #37) > > I think Arabic variants still don't have any unicode positions reserved... > > so let's ignore the arabic mathvariant for now. > > They were added to Unicode 6.1, in the U+1EE00–U+1EEFF block. Ah thanks, I was reading the MathML3 draft and didn't see any mention http://www.w3.org/Math/draft-spec/chapter3.html I'll send a mail to the MathML mailing list. Frédéric Wang (:fredw) 2013-09-29 09:50:32 PDT So the Math WG has updated the draft and added a reference to the Unicode document: http://www.unicode.org/charts/PDF/U1EE00.pdf IIUC, "Double-struck symbols 1EEA1 ب ARABIC MATHEMATICAL DOUBLE-STRUCK BEH ≈ 0628  arabic letter beh" means that ب will map to 𞺡 but I'm not really sure about those that have an additional arrow: "Isolated symbols 1EE00 ا ARABIC MATHEMATICAL ALEF → FE8D  arabic letter alef isolated form ≈ 0627  arabic letter alef" would mean that ا maps to 𞸀 or ﺍ? Khaled Hosny 2013-09-29 11:39:43 PDT (In reply to Frédéric Wang (:fredw) from comment #39) > would mean that ا maps to > 𞸀 or ﺍ? U+FE8D is a compatibility character (and so are the whole Arabic Presentation Forms A and B blocks) and should not be used for anything: http://www.unicode.org/faq/middleeast.html#1. Frédéric Wang (:fredw) 2013-09-30 01:59:42 PDT Thanks Khaled. @James: David updated the XML Entity Declarations for Characters draft, so the mapping can also be found here: http://lists.w3.org/Archives/Public/www-math/2013Sep/0012.html Regarding the tests, I'm wondering if rather than just testing a few mappings like in mathvariant-1 and mathvariant-2 we should rather have an exhaustive list, similar to what Martin Robinson started to do in WebKit: https://bugs.webkit.org/attachment.cgi?id=186282&action=diff#a/LayoutTests/mathml/presentation/mathvariant.html_sec1 James Kitchener (:jkitch) 2013-10-05 23:02:09 PDT Status update: Feedback up to comment 36 has been addressed. Revised patches will be uploaded when I am back. Support for Arabic mathvariant is blocked on actually supporting the new 1EExx characters. (I have filed bug 923890 for this). I have a plan for implementing it, but at present all it would accomplish is unreadable equations. James Kitchener (:jkitch) 2013-10-07 13:50:19 PDT (In reply to Frédéric Wang (:fredw) from bug 923890 comment #3) > OK, I see. It seems to me that this should not block the mathvariant bug, we > can just do the mapping to the plane 1 characters and expect that people > will have the appropriate Arabic fonts installed. This is true for the > double-struck or fraktur latin characters too, so I don't think the Arabic > one should be handled differently. One of the concern I have though, is for > the very common case of single-char mi like x that currently > renders correctly without math fonts but might render incorrectly if we rely > on the Math Alpha Num block instead of the style=italic style. I'm not quite > sure what would be the best solution, here. It looks like I can selectively apply font-style and font-weight within nsMathVariantTextRunFactory::RebuildTextRun, without resorting to CSS. It should work on a character by character basis, excluding inappropriate characters (e.g. non-alphanumeric symbols). I'm planning on using it for bold, italic and bold-italic mathvariants. James Kitchener (:jkitch) 2013-10-12 17:45:19 PDT According to http://www.w3.org/2003/entities/2007doc/double-struck.html, dāl (0xO62F) maps to "ARABIC MATHEMATICAL DOUBLE STRUCK DAL" (U+1EEA3) But there appears to be a similar character (ḏāl (0x0630)). Is the second one (0x0630) left unchanged or would both be transformed to the same character? Khaled Hosny 2013-10-13 00:49:16 PDT (In reply to James Kitchener (:jkitch) from comment #44) > According to http://www.w3.org/2003/entities/2007doc/double-struck.html, dāl > (0xO62F) maps to "ARABIC MATHEMATICAL DOUBLE STRUCK DAL" (U+1EEA3) > > But there appears to be a similar character (ḏāl (0x0630)). > > Is the second one (0x0630) left unchanged or would both be transformed to > the same character? 0x0630 is there near the end of the table (the fourth from below). Frédéric Wang (:fredw) 2013-10-13 11:17:03 PDT (In reply to James Kitchener (:jkitch) from comment #43) > (In reply to Frédéric Wang (:fredw) from bug 923890 comment #3) > > OK, I see. It seems to me that this should not block the mathvariant bug, we > > can just do the mapping to the plane 1 characters and expect that people > > will have the appropriate Arabic fonts installed. This is true for the > > double-struck or fraktur latin characters too, so I don't think the Arabic > > one should be handled differently. One of the concern I have though, is for > > the very common case of single-char mi like x that currently > > renders correctly without math fonts but might render incorrectly if we rely > > on the Math Alpha Num block instead of the style=italic style. I'm not quite > > sure what would be the best solution, here. > > It looks like I can selectively apply font-style and font-weight within > nsMathVariantTextRunFactory::RebuildTextRun, without resorting to CSS. It > should work on a character by character basis, excluding inappropriate > characters (e.g. non-alphanumeric symbols). I'm planning on using it for > bold, italic and bold-italic mathvariants. Karl, any idea about these bold, italic and bold-italic mathvariants? Should we follow the spec and use the characters from the math alpha num char? Or keep using CSS style to guarantee correct display even when people don't have the appropriate fonts? Khaled Hosny 2013-10-13 14:01:15 PDT (In reply to Frédéric Wang (:fredw) from comment #46) > (In reply to James Kitchener (:jkitch) from comment #43) > > (In reply to Frédéric Wang (:fredw) from bug 923890 comment #3) > > > OK, I see. It seems to me that this should not block the mathvariant bug, we > > > can just do the mapping to the plane 1 characters and expect that people > > > will have the appropriate Arabic fonts installed. This is true for the > > > double-struck or fraktur latin characters too, so I don't think the Arabic > > > one should be handled differently. One of the concern I have though, is for > > > the very common case of single-char mi like x that currently > > > renders correctly without math fonts but might render incorrectly if we rely > > > on the Math Alpha Num block instead of the style=italic style. I'm not quite > > > sure what would be the best solution, here. > > > > It looks like I can selectively apply font-style and font-weight within > > nsMathVariantTextRunFactory::RebuildTextRun, without resorting to CSS. It > > should work on a character by character basis, excluding inappropriate > > characters (e.g. non-alphanumeric symbols). I'm planning on using it for > > bold, italic and bold-italic mathvariants. > > Karl, any idea about these bold, italic and bold-italic mathvariants? Should > we follow the spec and use the characters from the math alpha num char? Or > keep using CSS style to guarantee correct display even when people don't > have the appropriate fonts? If we are considering OpenType math fonts (with MATH table) in the future, then following the spec is really the only option to get proper rendering. James Kitchener (:jkitch) 2013-10-13 18:03:21 PDT So the "single char MI without mathvariant" case should be rendered using fontstyle, everything else uses the the normal character mapping? Karl Tomlinson (:karlt) 2013-10-13 22:39:46 PDT (In reply to Khaled Hosny from comment #47) > If we are considering OpenType math fonts (with MATH table) in the future, > then following the spec is really the only option to get proper rendering. I'm not following the connection here. I didn't expect the MATH table to map bold, italic and bold-italic mathvariants. I thought mathvariant was a MathML quirk to support SMP characters using only the BMP plane, and to support these characters before math fonts were widely available. (In reply to Frédéric Wang (:fredw) from comment #46) > Karl, any idea about these bold, italic and bold-italic mathvariants? Should > we follow the spec and use the characters from the math alpha num char? Or > keep using CSS style to guarantee correct display even when people don't > have the appropriate fonts? MacOS 10.7 ships STIX, and Vista, and I assume later NT systems, have Cambria Math, so that leaves XP systems that don't have MS Office 2007 or PowerPoint Viewer, most of which systems will be unsupported after April. So, we could transform all these variants/characters to their mathematical alphanumeric symbols. But is there an advantage in doing that over using style and weight from within the Karl Tomlinson (:karlt) 2013-10-13 22:43:09 PDT ... text run transformations? Karl Tomlinson (:karlt) 2013-10-13 22:48:08 PDT Even if the advantage is just that it is simpler, that might be good enough reason. I don't feel strongly, but staying with style/weight feels safe. If code is added to support the italic case, then I assume it wouldn't be too difficult to also support bold? James Kitchener (:jkitch) 2013-10-13 23:38:35 PDT The italics case actually consists of two subcases. 1. Only single char italics are supported 2. All mathvariant=italic usage is supported The difference between the two depends on whether the text-run can consist of both transformable and non-transformable characters. Assuming case 1, adding bold support will take some effort (the same amount of effort as supporting case 2 actually), but I know what needs to be done. For case 2, supporting bold is trivial. Frédéric Wang (:fredw) 2013-10-13 23:48:04 PDT Two precisions: - In any case, I think the "single char MI without mathvariant" must be treated the same as mathvariant="italic" (otherwise the corresponding reftest will fail anyway). And that italic/bold-italic/bold must use consistent methods. - When I said "keeping CSS style", I just meant applying style/weight somewhere in the code path rather than performing a unicode change. If James is able to do that directly from within the text run transformation code, then perhaps it is better (to group the mathvariant implementation in the same place and ensure that mathvariant inhibits other CSS style/weight rules). Personally, I'd prefer to follow the specification and do something clear and consistent ; using different methods for mathvariant has always been the source of confusion. I think it is safe to think that the Desktop systems will have the appropriate fonts installed. However, my concern is for the mobile platforms like Android or FirefoxOS, given that there have not been any progress on https://code.google.com/p/android/issues/detail?id=36011 or bug 775060... Karl Tomlinson (:karlt) 2013-10-14 01:55:55 PDT Yes, thanks, mobile platforms are a good reason to use style/weight, and the idea of doing it from the transformation code sounds good to me. (In reply to James Kitchener (:jkitch) from comment #52) > The difference between the two depends on whether the text-run can consist > of both transformable and non-transformable characters. I would not expect a mix of transformable and non-transformable characters to be common, and transforming only some of the characters within one token seems odd to me. I'd be quite happy with an easy way out for this case, probably not transforming at all unless all characters should be transformed in the same way. Khaled Hosny 2013-10-14 03:56:03 PDT (In reply to Karl Tomlinson (:karlt) from comment #49) > (In reply to Khaled Hosny from comment #47) > > If we are considering OpenType math fonts (with MATH table) in the future, > > then following the spec is really the only option to get proper rendering. > > I'm not following the connection here. > I didn't expect the MATH table to map bold, italic and bold-italic > mathvariants. > > I thought mathvariant was a MathML quirk to support SMP characters using > only the BMP plane, and to support these characters before math fonts were > widely available. My understanding of using CSS styles here is that italic, bold etc. will use font styles just like regular text italic and bold. If true, this wouldn’t work with an OpenType math implementation since there is usually only a single math font with italic and bold etc. expected to be taken from the Math block, and glyphs there will have the proper italic correction, accent placement info, sub/superscript placement info and optical size variants, which all are required for proper rendering (not to mention that math italic can have different design from text italics). Frédéric Wang (:fredw) 2013-10-14 04:43:34 PDT Yes, I guess we are confronted to what to choose between the ideal situation with a single Open Type MATH and the intermediary hack using mixed fonts / styles. In the future, I think we want to avoid mixing fonts for stretchy characters. It seems that we should push to fix bugs regarding math font availability / autoinstallation or to better document how to use Web fonts. On a side note, a similar issue is that the mathml.css stylesheet contains default font-family for STIX or MathJax. If the text of the Web page is different, this may lead to inconsistency between text / math fonts ; especially the font-size difference may be significant. MathJax has hacks to try to recompute the font-size of the math elements and make it match the surrounding text, but ideally I'd like to have a clean solution here too and remove the font-family from the stylesheet in the future. Of course for all these problems, having a better support for math font would help. Karl Tomlinson (:karlt) 2013-10-14 12:53:21 PDT (In reply to Khaled Hosny from comment #55) > (In reply to Karl Tomlinson (:karlt) from comment #49) > My understanding of using CSS styles here is that italic, bold etc. will use > font styles just like regular text italic and bold. If true, this wouldn’t > work with an OpenType math implementation since there is usually only a > single math font with italic and bold etc. expected to be taken from the > Math block, and glyphs there will have the proper italic correction, accent > placement info, sub/superscript placement info and optical size variants, > which all are required for proper rendering (not to mention that math italic > can have different design from text italics). Thank you. I had forgotten that math fonts may not have italic and bold members on the font family. The font system will slant and embolden glyphs when requested, but it will not be as good as using the intended glyphs. If sites know that math fonts are being used, perhaps because the site is providing the font, they can specify the appropriate Unicode code points for the mathematical alphanumeric symbols, but it is unfortunate that these are rendered differently. I guess we could eventually check whether the primary font supports mathematical alphanumeric symbols before choosing between a unicode or style transformation, but I don't want to hold back the work here waiting for such a solution. Moving the mathvariant style selection to the textrun transformation seems a step in the right direction, which can be improved later. Raniere Silva 2013-10-14 19:59:29 PDT Created attachment 816923 [details] [diff] [review] bug-114365-testjs.patch Raniere Silva 2013-10-15 07:08:33 PDT Created attachment 817164 [details] [diff] [review] bug-114365-testjs.patch James Kitchener (:jkitch) 2013-10-22 02:59:26 PDT Created attachment 820245 [details] [diff] [review] Part 0: CSS related changes Minor changes since the last patch. Mathvariant is now a font property, rather than a text property and now overrides fontweight and fontstyle. James Kitchener (:jkitch) 2013-10-22 03:07:53 PDT Created attachment 820249 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Changes since the last patch: 1. Arabic now supported 2. bold/italic now performed through fontweight/fontstyle rather than character transformations. The code also enforces the mathvariant overriding fontweight/fontstyle behaviour. 3. A hashtable is used for Arabic and Latin lookups. Any comments before I send this off to review? James Kitchener (:jkitch) 2013-10-22 03:10:44 PDT Created attachment 820250 [details] [diff] [review] Part 2: Parse the mathvariant, fontstyle and fontweight properties fontstyle and fontweight now report a deprecation warning. James Kitchener (:jkitch) 2013-10-22 03:12:39 PDT Created attachment 820251 [details] [diff] [review] Part 3: MathML frame changes Comments have been applied James Kitchener (:jkitch) 2013-10-22 03:17:48 PDT Created attachment 820253 [details] [diff] [review] Part 4: Remove legacy stuff Correct patch this time. This mostly removes the previous mathvariant implementation. There is some rearrangements made to mathml.css which were copied from your experimental patches, but I don't think there are any behavioural differences. James Kitchener (:jkitch) 2013-10-22 03:19:43 PDT Created attachment 820255 [details] [diff] [review] Part 5: tests Dynamic and exhaustive tests have been added. Frédéric Wang (:fredw) 2013-10-22 03:34:29 PDT Comment on attachment 820245 [details] [diff] [review] Part 0: CSS related changes Review of attachment 820245 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsRuleNode.cpp @@ +3286,5 @@ > const nsCSSValue* weightValue = aRuleData->ValueForFontWeight(); > + if (aFont->mMathVariant != NS_MATHML_MATHVARIANT_NONE) { > + // -moz-math-variant overrides font-weight > + aFont->mFont.weight = NS_FONT_WEIGHT_NORMAL; > + } if (eCSSUnit_Enumerated == weightValue->GetUnit()) { do you mean } else if { here? Frédéric Wang (:fredw) 2013-10-22 04:40:46 PDT Comment on attachment 820249 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Review of attachment 820249 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsTextRunTransformations.cpp @@ +534,5 @@ > + } > + if (aMathVar > NS_MATHML_MATHVARIANT_STRETCHED) { > + // Illegal value > + return aCh; > + } So if this is never reached, should it raise some kind of assertions? @@ +540,5 @@ > + if ('A' <= aCh && aCh <= 'Z') { > + baseChar = aCh - 'A'; > + varType = kIsLatin; > + } > + else if ('a' <= aCh && aCh <= 'z') { I think here and elsewhere the coding style is } else if { (I haven't checked if this is to be consistent with the rest of the file) @@ +541,5 @@ > + baseChar = aCh - 'A'; > + varType = kIsLatin; > + } > + else if ('a' <= aCh && aCh <= 'z') { > + baseChar = MATH_BOLD_SMALL_A-MATH_BOLD_UPPER_A + aCh - 'a'; Perhaps here and elsewhere, you should explain why you consider the difference MATH_BOLD_SMALL_A-MATH_BOLD_UPPER_A. @@ +550,5 @@ > + varType = kIsNumber; > + } > + else if (GREEK_UPPER_ALPHA <= aCh && aCh <= GREEK_UPPER_OMEGA) { > + if (aCh == HOLE_GREEK_UPPER_THETA) > + // Nothing at this code point is transformed Perhaps the exceptions HOLE_GREEK_UPPER_THETA, GREEK_LETTER_DIGAMMA && aMathVar == NS_MATHML_MATHVARIANT_BOLD etc that return immediately should considered at the beginning (after the aMathVar > NS_MATHML_MATHVARIANT_STRETCHED check). That way, you won't have to bother with them in the rest of the code. @@ +561,5 @@ > + + aCh-GREEK_LOWER_ALPHA; > + varType = kIsGreekish; > + } > + else if (0x0600 <= aCh && aCh <= 0x06FF) { > + no need for this blank line. @@ +567,5 @@ > + } > + else { > + > + switch(aCh) { > + case GREEK_UPPER_THETA: bad indent. The "case" statement should be shift by 2 spaces with respect to the "switch" statement. @@ +629,5 @@ > + > + varType = kIsGreekish; > + } > + > + if (varType == kIsNumber) { Can you add a comment to explain that how the Unicode code points for digits form contiguous blocks in the order bold, double-struck etc Similarly elsewhere and how it relates to the computation of baseChar above. @@ +677,5 @@ > + } > + // exclude _NONE and _NORMAL > + return baseChar + MATH_BOLD_UPPER_ALPHA + > + multiplier*(MATH_ITALIC_UPPER_ALPHA - MATH_BOLD_UPPER_ALPHA); > + The is extra space on this blank line and after the "MATH_BOLD_UPPER_ALPHA +". @@ +680,5 @@ > + multiplier*(MATH_ITALIC_UPPER_ALPHA - MATH_BOLD_UPPER_ALPHA); > + > + } > + > + extra blank line @@ +707,5 @@ > + else { > + > + // Must be Latin > + if (aMathVar > NS_MATHML_MATHVARIANT_MONOSPACE) { > + //Latin doesn't support the arabic mathvariants missing space between "//" and "Latin" @@ +711,5 @@ > + //Latin doesn't support the arabic mathvariants > + return aCh; > + } > + multiplier = aMathVar - 2; // Offset to avoid _NONE and _NORMAL variants > + tempChar = baseChar + MATH_BOLD_UPPER_A + extra space at the end of the line @@ +716,5 @@ > + multiplier*(MATH_ITALIC_UPPER_A - MATH_BOLD_UPPER_A ); > + } > + > + if (!gMathVarCharTable) { > + InitMathVarTable(); bad indent @@ +722,5 @@ > + uint32_t newChar; > + bool found = gMathVarCharTable->Get(tempChar, &newChar); > + if (found) { > + return newChar; > + } else if ( varType == kIsLatin) { extra space before varType @@ +1100,5 @@ > + nsAutoTArray styleArray; > + nsAutoTArray canBreakBeforeArray; > + bool mergeNeeded = false; > + > + bool singleCharMI = extra space at end of line @@ +1107,5 @@ > + uint32_t length = aTextRun->GetLength(); > + const PRUnichar* str = aTextRun->mString.BeginReading(); > + nsRefPtr* styles = aTextRun->mStyles.Elements(); > + > + uint8_t mathVar; extra space at end of line @@ +1131,5 @@ > + > + if (ch == ch2 && ch != 0x20) { > + // Don't perform the transformation if a character cannot be transformed. > + // There is an exception for whitespace as it both common and innocuous. > + doTransform = false; So doTransform is only for bold/italic/bold-italic, right? If so it would need a better name. There are other space chars that could happen for example "U+00A0 no-break space" is frequent in MathJax. Perhaps it's safer and more simple to keep the current behavior and always perform the CSS style... We will fix that later when we implement mathvarian bold/italic/bold-italic correctly. @@ +1135,5 @@ > + doTransform = false; > + } > + if (mathVar == NS_MATHML_MATHVARIANT_BOLD || > + mathVar == NS_MATHML_MATHVARIANT_BOLD_ITALIC || > + mathVar == NS_MATHML_MATHVARIANT_ITALIC) { bad indent @@ +1136,5 @@ > + } > + if (mathVar == NS_MATHML_MATHVARIANT_BOLD || > + mathVar == NS_MATHML_MATHVARIANT_BOLD_ITALIC || > + mathVar == NS_MATHML_MATHVARIANT_ITALIC) { > + // Undo the change as it will be handled as a font styling. Can you open a follow-up bug to implement mathvariant with only char-transforms in the future? And mention the bug here? @@ +1140,5 @@ > + // Undo the change as it will be handled as a font styling. > + ch2 = ch; > + } > + > + if (ch2 == uint32_t(-1)) { I'm not exactly sure when this happens, but using unsigned and -1 looks weird... @@ +1223,5 @@ > + } > + > + if (mergeNeeded) { > + // Now merge multiple characters into one multi-glyph character as required > + // and deal with skipping deleted accent chars The reference to "accent chars" that you copy from the case transform routine is not relevant here. Frédéric Wang (:fredw) 2013-10-22 04:44:05 PDT Comment on attachment 820250 [details] [diff] [review] Part 2: Parse the mathvariant, fontstyle and fontweight properties Review of attachment 820250 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/mathml/content/src/nsMathMLElement.cpp @@ +652,5 @@ > + // "Specified the font style to use for the token. Deprecated in favor of > + // mathvariant." > + // > + // values: "normal" | "italic" > + // default: normal (except on ) extra space at end of line. Frédéric Wang (:fredw) 2013-10-22 04:51:33 PDT Comment on attachment 820251 [details] [diff] [review] Part 3: MathML frame changes Review of attachment 820251 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/nsMathMLTokenFrame.cpp @@ +59,1 @@ > } I think all these cases can now be grouped in one big if statement that returns eMathMLFrameType_ItalicIdentifier? @@ +65,5 @@ > { > + nsIFrame* child = nullptr; > + uint32_t childCount = 0; > + > + // Set flags on child text frames extra space at end of line @@ +68,5 @@ > + > + // Set flags on child text frames > + // - to force them to trim their leading and trailing whitespaces. > + // - Indicate which frames are suitable for mathvariant > + // - flag single charancter frames for special italic treatment typo Frédéric Wang (:fredw) 2013-10-22 04:55:22 PDT Comment on attachment 820253 [details] [diff] [review] Part 4: Remove legacy stuff Review of attachment 820253 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/mathml.css @@ +74,5 @@ > + - scriptminsize -> -moz-script-min-size > + - scriptlevel -> -moz-script-level > + - mathsize -> font-size > + - mathcolor -> color > + - mathbackground -> background} extra } Frédéric Wang (:fredw) 2013-10-22 05:00:47 PDT Comment on attachment 820255 [details] [diff] [review] Part 5: tests Review of attachment 820255 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, I haven't checked the details, but that looks a great set of tests. Can you remove the mathvariant-5.html.rej file? ::: layout/reftests/mathml/mathvariant-4.html @@ +7,5 @@ > + [itex] > + > + 𝒜 > + > + á So perhaps choose another mathvariant if we keep the current behavior for bold/italic/bold-italic. Frédéric Wang (:fredw) 2013-10-22 05:06:19 PDT Thanks James, the approach looks good to me (I'm still not sure about the best approach for part 1, though) and it's great to see mathvariant finally implemented properly and old hacks removed. I think you should ask David Baron to review part 0 and Karl Tomlinson to review part 1, as I'm less familiar about these parts of the code. James Kitchener (:jkitch) 2013-10-24 06:16:29 PDT (In reply to Frédéric Wang (:fredw) from comment #67) > Comment on attachment 820249 [details] [diff] [review] > Part 1: TextFrame and Textrun transformation changes > > Review of attachment 820249 [details] [diff] [review]: > ----------------------------------------------------------------- > There are other space chars that could happen for example "U+00A0 no-break > space" is frequent in MathJax. Perhaps it's safer and more simple to keep > the current behavior and always perform the CSS style... We will fix that > later when we implement mathvarian bold/italic/bold-italic correctly. The existing implementation ensures that only valid characters are transformed. If it was changed to transform everything, it will break layout/reftests/mathml/mi-mathvariant-2.xhtml and regress bug 413115. The robust solution is to divide the textRun into transformable and non-transformable subStrings and process them piecewise. nsFontVariantTextRunFactory::RebuildTextRun() does this so I have an example implementation from which to work. James Kitchener (:jkitch) 2013-10-24 06:30:39 PDT (In reply to Frédéric Wang (:fredw) from comment #67) > Comment on attachment 820249 [details] [diff] [review] > Part 1: TextFrame and Textrun transformation changes > > Review of attachment 820249 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +1136,5 @@ > > + } > > + if (mathVar == NS_MATHML_MATHVARIANT_BOLD || > > + mathVar == NS_MATHML_MATHVARIANT_BOLD_ITALIC || > > + mathVar == NS_MATHML_MATHVARIANT_ITALIC) { > > + // Undo the change as it will be handled as a font styling. > > Can you open a follow-up bug to implement mathvariant with only > char-transforms in the future? And mention the bug here? > I've opened 930504 for removing the fontstyle/fontweight special case. I'll also try to structure this patch so that doing so is easy. Frédéric Wang (:fredw) 2013-10-24 06:31:42 PDT (In reply to James Kitchener (:jkitch) from comment #73) > (In reply to Frédéric Wang (:fredw) from comment #67) > > Comment on attachment 820249 [details] [diff] [review] > > Part 1: TextFrame and Textrun transformation changes > > > > Review of attachment 820249 [details] [diff] [review]: > > ----------------------------------------------------------------- > > There are other space chars that could happen for example "U+00A0 no-break > > space" is frequent in MathJax. Perhaps it's safer and more simple to keep > > the current behavior and always perform the CSS style... We will fix that > > later when we implement mathvarian bold/italic/bold-italic correctly. > > The existing implementation ensures that only valid characters are > transformed. If it was changed to transform everything, it will break > layout/reftests/mathml/mi-mathvariant-2.xhtml and regress bug 413115. > > The robust solution is to divide the textRun into transformable and > non-transformable subStrings and process them piecewise. > nsFontVariantTextRunFactory::RebuildTextRun() does this so I have an example > implementation from which to work. OK, I see. So let's keep it that way until bug 930504 is fixed. But please add at least the non-breaking space   since it is mentioned on http://www.w3.org/TR/MathML/chapter2.html#fund.collapse James Kitchener (:jkitch) 2013-10-25 05:58:52 PDT Created attachment 822284 [details] [diff] [review] Part 0: CSS related changes CSS related additions to support mathvariant. -moz-math-variant is an implementation detail which is explicitly inaccessible. People wishing to determine mathvariant status can query the mathvariant attribute within a MathML element. James Kitchener (:jkitch) 2013-10-25 06:21:55 PDT Created attachment 822301 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Feedback from comment 67 has been addressed and detailed comments describing the algorithm have been added. All or nothing transformation has been implemented for bold, italic and bold-italic with an exception for whitespace (at least for the moment. They will eventually adopt the standard behaviour in bug 930504). The remaining mathvariant options transform valid characters and pass untransformable characters unchanged (in this instance it is the easy option). Should the all or nothing behaviour be adopted here as well? I've looked at the standard, and it doesn't mention this situation. James Kitchener (:jkitch) 2013-10-25 06:22:36 PDT Created attachment 822302 [details] [diff] [review] Part 2: Parse the mathvariant, fontstyle and fontweight properties James Kitchener (:jkitch) 2013-10-25 06:23:14 PDT Created attachment 822303 [details] [diff] [review] Part 3: MathML frame changes James Kitchener (:jkitch) 2013-10-25 06:23:56 PDT Created attachment 822304 [details] [diff] [review] Part 4: Remove legacy stuff James Kitchener (:jkitch) 2013-10-25 06:24:36 PDT Created attachment 822305 [details] [diff] [review] Part 5: tests Frédéric Wang (:fredw) 2013-10-25 06:26:42 PDT (In reply to James Kitchener (:jkitch) from comment #77) > Created attachment 822301 [details] [diff] [review] > The remaining mathvariant options transform valid characters and pass > untransformable characters unchanged (in this instance it is the easy > option). Should the all or nothing behaviour be adopted here as well? I've > looked at the standard, and it doesn't mention this situation. I think only the characters that can be transformed should be transformed and the others should remain unchanged. IIUC, that it is what your patch does (except for italic/bold-italic/bold for now). Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-11-22 16:04:40 PST Comment on attachment 822301 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Review of attachment 822301 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsTextFrame.h @@ +24,5 @@ > // This state bit is set on children of token MathML elements > #define TEXT_IS_IN_TOKEN_MATHML NS_FRAME_STATE_BIT(32) > > +// XXX Not in reserved space. Need to find a permanent home? > +#define TEXT_IS_IN_SINGLE_CHAR_MI NS_FRAME_STATE_BIT(59) Need to document what this flag means! Actually I don't see why we need a frame state bit here. Can you explain that? ::: layout/generic/nsTextRunTransformations.cpp @@ +312,5 @@ > > +/* > + Entries for the mathvariant hash table. The data is divided into pairs. > + The first entry in each pair represents the hashtable key, the second > + contains the value. Please describe what the keys and values actually mean! It seems simpler and more efficient just to binary-search the table in-place (and make it pre-sorted) instead of building the hash table. Squeezing out every last drop of performance here just isn't worth it. @@ +315,5 @@ > + The first entry in each pair represents the hashtable key, the second > + contains the value. > + Table must be terminated with an 0 to avoid an infinite loop. > +*/ > +static uint32_t mathVarMappingTable[] = { static const @@ +1120,5 @@ > } > > void > +nsMathVariantTextRunFactory::RebuildTextRun(nsTransformedTextRun* aTextRun, > + gfxContext* aRefContext) I'd like to put nsMathVariantTextRunFactory into its own file(s) please. Cameron McCormack (:heycam) 2013-11-22 17:56:22 PST Comment on attachment 822284 [details] [diff] [review] Part 0: CSS related changes Review of attachment 822284 [details] [diff] [review]: ----------------------------------------------------------------- I think we want to treat this new property like the MathML properties in that if MathML is disabled, we want to be able to pretend that a whole nsStyleFont's worth of properties was specified even if -moz-math-variant wasn't. So please add a check for this new property in AreAllMathMLPropertiesUndefined in nsRuleNode.cpp. r=me with that and the comments below addressed. ::: layout/style/nsCSSPropList.h @@ +3352,5 @@ > + VARIANT_HK, > + kMathVariantKTable, > + CSS_PROP_NO_OFFSET, > + eStyleAnimType_None) > +#endif // !defined(CSS_PROP_LIST_EXCLUDE_INTERNAL) Please place this within the !defined(CSS_PROP_LIST_ONLY_COMPONENTS_OF_ALL_SHORTHAND) block just above so that an "all: reset" property doesn't reset -moz-math-variant. ::: layout/style/nsRuleNode.cpp @@ +3249,5 @@ > + // -moz-math-variant: enum, inherit, initial > + SetDiscrete(*aRuleData->ValueForMathVariant(), aFont->mMathVariant, > + aCanStoreInRuleTree, > + SETDSC_ENUMERATED, aParentFont->mMathVariant, > + NS_MATHML_MATHVARIANT_NONE, 0, 0, 0, 0); Since this is an inherited property, use the SETDSC_UNSET_INHERIT flag so that "unset" gets handled like "inherit". ::: layout/style/nsStyleConsts.h @@ +519,5 @@ > // defaults per MathML spec > #define NS_MATHML_DEFAULT_SCRIPT_SIZE_MULTIPLIER 0.71f > #define NS_MATHML_DEFAULT_SCRIPT_MIN_SIZE_PT 8 > > +// See nsStyleText nsStyleFont ::: layout/style/nsStyleStruct.cpp @@ +90,5 @@ > nsStyleFont::nsStyleFont(const nsFont& aFont, nsPresContext *aPresContext) > : mFont(aFont) > , mGenericID(kGenericFont_NONE) > , mExplicitLanguage(false) > + , mMathVariant(NS_MATHML_MATHVARIANT_NONE) How about we initialize mMathVariant inside nsStyleFont::Init, like the other MathML properties are. Frédéric Wang (:fredw) 2013-11-23 01:44:30 PST (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #83) > ::: layout/generic/nsTextFrame.h > @@ +24,5 @@ > > // This state bit is set on children of token MathML elements > > #define TEXT_IS_IN_TOKEN_MATHML NS_FRAME_STATE_BIT(32) > > > > +// XXX Not in reserved space. Need to find a permanent home? > > +#define TEXT_IS_IN_SINGLE_CHAR_MI NS_FRAME_STATE_BIT(59) > > Need to document what this flag means! > > Actually I don't see why we need a frame state bit here. Can you explain > that? > Concretely, for most token frames the mapping @mathvariant to -moz-mathvariant is straightforward: x => normal x => fraktur except for whose default value is a bit special: x => italic x => italic sin => normal that is the default is "italic" for single-char (with whitespaces trimmed) and "normal" for multi-char . The logic is slightly more involved so I'm not sure we can do that in mathml.css or in nsMathMLElement::MapMathMLAttributesInto. Hence I've suggested to James to set a frame bit in nsMathMLTokenFrame::MarkTextFramesAsTokenMathML (see attachment 822303 [details] [diff] [review]) where we already set the TEXT_IS_IN_TOKEN_MATHML bit. > ::: layout/generic/nsTextRunTransformations.cpp > @@ +312,5 @@ > > > > +/* > > + Entries for the mathvariant hash table. The data is divided into pairs. > > + The first entry in each pair represents the hashtable key, the second > > + contains the value. > > Please describe what the keys and values actually mean! > > It seems simpler and more efficient just to binary-search the table in-place > (and make it pre-sorted) instead of building the hash table. Squeezing out > every last drop of performance here just isn't worth it. > Yes, I don't know why I suggest that. Sorry, James! James Kitchener (:jkitch) 2013-11-26 00:05:10 PST Created attachment 8338329 [details] [diff] [review] Part 0: CSS related changes Review comments addressed. James Kitchener (:jkitch) 2013-11-26 00:09:21 PST Created attachment 8338333 [details] [diff] [review] Part 0: CSS related changes Overlooked something. Cameron McCormack (:heycam) 2013-11-26 00:14:10 PST Comment on attachment 8338333 [details] [diff] [review] Part 0: CSS related changes >diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h >--- a/layout/style/nsStyleStruct.h >+++ b/layout/style/nsStyleStruct.h >@@ -109,16 +109,17 @@ public: > // size on this nsStyleFont? > bool mAllowZoom; // [inherited] > > // The value mSize would have had if scriptminsize had never been applied > nscoord mScriptUnconstrainedSize; > nscoord mScriptMinSize; // [inherited] length > float mScriptSizeMultiplier; // [inherited] > nsCOMPtr mLanguage; // [inherited] >+ uint8_t mMathVariant; // [inherited] > }; Sorry, should have mentioned this before: do you mind moving mMathVariant just beneath mScriptLevel to reduce the size of an nsStyleFont (because of alignment of its members)? James Kitchener (:jkitch) 2013-11-26 00:20:16 PST Created attachment 8338340 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Review comments have been addressed. The hashtable has been converted into several arrays which are selected as needed. This removes the need to encode the data, ensuring that all values actually represent unicode character points. James Kitchener (:jkitch) 2013-11-26 00:30:27 PST Created attachment 8338347 [details] [diff] [review] Part 0: CSS related changes Done James Kitchener (:jkitch) 2013-11-26 00:31:11 PST Created attachment 8338348 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Assorted whitespace fixes. James Kitchener (:jkitch) 2013-11-26 00:36:21 PST Created attachment 8338352 [details] [diff] [review] Part 5: tests This now includes a fix for mpadded-7, mpadded-8 and mpadded-9. Characters such as '|' and '_' are no longer transformed by mathvariant, breaking the test. It now uses style="font-family: monospace" which should be equivalent to the older mathvariant implementation. I've also added tests for mathvariant="monospace" (using 'i' and 'X', unless you can think of better representative characters). Frédéric Wang (:fredw) 2013-11-26 05:42:35 PST Created attachment 8338470 [details] [diff] [review] Part 0: CSS related changes Updated patch from James to fix a build error. Frédéric Wang (:fredw) 2013-11-26 05:43:47 PST Created attachment 8338471 [details] [diff] [review] Part 4: Remove legacy stuff Refresh patch + remove _moz_fontstyle from GkAtomList.h Frédéric Wang (:fredw) 2013-11-26 07:20:16 PST Results from James: https://tbpl.mozilla.org/?tree=Try&rev=f1bd8d4d4f85 So the new assertions seems to be due to the verification added in AreAllMathMLPropertiesUndefined. I believe the SetDiscrete on mathvariant will set it not the MATHVARIANT_NONE even when MathML is not used. As a workaround, I also allowed aRuleData->ValueForMathVariant()->GetUnit() == eCSSUnit_Initial but perhaps there is a best way to fix that. Frédéric Wang (:fredw) 2013-11-26 09:20:58 PST Comment on attachment 822303 [details] [diff] [review] Part 3: MathML frame changes Review of attachment 822303 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/nsMathMLTokenFrame.cpp @@ +46,5 @@ > + mathVariant == NS_MATHML_MATHVARIANT_ITALIC || > + mathVariant == NS_MATHML_MATHVARIANT_ITALIC || > + mathVariant == NS_MATHML_MATHVARIANT_BOLD_ITALIC || > + mathVariant == NS_MATHML_MATHVARIANT_SCRIPT || > + mathVariant == NS_MATHML_MATHVARIANT_BOLD_SCRIPT || So the remaining failing test on Linux for attachment 8338560 [details] seems to be due to the fact that MATHVARIANT_SCRIPT and MATHVARIANT_BOLD_SCRIPT are considered ItalicIdentifier rather than upright. Frédéric Wang (:fredw) 2013-11-26 13:35:06 PST https://tbpl.mozilla.org/?tree=Try&rev=51ab041e5ec1 Cameron McCormack (:heycam) 2013-11-26 14:50:54 PST (In reply to Frédéric Wang (:fredw) from comment #95) > So the new assertions seems to be due to the verification added in > AreAllMathMLPropertiesUndefined. I believe the SetDiscrete on mathvariant > will set it not the MATHVARIANT_NONE even when MathML is not used. As a > workaround, I also allowed aRuleData->ValueForMathVariant()->GetUnit() == > eCSSUnit_Initial but perhaps there is a best way to fix that. Oh, you also need to skip that property in nsInitialStyleRule::MapRuleInfoInto (in nsStyleSet.cpp) if MathML is disabled. Frédéric Wang (:fredw) 2013-11-28 06:50:53 PST Comment on attachment 8338470 [details] [diff] [review] Part 0: CSS related changes >+CSS_PROP_FONT( >+ -moz-math-variant, >+ _moz_math_variant, >+ MathVariant, For consistency with the other MathML properties (scriptminsize etc), shouldn't this be -moz-math-variant, math_variant, MathVariant, (without the _moz prefix)? James Kitchener (:jkitch) 2013-11-29 00:36:13 PST Created attachment 8340261 [details] [diff] [review] Part 0: CSS related changes Addresses comments 98 and 99. James Kitchener (:jkitch) 2013-11-29 00:37:36 PST Created attachment 8340264 [details] [diff] [review] Part 3: MathML frame changes Addresses comment 96. Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-11-29 06:00:12 PST Comment on attachment 8338348 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Review of attachment 8338348 [details] [diff] [review]: ----------------------------------------------------------------- Mostly cosmetic comments. Just about done :-) ::: layout/generic/MathVariantTextRunFactory.cpp @@ +3,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + > +#include "MathVariantTextRunFactory.h" Unnecessary blank line @@ +7,5 @@ > +#include "MathVariantTextRunFactory.h" > +#include "nsStyleConsts.h" > +#include "nsStyleContext.h" > +#include "nsTextFrameUtils.h" > +/* Blank line after #includes @@ +18,5 @@ > + > +struct MathVarMapping > +{ > + uint32_t key; > + uint32_t replacement; mKey, mReplacement @@ +34,5 @@ > + As a replacement, 0 is reserved to indicate no mapping was found. > +*/ > +#define ARABICINITIALSIZE 20 > +static const MathVarMapping arabicInitialMapTable[ARABICINITIALSIZE] = { > + { 0x628, 0x1EE21 }, gArabicInitialMapTable (same below) @@ +191,5 @@ > + > +// Finds a MathVarMapping struct with the specified key (aKey) within aTable. > +// aTable must be an array, whose length is specified by aNumElements > +static uint32_t > +mathvarMappingSearch(uint32_t aKey, const MathVarMapping* aTable, uint32_t aNumElements) MathvarMappingSearch @@ +199,5 @@ > + while (high > low) { > + uint32_t midPoint = (low+high) >> 1; > + if (aKey == aTable[midPoint].key) { > + return aTable[midPoint].replacement; > + } else if (aKey > aTable[midPoint].key) { No need for "else" after return. @@ +255,5 @@ > +#define MATH_BOLD_PI_SYMBOL 0x1D6E1 > + > + > +static uint32_t > +MathVariant(uint32_t aCh, uint8_t aMathVar) Document what this function does @@ +258,5 @@ > +static uint32_t > +MathVariant(uint32_t aCh, uint8_t aMathVar) > +{ > + > + uint32_t baseChar; Delete blank line @@ +275,5 @@ > + return aCh; > + } > + if (aMathVar > NS_MATHML_MATHVARIANT_STRETCHED) { > + NS_ASSERTION(aMathVar <= NS_MATHML_MATHVARIANT_STRETCHED, > + "Illegal mathvariant value"); Just NS_ERROR since we know the condition is false @@ +375,5 @@ > + varType = kIsGreekish; > + } > + > + if (varType == kIsNumber) { > + switch(aMathVar) { Space before ( (same below) @@ +447,5 @@ > + * lookup table. > + */ > + case NS_MATHML_MATHVARIANT_INITIAL: > + mapTable = arabicInitialMapTable; > + numElements = ARABICINITIALSIZE; Instead of using a #define here, just do "numElements = ArrayLength(gArabicInitialMapTable);" (from mfbt/Utils.h). @@ +472,5 @@ > + } > + newChar = mathvarMappingSearch(aCh, mapTable, numElements); > + } else { > + > + // Must be Latin Remove blank line @@ +484,5 @@ > + // characters are located within their unicode block (less an offset to > + // avoid _NONE and _NORMAL variants) > + // See the kIsNumber case for an explanation of the following calculation > + tempChar = baseChar + MATH_BOLD_UPPER_A + > + multiplier*(MATH_ITALIC_UPPER_A - MATH_BOLD_UPPER_A ); Remove space before ) @@ +488,5 @@ > + multiplier*(MATH_ITALIC_UPPER_A - MATH_BOLD_UPPER_A ); > + // There are roughly twenty characters that are located outside of the > + // mathematical block, so so the spaces where they ought to be are used > + // as keys for a lookup table containing the correct character mappings. > + newChar = mathvarMappingSearch(tempChar, latinExceptionMapTable, LATINEXCEPTIONSIZE); Use ArrayLength here too @@ +528,5 @@ > + bool doMathvariantStyling = true; > + > + for (uint32_t i = 0; i < length; ++i) { > + > + int extraChars = 0; Remove blank line here @@ +549,5 @@ > + mathVar == NS_MATHML_MATHVARIANT_BOLD_ITALIC || > + mathVar == NS_MATHML_MATHVARIANT_ITALIC) { > + if (ch == ch2 && ch != 0x20 && ch != 0xA0) { > + // Don't perform the transformation if a character cannot be > + // transformed. There is an exception for whitespace as it both common "as it is" @@ +649,5 @@ > + // the data would break the cache. > + aTextRun->ResetGlyphRuns(); > + aTextRun->CopyGlyphDataFrom(child, 0, child->GetLength(), 0); > + } > + Delete blank line James Kitchener (:jkitch) 2013-11-30 16:36:34 PST Created attachment 8340708 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Comments addressed. Also some minor whitespace and comment improvements. James Kitchener (:jkitch) 2013-11-30 16:50:46 PST https://tbpl.mozilla.org/?tree=Try&rev=a703caf18ba6 One test failure specific to Android 2.2 I suspect there is some difference in the bounding box or font metric calculations between style="font-style:italic" and the italic fontstyle set by the TextRunTransformation. Is this something worth chasing up? The test will eventually need changing to avoid using explicit font-style when bug 930504 lands, as style="font-style:italic" and mathvariant="italic" will likely give different results. Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-01 02:20:03 PST Comment on attachment 8340708 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Review of attachment 8340708 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: layout/generic/MathVariantTextRunFactory.cpp @@ +295,5 @@ > + if (aCh == GREEK_LETTER_DIGAMMA) { > + if (aMathVar == NS_MATHML_MATHVARIANT_BOLD) > + return MATH_BOLD_CAPITAL_DIGAMMA; > + else > + return aCh; Don't do "else" after "return". Just "return aCh;" after the "if" statement. Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-01 02:24:30 PST Curious that it's Android-only. (In reply to James Kitchener (:jkitch) from comment #104) > I suspect there is some difference in the bounding box or font metric > calculations between style="font-style:italic" and the italic fontstyle set > by the TextRunTransformation. But only on Android, which is odd. > Is this something worth chasing up? I think it's worth spending a little bit of time examining the possibilities. But not too much. > The test will eventually need changing to avoid using explicit font-style > when bug 930504 lands, as style="font-style:italic" and mathvariant="italic" > will likely give different results. In that case, it seems reasonable to modify the test appropriately now. James Kitchener (:jkitch) 2013-12-01 05:05:15 PST Created attachment 8340762 [details] [diff] [review] Part 1: TextFrame and Textrun transformation changes Comment 105 addressed. (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away Nov 26-29) from comment #106) > But only on Android, which is odd. > This isn't the first time such behaviour has appeared in nsMathMLmmultiscriptsFrame related tests. Bug 827713 had one which would only appear with OSX10.6 (10.7+ were fine). I'll upload the test changes after a try run. Karl Tomlinson (:karlt) 2013-12-01 11:54:40 PST (In reply to James Kitchener (:jkitch) from comment #104) > https://tbpl.mozilla.org/?tree=Try&rev=a703caf18ba6 > > One test failure specific to Android 2.2 > > I suspect there is some difference in the bounding box or font metric > calculations between style="font-style:italic" and the italic fontstyle set > by the TextRunTransformation. I wonder whether the italic and normal fonts have different ascent/descent metrics on Android. If the font-style is set only in the TextRunTransformation, then that won't change the logical font metrics of the element, which are based on the element font-style, which is normal. That is OK, I think, but it would be nice to make the test continue to work. > The test will eventually need changing to avoid using explicit font-style > when bug 930504 lands, as style="font-style:italic" and mathvariant="italic" > will likely give different results. I wouldn't expect style="font-style:italic to help, but mathvariant="italic" in 414123-ref.xhtml should make it pass. James Kitchener (:jkitch) 2013-12-02 01:10:13 PST Created attachment 8340962 [details] [diff] [review] Part 5: tests Now fixes layout/reftests/mathml/414123 by comparing it with an explicit mathvariant, rather than faking it with font-style. Green try run https://tbpl.mozilla.org/?tree=Try&rev=a3143918816e Ryan VanderMeulen [:RyanVM] 2013-12-02 09:05:03 PST https://hg.mozilla.org/integration/mozilla-inbound/rev/ab78b049fa04 https://hg.mozilla.org/integration/mozilla-inbound/rev/67e38708a5e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/b95c0c041869 https://hg.mozilla.org/integration/mozilla-inbound/rev/512193946706 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5226495f47 https://hg.mozilla.org/integration/mozilla-inbound/rev/a3037e75d1dd Ryan VanderMeulen [:RyanVM] 2013-12-02 13:53:12 PST https://hg.mozilla.org/mozilla-central/rev/ab78b049fa04 https://hg.mozilla.org/mozilla-central/rev/67e38708a5e9 https://hg.mozilla.org/mozilla-central/rev/b95c0c041869 https://hg.mozilla.org/mozilla-central/rev/512193946706 https://hg.mozilla.org/mozilla-central/rev/6a5226495f47 https://hg.mozilla.org/mozilla-central/rev/a3037e75d1dd James Kitchener (:jkitch) 2013-12-02 17:47:55 PST Documentation changes: mathvariant is now fully implemented. It works by replacing the characters within the tag by new ones that correspond to the original character and the mathvariant selected which is located within the mathematical blocks of unicode. Each transformable character only supports a subset of the possible mathvariants. Characters without a valid representation are displayed unaltered. The lists of transformable characters and the mathvariant values they accept can be found at http://www.w3.org/TR/MathML2/chapter6.html#chars.letter-like-tables http://lists.w3.org/Archives/Public/www-math/2013Sep/0012.html A warning might be needed to say that users without fonts supporting the mathematical blocks (0x1D*** and 0x1EE**) will see missing character symbols instead. Note there is a special case for bold, italic and bold-italic. These are still implemented using font-style/fontweight to allow them to remain readable even with fonts lacking the mathematical blocks. The treatment of non-transformable characters is also handled differently. If a non-transformable character is found within the tag (other than certain whitespace characters), the transformation will not occur for any of the characters. This behaviour will eventually be standardised to follow the majority behaviour in bug 930504. Daniel Veditz [:dveditz] 2013-12-03 11:39:52 PST Jesse: please make sure this feature is covered by the MathML fuzzer. Frédéric Wang (:fredw) 2013-12-04 02:41:29 PST (In reply to Frédéric Wang (:fredw) from comment #36) > BTW, your patch may also allow to pass test 32 of http://fred-wang.github.io/AcidTestsMathML/acid3/ FYI, I just checked that, and indeed we obtained one point more the MathML Acid 3 test! Jesse Ruderman 2013-12-05 12:23:11 PST > Jesse: please make sure this feature is covered by the MathML fuzzer. The attributes {mathvariant, fontweight, fontstyle} are already tested by my MathML-generation module. I was missing a few values for mathvariant, so I added those (fuzzing rev cbe72a97462a). Frédéric Wang (:fredw) 2013-12-06 12:47:53 PST (In reply to James Kitchener (:jkitch) from comment #112) > Documentation changes: > > A warning might be needed to say that users without fonts supporting the mathematical blocks (0x1D*** and 0x1EE**) will see missing character symbols instead. I believe all the non-Arabic characters are already covered by the STIX fonts. Khaled Hosny is working on creating glyphs for his Amiri and XITS fonts (https://github.com/khaledhosny/xits-math/issues/36#issuecomment-29606565), so hopefully they will be available when Gecko 28 is released. We'll have to make these Arabic fonts available on MDN, in the font installers etc (bug 923890). Frédéric Wang (:fredw) 2014-05-07 08:12:57 PDT So after some delay, I finally updated the MDN doc. The relnote was https://www.mozilla.org/en-US/firefox/28.0/releasenotes/ These had already been updated: https://developer.mozilla.org/en-US/Firefox/Releases/28#MathML https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/Status Additionally, I updated the info on the token elements pages: https://developer.mozilla.org/en-US/docs/Web/MathML/Element/mtext https://developer.mozilla.org/en-US/docs/Web/MathML/Element/ms https://developer.mozilla.org/en-US/docs/Web/MathML/Element/mo https://developer.mozilla.org/en-US/docs/Web/MathML/Element/mn https://developer.mozilla.org/en-US/docs/Web/MathML/Element/mi and two mathvariant tests on https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML3Testsuite/Topics/TablesTokens I think this was the minimum needed, but feel free to update more pages if necessary.

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