Bob Mathews reports that MathType is not able to generate <mmultiscripts> <mi>F</mi> <mn>1</mn> <mprescripts/> <mn>1</mn> </mmultiscripts> correctly and so uses <mmultiscripts> <mi>F</mi> <mprescripts/> <mn>1</mn> <none/> </mmultiscripts> <msub> <mrow></mrow <mi>1</mi> </msub> instead. In Gecko the two markups do not render the same, but they do in MathPlayer/MathJax. Also, if you replace <msub> by a <msubsup> with an empty supscript, then it renders the same in Gecko as the <mmultiscripts> tag. I attach Bob's original testcase. In general, this seems to be the counterpart of bug 669932. I actually already noticed in bug 668204 comment 7 that there were inconsistent rendering of mub / mup / msubsup too. I attach another testcase that includes the mmultiscripts element. Note that the invalid markup in the mmultiscripts without scripts is due to bug 823939. If someone wants to work on this, the relevant files to look at are: layout/mathml/nsMathMLmsupFrame.cpp layout/mathml/nsMathMLmsubFrame.cpp layout/mathml/nsMathMLmsubsupFrame.cpp layout/mathml/nsMathMLmmultiscriptsFrame.cpp I suspect that as in bug 669932, the solution would be to merge all these frames in one nsMathMLmmultiscriptsFrame. That will give a consistent behavior and less code. It's certainly possible to merge the three first frames in one nsMathMLmsubsupFrame, but the nsMathMLmmultiscriptsFrame would probably need more care. I put this as a mentored bug, but note that there are probably too much things to modify for this to be a good first bug. However, that's a good candidate for someone who is already familiar with Mozilla's development process and wants to work on a bug of medium difficulty.

Here is a testcase. It should be fairly easy to convert it into a reftest. See https://developer.mozilla.org/en/Creating_reftest-based_unit_tests for the reftest documentation and layout/reftests/mathml/munderover-empty-scripts.html layout/reftests/mathml/munderover-empty-scripts-ref.html for the munderover version.

Status update: I've merged msub, msup, msubsup and mmultiscripts. The unified code works correctly for the reftest based on attachment 699027 [details] (multiscripts-1.html). Next task is to get the reftest based on the code in the description to match.

This fixes the horizontal difference between the two testcases in the description. There is still a small vertical discrepancy. Correcting this is proving to be tricky. > Also, if you replace <msub> by a <msubsup> with an empty supscript, then it renders the same in Gecko as the <mmultiscripts> tag. I attach Bob's original testcase. I'm not able able to replicate this in an unpatched nightly build. On maximum zoom I observe a small vertical difference.

> > Also, if you replace <msub> by a <msubsup> with an empty supscript, then it renders the same in Gecko as the <mmultiscripts> tag. I attach Bob's original testcase. > > I'm not able able to replicate this in an unpatched nightly build. On > maximum zoom I observe a small vertical difference. Right, I also see a small difference. But the difference between msub & mmultiscript is much more obvious.

It seems that our minimal script shifts is also inconsistent. In the attached testcase, I would expect the top of the blue square to be aligned with the bottom of the green square and the bottom of the blue square to be aligned with the top of the red square. That seems to be the case for msub but not for msup/msubsup. Does the wip patch improve the situation here?

There was a bug in the wip patch preventing scriptshifts from working at all. After fixing it there was no change in msup and msubsup behaviour. msub no longer works (it now behaves like the subscript rectangle of msubsup). I'll add it to the list of things to fix.

Updated testcase as the original one had incorrect attribute names for superscriptshift. I've isolated the code causing the testcase to break. For subscript calculation in msubsup and mmultiscripts (similar code exists for superscripts): > float scaler = ((float) subScriptShift2) / subScriptShift1; > subScriptShift1 = std::max(subScriptShift1, aUserSubScriptShift); > subScriptShift2 = NSToCoordRound(scaler * subScriptShift1); > subScriptShift = std::max(subScriptShift1, subScriptShift2); We generate a ratio of the font's default scriptshifts and then apply it to the user specified one. To fix this, I have removed the scaling factor and instead choose the maximum of the default and user scriptshifts. This makes the testcase behave as expected. However, I am not certain that this fix is valid as I do not understand why the scaling was initially introduced.

