Inconsistent rendering of mub / mup / msubsup / mmultiscripts

RESOLVED FIXED in mozilla26

Status

()

Core
MathML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: fredw, Assigned: jkitch)

Tracking

(Depends on: 1 bug, {dev-doc-complete, helpwanted, student-project})

Trunk
mozilla26
dev-doc-complete, helpwanted, student-project
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=fredw][lang=c++][MDN people: see comment 47 and below])

Attachments

(8 attachments, 23 obsolete attachments)

910 bytes, text/html
Details
3.93 KB, text/html
Details
1.05 KB, text/html
Details
1.39 KB, patch
fredw
: review+
Details | Diff | Splinter Review
41.78 KB, patch
Details | Diff | Splinter Review
34.14 KB, patch
Details | Diff | Splinter Review
7.10 KB, patch
Details | Diff | Splinter Review
12.89 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 699025 [details]
Bob Mathews' orignal testcase

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

Comment 1

5 years ago
Created attachment 699027 [details]
Testcase

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

Updated

5 years ago
Assignee: nobody → jkitch.bug
(Assignee)

Comment 2

5 years ago
Created attachment 708541 [details] [diff] [review]
unify msub, msup, msubsup, mmultiscripts

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

Comment 3

5 years ago
Created attachment 712251 [details] [diff] [review]
wip

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.
Attachment #708541 - Attachment is obsolete: true
(Reporter)

Comment 4

5 years ago
> > 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.
(Reporter)

Comment 5

5 years ago
Created attachment 713139 [details]
testcase script shifts

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

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 714317 [details]
updated testcase script shifts

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.
Attachment #713139 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
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)
(Reporter)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
Created attachment 717607 [details] [diff] [review]
wip v2

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.
Attachment #712251 - Attachment is obsolete: true
(Reporter)

Comment 11

5 years ago
(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!
(Assignee)

Comment 12

5 years ago
Created attachment 717771 [details] [diff] [review]
unify sub/sup scripts with TeX behaviour

The problems in the previous patch have been resolved.

I've tested it against the layour reftests, and there are no additional failures.
Attachment #717607 - Attachment is obsolete: true
Attachment #717771 - Flags: review?(fred.wang)
(Reporter)

Comment 13

5 years ago
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)
(Assignee)

Comment 14

5 years ago
Created attachment 718881 [details] [diff] [review]
unify sub/sup scripts with correct behaviour for mover/under

Tests 34-38 now pass.
Attachment #717771 - Attachment is obsolete: true
Attachment #717771 - Flags: review?(fred.wang)
Attachment #718881 - Flags: review?(fred.wang)
(Reporter)

Comment 15

5 years ago
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
(Assignee)

Comment 16

5 years ago
Created attachment 723387 [details] [diff] [review]
Unify script behaviour with comments addressed

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.
Attachment #718881 - Attachment is obsolete: true
Attachment #718881 - Flags: review?(fred.wang)
Attachment #723387 - Flags: review?(fred.wang)
(Assignee)

Comment 17

5 years ago
Created attachment 723388 [details] [diff] [review]
Part 0: 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?
Attachment #723388 - Flags: review?(fred.wang)
(Assignee)

Comment 18

5 years ago
Created attachment 723405 [details] [diff] [review]
Unify script behaviour with comments addressed

Fix a few minor problems.
Attachment #723387 - Attachment is obsolete: true
Attachment #723387 - Flags: review?(fred.wang)
Attachment #723405 - Flags: review?(fred.wang)
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::"
(Reporter)

Comment 20

5 years ago
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.
(Assignee)

Comment 21

5 years ago
Created attachment 730047 [details] [diff] [review]
Part 1: Changes to mmultiscripts
Attachment #730047 - Flags: review?(fred.wang)
(Assignee)

Comment 22

5 years ago
Created attachment 730048 [details] [diff] [review]
Part 2: Use mmultiscripts for all script shifts
Attachment #730048 - Flags: review?(fred.wang)
(Assignee)

Comment 23

5 years ago
Created attachment 730050 [details] [diff] [review]
Part 3: tests
Attachment #723405 - Attachment is obsolete: true
Attachment #723405 - Flags: review?(fred.wang)
Attachment #730050 - Flags: review?(fred.wang)
(Assignee)

Comment 24

5 years ago
Created attachment 730052 [details] [diff] [review]
Part 2: Use mmultiscripts for all script shifts

Correct patch this time
Attachment #730048 - Attachment is obsolete: true
Attachment #730048 - Flags: review?(fred.wang)
Attachment #730052 - Flags: review?(fred.wang)
(Reporter)

Comment 25

