For subscripts after bases with italic correction; the italic correction should be applied before the superscript only. This patch fixes this, and also opens the door for more fine grained control with MATH table in the future (where the placement of super and subscripts in both sides can be adjusted by the font developer).

@Khaled: any update on this? I can try to update you patch with tests if you wish.

I was just wating for the bug 407059 to land, so we can use the full potential of OpenType math fonts here (using math cut-ins for pre as well as postscripts, etc.), but if you think this should be done in a follow up issue, please take this one.

Rebased patch.

(In reply to Khaled Hosny from comment #2) > I was just wating for the bug 407059 to land, so we can use the full > potential of OpenType math fonts here (using math cut-ins for pre as well as > postscripts, etc.), but if you think this should be done in a follow up > issue, please take this one. OK, I think I'll probably need that for my issue with subscript placement in bug 407059. I will see what I can do. Karl is only back in the end of January so I don't expect the patches for bug 663740 and bug 407059 to be reviewed soon.

Here is a testcase. I see that your patch only removes the italic correction from the postsubscript next to the base. Is it intentional? Or should we remove the italic correction from *all* the post subscripts? In the former case, I have a simpler patch that don't use a boolean but just check "baseFrame->GetNextSibling() == subScriptFrame".

(In reply to Frédéric Wang (:fredw) from comment #6) > Created attachment 8361049 [details] [diff] [review] > Patch This patch has the wrong bug number in the bug comment.

(In reply to Frédéric Wang (:fredw) from comment #5) > Created attachment 8361047 [details] > test-italic-correction.html > > Here is a testcase. > > I see that your patch only removes the italic correction from the > postsubscript next to the base. Is it intentional? Or should we remove the > italic correction from *all* the post subscripts? In the former case, I have > a simpler patch that don't use a boolean but just check > "baseFrame->GetNextSibling() == subScriptFrame". I’m not sure, I don’t actually know what is the use case of multisubscripts, or how it should look, so I accept your judgment here (It might be worth comparing this to MS’s OpenType math implementation, no idea how though).

Comment on attachment 8362062 [details] [diff] [review] Patch V2 >+ // a postscript subscript should be immediately after the base, so >+ // we subtract the italic correction that was added above How about instead adding the italic correction for the postscript superscript? Then it doesn't need to be added to the base. Are you able to adjust boundingMetrics.rightBearing consistently too, please? For boundingMetrics.width, I think we can just remove the code that adds italicCorrection (with bmBase.width). The superscript may extended beyond the width a little, but that is just like an italic identifier.

Other tests seem to assume that the italic correction is always added to the width and rightbearing, so let's keep that for now.

I have remove the test for msup with empty scripts as I don't think it is possible to make it work now and I don't think it's too important (I also tried to comparing with an msubsup with empty scripts but this fails on Mac and android).

Comment on attachment 8365926 [details] [diff] [review] Patch V5 >+ if (firstPostscriptsPair) { >+ // we add the italic correction to the first superscript >+ rightBearing = std::max(rightBearing, italicCorrection + >+ bmSupScript.rightBearing); >+ firstPostscriptsPair = false; >+ } else { >+ rightBearing = std::max(rightBearing, bmSupScript.rightBearing); >+ } All post superscripts are offset by italicCorrection, so the maths in the first block above should be used for all of them. (Actually, the rightBearing of only the last post superscript is used, but it makes logical sense to consider all them.) The second block is not necessary. (|rightBearing| is used only for post scripts.) (In reply to Frédéric Wang (:fredw) from comment #13) > I have remove the test for msup with empty scripts as I don't think it is > possible to make it work now and I don't think it's too important (I also > tried to comparing with an msubsup with empty scripts but this fails on Mac > and android). Do you know why this change makes that test fail? I would have expected this change to affect the logical metrics of mmultiscripts and msup in the same way. Do you have a link to the failing test? Would it pass if the msup and msu*b*su*p* were on different lines?

(In reply to Karl Tomlinson (back Jan 28 :karlt) from comment #14) > I would have expected this change to affect the logical metrics of > mmultiscripts and msup in the same way. > Do you have a link to the failing test? > Would it pass if the msup and msu*b*su*p* were on different lines? https://tbpl.mozilla.org/?tree=Try&rev=c51b45ce5aae

(In reply to Frédéric Wang (:fredw) from comment #15) > (In reply to Karl Tomlinson (back Jan 28 :karlt) from comment #14) > > > I would have expected this change to affect the logical metrics of > > mmultiscripts and msup in the same way. > > Do you have a link to the failing test? > > Would it pass if the msup and msu*b*su*p* were on different lines? > > https://tbpl.mozilla.org/?tree=Try&rev=c51b45ce5aae Isn't that difference from to adding the subscript to the mmultiscripts?: msup / msupsub: <math> <mmultiscripts style="background: red;"> <mtext style="background-color: rgba(0, 0, 255, 0.4);">AAA</mtext> + <none /> + <none /> </mmultiscripts>

(In reply to Karl Tomlinson (back Jan 28 :karlt) from comment #16) > Isn't that difference from to adding the subscript to the mmultiscripts?: > > msup / msupsub: > <math> > <mmultiscripts style="background: red;"> > <mtext style="background-color: rgba(0, 0, 255, 0.4);">AAA</mtext> > + <none /> > + <none /> > </mmultiscripts> Without that the mmultiscripts was wider than the msup, IIRC.

Comment on attachment 8366499 [details] [diff] [review] Patch V6 >+ rightBearing = std::max(rightBearing, bmSupScript.rightBearing); Please remove this line, as the value will not be used.