First, I think it would be helpful to find a TeXBook e.g. at a library, so that we can understand what is done and check whether or not there are programming errors. I don't understand this scaling either, but I would expect that the user preference overrides the TeX behavior, so taking the maximum seems to make sense to me. Next, it is indicated in comments that, for example, the sub shift of msubsup is different from the sub shift of msub. This is why I suspect merging should be done with care. What I propose is to determine whether the supscript of msubsup is empty (perhaps by checking whether supScriptSize or bmSupScript have null dimensions) and if so handle an msubsup as an msub. More generally, by checking the number of script children and whether they are empty or not, you can determine whether mmultiscripts should be interpreted as msubsup, msub or msup and whether msubsup should be interpreted as msup or msub. If all the scripts are empty, perhaps treat the element as an mrow. I'm not sure that will solve Bob's original testcase [*], but that will make our handling of elements consistent and preserve the TeX rules. ([*] I think MathType should really generate an mmultiscripts. Neil confirmed in another mail that even in MathPlayer, the alignment is not guaranteed as the default shifts depends on many parameters. However, if we make user shifts override the default, perhaps MathType could use the script elements with user shifts to guarantee alignment)

(In reply to Frédéric Wang (:fredw) from comment #8) (perhaps by checking whether supScriptSize or bmSupScript have null dimensions) In munderover, this is done by testing the ink metrics bmUnder and bmOver. So I guess here you shoud use bmSupScript, bmSubScript etc.

The patch follows the suggestion in comment 8 and improves compliance of the TeX rendering rules. There may be an exception with asymmetric multiscripts. At the moment, a multiscript with one presubscript and one (post) supscript will be considered as a combined sub and supscript, rather than handling them independently. Is this acceptable? The testcase in the description works for subscripts, but not for superscripts. This is a consequence of following rule 18a from Appendix G of the TexBook. Major issue: I seem to have broken the bounding box. Subsequent elements are rendered on top of the script element. I'm still trying to find the cause.

(In reply to James Kitchener from comment #10) > The patch follows the suggestion in comment 8 and improves compliance of the > TeX rendering rules. There may be an exception with asymmetric > multiscripts. At the moment, a multiscript with one presubscript and one > (post) supscript will be considered as a combined sub and supscript, rather > than handling them independently. Is this acceptable? I guess this is acceptable, but this does not correspond to any msubsup/msub/msup construction, so I would expect the current behavior of mmultiscripts to be preserved? > > The testcase in the description works for subscripts, but not for > superscripts. This is a consequence of following rule 18a from Appendix G > of the TexBook. That's what I suspected. As I said in comment 8, I think that's fine and MathType should better handle the mmultiscripts. > Major issue: > I seem to have broken the bounding box. Subsequent elements are rendered on > top of the script element. I'm still trying to find the cause. Thanks for the info. I'm looking forward to hearing more about your progress. Keep up the good work!

The problems in the previous patch have been resolved. I've tested it against the layour reftests, and there are no additional failures.

I just tried your patch on my MathML Acid3test: https://github.com/fred-wang/AcidTestsMathML There was an error in test 18 that I just fixed and now the sub/supscriptshift seems to work correctly with your patch. However, I suspect your patch breaks tests 34, 35, 36, 37 and 38. Can you check that your patch is doing the right thing with respect to scriptlevel and displaystyle? (see the descriptions of scripts in the chapter 3 of the MathML spec)

Tests 34-38 now pass.

Comment on attachment 718881 [details] [diff] [review] unify sub/sup scripts with correct behaviour for mover/under Review of attachment 718881 [details] [diff] [review]: ----------------------------------------------------------------- I'm wondering if multiscripts-2 is really relevant. Bob expected that putting two script elements on the same line should be the same as putting only one script element, but I'm not sure we can assume that in general. The only thing we want to verify is that the rendering of a script element with empty scripts is the same as the equivalent script element, and this is already tested in multiscript-1. However, I think we should add tests for the errors that were not detected by our current test suite, namely script shifts and display style. When you move or modify some code, don't hesitate to take the opportunity to fix the coding style. However, try not doing whitespace change only as this mess up the change history. I haven't verified in details the changes to Place yet, but I already give you partial feedback. ::: layout/mathml/nsMathMLmmultiscriptsFrame.cpp @@ +45,3 @@ > // displaystyle to "false", within each of its arguments except base > + UpdatePresentationDataFromChildAt(1, -1, ~NS_MATHML_DISPLAYSTYLE, > + NS_MATHML_DISPLAYSTYLE); Coding style: don't do whitespace change only ; Also it seems you have Windows carriage return "\r\n" instead of Unix "\n" (here and elsewhere). @@ +54,5 @@ > + if (mContent->Tag() == nsGkAtoms::msup_) > + isSubScript = false; > + else > + isSubScript = true; > + msup does not have any subscript so the work below is not necessary and you can return NS_OK immediately. Then you could just initialize isSubScript to true. @@ +59,5 @@ > nsAutoTArray<nsIFrame*, 8> subScriptFrames; > nsIFrame* childFrame = mFrames.FirstChild(); > while (childFrame) { > + nsIAtom* tag = childFrame->GetContent()->Tag(); > + if (tag == nsGkAtoms::mprescripts_) { I don't think this change is necessary? @@ +84,4 @@ > childFrame = subScriptFrames[i]; > PropagatePresentationDataFor(childFrame, > NS_MATHML_COMPRESSED, NS_MATHML_COMPRESSED); > } OK, I'm not sure why we collect subscripts in an array first and then do the propagation in the reverse child order (it seems to me that we could do that directly in the loop). But let's keep the current code... @@ +142,2 @@ > } > + // scriptspace from TeX for extra spacing after sup/subscript (0.5pt in plain TeX) coding style: line length of at most 80 chars. @@ +154,4 @@ > } > > +// exported routine that both moverunder and mmultiscripts share. > +// moverunder uses this when movablelimits is set. It's called munderover (fix that elsewhere too). @@ +191,5 @@ > + bool isSubScript; > + if (tag == nsGkAtoms::msup_) > + isSubScript = false; > + else > + isSubScript = true; This could simply be bool isSubScript = (tag != nsGkAtoms::msup_); also you could add a comment before: // Boolean to determine whether the current child is a subscript. // Note that only msup starts with a superscript. @@ +195,5 @@ > + isSubScript = true; > + > + // Examine the structure of the script to see which rules from AppG TexBook > + // should apply to the element > + // eg. A tag with the structure the latin abbreviation is "e.g." with two dots (fix elsewhere too) @@ +199,5 @@ > + // eg. A tag with the structure > + // <msubsup> > + // <mn>4</mn> > + // <mn>7</mn> > + // <none/> I don't think the RelaxNG schema allows <none/> as a child of <msubsup>. I guess that should be replaced by an <mrow></mrow> in this example. @@ +214,5 @@ > + } > + return aFrame->ReflowError(aRenderingContext, aDesiredSize); > + } > + prescriptsFrame = childFrame; > + } The error should only happen with mmultiscripts, but here it will work for <msubsup> <mprescripts/> <mprescripts/> </msubsup> or even <msub> <mtext>a</mtext> <mprescripts/> <mtext>a</mtext> <mprescripts/> </msub> I think you should report an error if you find a mprescripts/none element outside a mmultiscripts (that is you are in msubsup, munderover or other the variants). See bug 553917 and bug 823939 about how to do that. Perhaps a generic message "Invalid Markup: <%1$S> is not allowed as a child of <%1$S>" @@ +263,5 @@ > + } > + } > + return aFrame->ReflowError(aRenderingContext, aDesiredSize); > + } > + I think the width variable is not necessary in this first phase and using it to detect the sup-sub pairs is a bit confusing. Actually, the old code was already incorrect, for example it accepts <math> <mmultiscripts> <mtext>base</mtext> <mtext>c</mtext> <mprescripts/> <mtext>a</mtext> </mmultiscripts> </math> and treats it as if <mprescripts/> was not here. What about just breaking the loop when you arrive on a mprescripts and isSubScript is true? Then script pair does not match iff isSubScript is true and the error condition will become if (count != 2 && (tag == nsGkAtoms::msup_ || tag == nsGkAtoms::msub_) || count != 3 && tag == nsGkAtoms::msubsup_ || (!baseFrame || count > 1 && isSubScript) @@ +266,5 @@ > + } > + > + // Note that the equiv tags are overloaded. mmultiscripts_ also > + // represents msubsup_, and the subsequent entries include > + // multiscripts_ or msubsup_ with either the superscripts or subscripts mmultiscripts_ @@ +302,5 @@ > > nscoord ruleSize; > GetRuleThickness (aRenderingContext, fm, ruleSize); > > + // force the scriptSpace to be atleast 1 pixel at least

Comments have been addressed and additional mochitests added. There is also a change of behaviour regarding italics correction. The previous patch only added an extra one pixel to the italics correction if the italics correction was greater than 0. The attached patch always adds the additional one pixel whenever there is a post-superscript. This avoids any false negatives.

The InvalidChild error is also triggered when the base element of a mmultiscripts element is <none/>, as my interpretation of the schema suggested only the script elements allowed <none/>. Is not mentioning which <none/> is objectionable too ambiguous?

Fix a few minor problems.

Comment on attachment 723405 [details] [diff] [review] Unify script behaviour with comments addressed > nsresult > ReportChildCountError(); > > /* >+ * Helper to call ReportErrorToConsole when certain tags have >+ * invalid child tags >+ * @param aChildTag The tag which is forbidden in this context >+ */ >+ nsresult >+ nsMathMLContainerFrame::ReportInvalidChildError(nsIAtom* aChildTag); This results in a build failure using gcc. Please remove the redundant "nsMathMLContainerFrame::"

Sorry, I haven't found time to review your patches, since I've been busy with other projects. Last time I checked, nsMathMLmmultiscriptsFrame.cpp was the one that would take the most of time to review since it requires some care. If you could split your patch into several pieces say 1) Tests 2) Code removal and other modifications to make script elements use the mmultiscripts frame. 3) Code changes to mmultiscripts that would help a bit the review.