5 years ago
(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.
(Reporter)

Updated

5 years ago
Attachment #730052 - Flags: review?(fred.wang) → review+
(Reporter)

Comment 26

5 years ago
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.
Attachment #730047 - Flags: review?(karlt)
(Reporter)

Updated

5 years ago
Attachment #723388 - Flags: review?(fred.wang) → review+
(Reporter)

Updated

5 years ago
Attachment #723388 - Flags: review?(l10n)
(Reporter)

Comment 27

5 years ago
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
Attachment #730050 - Flags: review?(fred.wang) → review+
(Assignee)

Comment 28

5 years ago
Created attachment 731633 [details] [diff] [review]
Part 1: Changes to mmultiscripts v2

<none/> for mmultiscripts base element reports NoBase as suggested.
Attachment #730052 - Attachment is obsolete: true
Attachment #731633 - Flags: review?(karlt)
Attachment #731633 - Flags: review?(fred.wang)
(Assignee)

Updated

5 years ago
Attachment #730052 - Attachment is obsolete: false
(Assignee)

Comment 29

5 years ago
Created attachment 731634 [details] [diff] [review]
Part 3: tests v2

NoBase change
Attachment #730047 - Attachment is obsolete: true
Attachment #730050 - Attachment is obsolete: true
Attachment #730047 - Flags: review?(karlt)
Attachment #730047 - Flags: review?(fred.wang)

Updated

4 years ago
Attachment #723388 - Flags: review?(l10n)
(Reporter)

Comment 30

4 years ago
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 {
Attachment #731633 - Flags: review?(fred.wang) → feedback+
(Assignee)

Comment 31

4 years ago
Created attachment 736788 [details] [diff] [review]
Part 1: Changes to mmultiscripts v3

Applied feedback changes.
Attachment #731633 - Attachment is obsolete: true
Attachment #731633 - Flags: review?(karlt)
Attachment #736788 - Flags: review?(karlt)
(Assignee)

Updated

4 years ago
Attachment #736788 - Flags: review?(fred.wang)
(Reporter)

Comment 32

4 years ago
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.
Attachment #736788 - Flags: review?(fred.wang)
(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
(Assignee)

Comment 34

4 years ago
Created attachment 759696 [details] [diff] [review]
Part 3: Use mmultiscripts for all script shifts v2

rebase
Attachment #730052 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
Created attachment 759697 [details] [diff] [review]
Part 1: Changes to mmultiscripts v4

rebase
Attachment #736788 - Attachment is obsolete: true
Attachment #736788 - Flags: review?(karlt)
Attachment #759697 - Flags: review?(karlt)
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.
Attachment #759697 - Flags: review?(karlt) → review-
(Assignee)

Comment 37

4 years ago
Created attachment 771297 [details] [diff] [review]
Part 1: Changes to mmultiscripts

Required changes have been made
Attachment #759697 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #771297 - Flags: review?(karlt)
(Assignee)

Comment 38

4 years ago
Created attachment 771300 [details] [diff] [review]
Part 2: Adopt msubsup style metrics for mmultiscripts
Attachment #771300 - Flags: review?(karlt)
(Assignee)

Updated

4 years ago
Attachment #759696 - Attachment description: Part 2: Use mmultiscripts for all script shifts v2 → Part 3: Use mmultiscripts for all script shifts v2
(Assignee)

Comment 39

4 years ago
Created attachment 771301 [details] [diff] [review]
Part 4: tests

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.
Attachment #731634 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #723388 - Attachment description: Localisation changes → Part 0: Localisation changes
(Assignee)

Comment 40

4 years ago
Comment on attachment 771297 [details] [diff] [review]
Part 1: Changes to mmultiscripts

attached patch is buggy.  I'll upload a new one when fixed.
Attachment #771297 - Flags: review?(karlt)
(Assignee)

Updated

4 years ago
Attachment #771300 - Flags: review?(karlt)
(Assignee)

Comment 41

4 years ago
Created attachment 771618 [details] [diff] [review]
Part 1: Changes to mmultiscripts

This one has correct behaviour.
Attachment #771297 - Attachment is obsolete: true
Attachment #771618 - Flags: review?(karlt)
(Assignee)

Comment 42

4 years ago
Created attachment 771619 [details] [diff] [review]
Part 2: Adopt msubsup style metrics for mmultiscripts

Rebased, but otherwise unchanged since the last attempt.
Attachment #771300 - Attachment is obsolete: true
Attachment #771619 - Flags: review?(karlt)
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".
Attachment #771618 - Flags: review?(karlt) → review+
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.
Attachment #771619 - Flags: review?(karlt) → review+
(Assignee)

Comment 45

4 years ago
Created attachment 772596 [details] [diff] [review]
Part 1: Changes to mmultiscripts
Attachment #771618 - Attachment is obsolete: true
(Assignee)

Comment 46

4 years ago
Created attachment 772598 [details] [diff] [review]
Part 2: Adopt msubsup style metrics for mmultiscripts

Fixed.

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

Unless there are any further issues, this may be ready to land.
Attachment #771619 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Comment 47

4 years ago
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.
Whiteboard: [mentor=fredw][lang=c++] → [mentor=fredw][lang=c++][MDN people: see comment 47 and below]
(Assignee)

Comment 48

4 years ago
* 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
Flags: in-testsuite+
Keywords: checkin-needed
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
(Assignee)

Comment 52

4 years ago
Created attachment 791783 [details] [diff] [review]
Part 4: tests

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

Updated

4 years ago
Attachment #771301 - Attachment is obsolete: true
(Assignee)

Comment 53

4 years ago
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).
Flags: needinfo?(fred.wang)
(Reporter)

Comment 54

4 years ago
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...
Flags: needinfo?(fred.wang)
(Assignee)

Comment 55

4 years ago
Created attachment 791808 [details] [diff] [review]
Part 2: Adopt msubsup style metrics for mmultiscripts

Small typo correction.

>-        bmMultiSup.ascent = std::max(bmMultiSup.ascent, bmSubScript.ascent);
>+        bmMultiSup.ascent = std::max(bmMultiSup.ascent, bmSupScript.ascent);
Attachment #772598 - Attachment is obsolete: true
(Assignee)

Comment 56

4 years ago
Created attachment 791809 [details] [diff] [review]
Part 4: tests

Reftest changed to avoid 10.6-only orange.
Attachment #791783 - Attachment is obsolete: true
(Assignee)

Comment 57

4 years ago
Green try run

https://tbpl.mozilla.org/?tree=Try&rev=381ae6116b2c
Keywords: checkin-needed
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
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 970622
You need to log in before you can comment on or make changes to this bug.