Correct patch this time

(In reply to James Kitchener from comment #17) > Created attachment 723388 [details] [diff] [review] > Localisation changes > > The InvalidChild error is also triggered when the base element of a > mmultiscripts element is <none/>, as my interpretation of the schema > suggested only the script elements allowed <none/>. > > Is not mentioning which <none/> is objectionable too ambiguous? Yes, this is a problem. Perhaps NoBase could be triggered in that case, since <none/> is always a script of mmultiscript.

Comment on attachment 730047 [details] [diff] [review] Part 1: Changes to mmultiscripts Asking review to Karl too, in case he is more available than me to look at this one.

Comment on attachment 730050 [details] [diff] [review] Part 3: tests OK that looks good to me, module the necessary changes to test_bug827713-2.html if you decide to modify the message for mmultiscripts+none

<none/> for mmultiscripts base element reports NoBase as suggested.

NoBase change

Comment on attachment 731633 [details] [diff] [review] Part 1: Changes to mmultiscripts v2 Review of attachment 731633 [details] [diff] [review]: ----------------------------------------------------------------- Thanks James. I still need to analyze precisely what you've done in the main loop (to compute the bounding metrics) but the rest looks good. I give some feedback below. ::: layout/mathml/nsMathMLmmultiscriptsFrame.cpp @@ +227,5 @@ > + > + prescriptsFrame = childFrame; > + > + } > + else if (0 == count) { } else if (0 == count) { @@ +228,5 @@ > + prescriptsFrame = childFrame; > + > + } > + else if (0 == count) { > + if ( childTag == nsGkAtoms::none) { no space after the ( When tag != nsGkAtoms::mmultiscripts_, I guess you don't need to report an error but just set foundNoneTag to true. @@ +236,5 @@ > + return aFrame->ReflowError(aRenderingContext, aDesiredSize); > + } > + baseFrame = childFrame; > + } > + else { } else { @@ +237,5 @@ > + } > + baseFrame = childFrame; > + } > + else { > + if ( childTag == nsGkAtoms::none) { no space after the ( @@ +248,5 @@ > + // not being present for equivTag purposes > + subScriptFrame = childFrame; > + } > + } > + else { } else { @@ +270,5 @@ > + foundNoneTag && tag != nsGkAtoms::mmultiscripts_ || !baseFrame || > + !isSubScript && tag == nsGkAtoms::mmultiscripts_) { > + // report an error, encourage people to get their markups in order > + if (aPlaceOrigin) { > + if ( count != 2 && (tag == nsGkAtoms::msup_ || No space after ( @@ +273,5 @@ > + if (aPlaceOrigin) { > + if ( count != 2 && (tag == nsGkAtoms::msup_ || > + tag == nsGkAtoms::msub_)) { > + aFrame->ReportChildCountError(); > + } else if ( count != 3 && tag == nsGkAtoms::msubsup_ ) { It looks like this one can be merged with the previous condition. @@ +275,5 @@ > + tag == nsGkAtoms::msub_)) { > + aFrame->ReportChildCountError(); > + } else if ( count != 3 && tag == nsGkAtoms::msubsup_ ) { > + aFrame->ReportChildCountError(); > + } else if ( foundNoneTag && tag != nsGkAtoms::mmultiscripts_) { no space after ( @@ +302,5 @@ > + } > + else { > + // degenerate element with neither supscripts nor subscripts > + equivTag = nsGkAtoms::mrow_; > + } Coding style: } else if { } else { @@ +311,5 @@ > // Initialize super/sub shifts that > // depend only on the current font > //////////////////////////////////////// > > + baseFrame = aFrame->GetFirstPrincipalChild(); Hasn't the baseFrame already been determined above? @@ +319,2 @@ > nsRefPtr<nsFontMetrics> fm; > + nsLayoutUtils::GetFontMetricsForFrame(baseFrame, getter_AddRefs(fm)); Not sure why mmultiscripts used "this" rather than baseFrame, but the other scripts did use baseFrame, so I guess this change is correct. @@ +350,5 @@ > + subScriptShift1 = std::max(subScriptShift1, aUserSubScriptShift); > + } > + // the font dependent shift > + subScriptShift = std::max(subScriptShift1,subScriptShift2); > + } It seems to me that this can be rewritten nscoord subScriptShift; if (equivTag == nsGkAtoms::msub_) { subScriptShift = subScriptShift1; } else { // the font dependent shift subScriptShift = std::max(subScriptShift1, subScriptShift2); } if (0 < aUserSubScriptShift) { // the user has set the subscriptshift attribute subScriptShift = std::max(subScriptShift, aUserSubScriptShift); } @@ +373,1 @@ > } I think you can remove this if (0 < aUserSupScriptShift) { block (see below)... @@ +390,5 @@ > else { > // everything else = T,S,SS > supScriptShift = supScriptShift2; > } > ...at this point, the final supScriptShift is determined. You can place your if (0 < aUserSubScriptShift) { // the user has set the supscriptshift attribute supScriptShift = std::max(supScriptShift, aUserSupScriptShift); } @@ +396,5 @@ > // Get the children's sizes > //////////////////////////////////// > > nscoord width = 0, prescriptsWidth = 0, rightBearing = 0; > + isSubScript = (tag != nsGkAtoms::msup_); Do you need to reset prescriptsFrame? It has been set above and it is tested whether it is null below. @@ +441,5 @@ > + > + // we update boundingMetrics.{ascent,descent} with that > + // of the baseFrame only after processing all the sup/sub pairs > + // XXX need italic correction only *if* there are postscripts ? > + boundingMetrics.width = bmBase.width + italicCorrection; So it seems that thanks to the pre-processing you are able to address the XXX comment here, right? In that case, the comment can now be removed. Also, you should add italicCorrection only when it has been set in the hasPostSupScript block above, otherwise the value from the previous step will be used. @@ +619,4 @@ > dy, 0); > dx += bmBase.width + italicCorrection; > } > + else if (prescriptsFrame != childFrame) { } else if {

Applied feedback changes.

Comment on attachment 736788 [details] [diff] [review] Part 1: Changes to mmultiscripts v3 Review of attachment 736788 [details] [diff] [review]: ----------------------------------------------------------------- I don't think I have more comments to the previous feedback, but I prefer to let Karl review these changes.

(In reply to James Kitchener from comment #24) > Created attachment 730052 [details] [diff] [review] > Part 2: Use mmultiscripts for all script shifts > > Correct patch this time Due to other changes in the build system the changes in layout/mathml/Makefile.in need to move to layout/mathml/moz.build

rebase

rebase

Comment on attachment 759697 [details] [diff] [review] Part 1: Changes to mmultiscripts v4 Thank you for this. Sorry it's taken me a while to reply. There a lot of similar code but slightly different code involved (which took me some time to work interpret) and merging it together helps clarify what the variations are, thank you. I see you've also fixed up some bugs here. However, I'm not sure that detecting empty scripts and changing the logic is a good idea. It adds additional complexity to the code and is of questionable value. In some ways keeping the behavior of an element with empty scripts more similar to the behavior with non-empty scripts is better because it allows an author to make elements look more similar if desired. I think the detection of empty scripts should be removed, sorry. > // The TeXbook (Ch 17. p.141) says the superscript inherits the compression > // while the subscript is compressed. So here we collect subscripts and set > // the compression flag in them. > int32_t count = 0; >- bool isSubScript = false; >+ if (mContent->Tag() == nsGkAtoms::msup_) >+ return NS_OK; >+ bool isSubScript = true; >+ Please move the early return to before the declaration of |count| (because that is not used before the early return) and leave a blank line after an unbraced return block (to make the return statement stand out a bit more). Nice catch in finding the incorrect initialization of isSubScript before the toggling. In future, in general, it is helpful to have patches for bug fixes separate from refactoring patches. That makes it clear whether a patch is intending to change behavior and exactly what behavior is changing. >+ //////////////////////////////////// >+ // Get the children's desired sizes > > // subscriptshift The moved comment doesn't seem to belong here and it didn't seem to belong in it previous location either, so please just remove it. >+ return nsMathMLmmultiscriptsFrame::PlaceMultiScript(PresContext(), >+ aRenderingContext, >+ aPlaceOrigin, >+ aDesiredSize, >+ this, >+ subScriptShift, >+ supScriptShift, >+ scriptSpace); I expect |nsMathMLmmultiscriptsFrame::| is not necessary here and so it will be easier to fit in the parameters and tidy up the alignment. > // Get subScriptShift{1,2} default from font > GetSubScriptShifts (fm, subScriptShift1, subScriptShift2); >- if (0 < mSubScriptShift) { >+ nscoord subScriptShift; >+ if (equivTag == nsGkAtoms::msub_) { >+ subScriptShift = subScriptShift1; >+ } else { >+ // the font dependent shift >+ subScriptShift = std::max(subScriptShift1, subScriptShift2); >+ } Having the "font dependent shift" comment here suggests that the first part is not font dependent (but it is), so I think it's best to just remove the comment. The comment above already says it comes from the font. >+ if (!prescriptsFrame) { // we are still looping over base & postscripts This patch has prescriptsFrame set before this loop, which I assume breaks this logic. >+ if (equivTag == nsGkAtoms::msub_) { >+ width = rightBearing = 0; I don't think these get used again, and so they don't need to be reset. >+ if (gap > 0.0f) { gap is an nscoord, so (gap > 0) would be better here (as in msubsup). >+ boundingMetrics.ascent = >+ std::max(boundingMetrics.ascent+maxSupScriptShift,bmBase.ascent); >+ boundingMetrics.descent = >+ std::max(boundingMetrics.descent+maxSubScriptShift,bmBase.descent); >+ aFrame->SetBoundingMetrics(boundingMetrics); There is a difference here between this and the current msubsup calculation. mmultiscripts is adding the maximum ascent including *sub*scripts to max*Sup*ScriptShift to determine the ascent of the mmultiscripts. msubsup does a much more precise calculation. Similarly for aDesiredSize. I think mmultiscripts should be made similar to msubsup by keeping track of the maximum supscript metrics and maximum subscript metrics separately. That should be a separate patch before the "Use mmultiscripts for all script shifts" patch. > private: >- nscoord mSubScriptShift; >- nscoord mSupScriptShift; >- > void > ProcessAttributes(); This ProcessAttributes() declaration should be removed now too.

Required changes have been made

Tests needed to be changed because 1. msub and msubsup now behave differently as different rules (18b/c) apply according to App. G, Texbook. 2. The code to detect whether a supscript is present has been removed from part 1, so msup, msubsup and mmultiscripts will always add the additional pixel for safety.

Comment on attachment 771297 [details] [diff] [review] Part 1: Changes to mmultiscripts attached patch is buggy. I'll upload a new one when fixed.

This one has correct behaviour.

Rebased, but otherwise unchanged since the last attempt.

Comment on attachment 771618 [details] [diff] [review] Part 1: Changes to mmultiscripts >+ //NoBase error is reported at the start "NoBase error may also have been reported above" because it may be reported below and there are two places above where NoBase might have been reported, one of which was not "at the start".

Comment on attachment 771619 [details] [diff] [review] Part 2: Adopt msubsup style metrics for mmultiscripts >+ bmMultiSup.descent = bmSupScript.descent; I expect this should use the maximum, like the subscript case. r=karlt with that change.

Fixed. Green try run. https://tbpl.mozilla.org/?tree=Try&rev=9ec34f427568 Unless there are any further issues, this may be ready to land.

Thanks James. Can you leave a note for MDN people about important changes that should be documented? IIUC, there are at least: - fix a bug with some script shifts (and thus win one point at MathML Acid3) - make mmultiscripts behave as msubsup Other than that I guess the merge of all frames into mmultiscripts should not be apparent to users.

* The rendering of msup, msubsup and mmultiscripts is now identical for equivalent configurations. msub remains visually distinct and should not have noticeable differences. * The unified behaviour adopts the old msubsup behaviour for sub/sup script locations and the old mmultiscripts behaviour for the bounding box and for the distance between base and (post) supscript. * A bug causing incorrect application of the subscriptshift and supscriptshift attributes for msup, msubsup and mmultiscripts has been fixed. Error handling: * msub, msup and msubsup may no longer have <none/> as a child element. * A more relevant error is returned when <mprescripts/> is a child elemet for msub, msup and mmultiscripts * The base element of mmultiscripts can no longer be <none/> * The SubSupMismatch error check now takes into account the presence of a <mprescripts/> element.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1279664e0d41 https://hg.mozilla.org/integration/mozilla-inbound/rev/b67a72618c66 https://hg.mozilla.org/integration/mozilla-inbound/rev/6448c7e05f11 https://hg.mozilla.org/integration/mozilla-inbound/rev/27a5c8dd5ff7 https://hg.mozilla.org/integration/mozilla-inbound/rev/4113172193aa

Interestingly, test_bug827713-2.html only asserts 20 times on WinXP, which was causing orange. Pushed an annotation update. Also, I don't see a bug on file for the asserts this test is hitting. Can you please file one and add a comment to the test pointing to it? Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/014cc3de08fb

Unfortunately, this was also hitting reftest failures on OSX 10.6. Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/a59ad3157c17 https://tbpl.mozilla.org/php/getParsedLog.php?id=25867784&tree=Mozilla-Inbound

The assertions were caused by a fault in the testing code, resolvable by a two line fix.

Regarding the reftest failure: The bug is limited to the following pattern for mmultiscripts and msubsup on OS X 10.6: <mmultiscripts> <mi>aaa</mi> <mrow></mrow> <mrow></mrow> </mmultiscripts> The scriptless variant of mmultiscripts is not affected, which is why the reftest fails <mmultiscripts> <mi>aaa</mi> </mmultiscripts> I have determined that it is an already existing bug, not one introduced by my code (although it now affects msubsup as well as mmultiscripts). Other than that, I have been unable to identify the underlying cause. My question is whether it must be fixed before these changes can land? It is limited to a single platform and is probably unlikely to be triggered in real-world MathML (presumably most people that invoke sub/sup script elements actually want to use the sub/sup script capabilities). In the event that it is triggered, the consequences are minor (a slightly larger bounding box).

I think it's OK to ignore this failure for now. IIUC, the patch does not entirely make all the scripts behave the same anyway. What about adding <none/> or <mrow></mrow> elements to the last <mmultiscripts> element of multiscripts-1-ref.html? Or perhaps just removing that element...

Small typo correction. >- bmMultiSup.ascent = std::max(bmMultiSup.ascent, bmSubScript.ascent); >+ bmMultiSup.ascent = std::max(bmMultiSup.ascent, bmSupScript.ascent);

Reftest changed to avoid 10.6-only orange.

Green try run https://tbpl.mozilla.org/?tree=Try&rev=381ae6116b2c

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e3a08f5c776 https://hg.mozilla.org/integration/mozilla-inbound/rev/946bd9ad33d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/535584793915 https://hg.mozilla.org/integration/mozilla-inbound/rev/bdcadb9eda95 https://hg.mozilla.org/integration/mozilla-inbound/rev/9a06d4cd909c

https://hg.mozilla.org/mozilla-central/rev/7e3a08f5c776 https://hg.mozilla.org/mozilla-central/rev/946bd9ad33d5 https://hg.mozilla.org/mozilla-central/rev/535584793915 https://hg.mozilla.org/mozilla-central/rev/bdcadb9eda95 https://hg.mozilla.org/mozilla-central/rev/9a06d4cd909c

These fixes are mentioned on the Fx 27 for developer page: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/26#MathML A note has been added to these reference pages ("Gecko-specific notes"): https://developer.mozilla.org/en-US/docs/Web/MathML/Element/mmultiscripts https://developer.mozilla.org/en-US/docs/Web/MathML/Element/msubsup https://developer.mozilla.org/en-US/docs/Web/MathML/Element/msup https://developer.mozilla.org/en-US/docs/Web/MathML/Element/msub As always, feel free to review and edit.