945254 - Take italic correction into account when positioning subscripts

Status: RESOLVED FIXED

(Core :: MathML, defect, P4)

Description

[Khaled Hosny]

Attachment: Don't apply italic correction before postscript subscripts

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

Comment 1

[Frédéric Wang (:fredw)]

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

Comment 2

[Khaled Hosny]

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.

Comment 3

[Khaled Hosny]

Attachment: Don't apply italic correction before postscript subscripts

Rebased patch.

Comment 4

[Frédéric Wang (:fredw)]

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

Comment 5

[Frédéric Wang (:fredw)]

Attachment: 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".

Comment 6

[Frédéric Wang (:fredw)]

Attachment: Patch

Comment 7

[Bill Gianopoulos [:WG9s]]

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

Comment 8

[Khaled Hosny]

(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 9

[Frédéric Wang (:fredw)]

Attachment: Patch V2

Comment 10

[Karl Tomlinson (:karlt)]

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.

Comment 11

[Frédéric Wang (:fredw)]

Attachment: Patch V3

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

Comment 12

[Frédéric Wang (:fredw)]

Attachment: Patch V4

Comment 13

[Frédéric Wang (:fredw)]

Attachment: Patch V5

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 14

[Karl Tomlinson (:karlt)]

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?

Comment 15

[Frédéric Wang (:fredw)]

(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

Comment 16

[Karl Tomlinson (:karlt)]

(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>

Comment 17

[Frédéric Wang (:fredw)]

(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 18

[Frédéric Wang (:fredw)]

Attachment: Patch V6

Comment 19

[Karl Tomlinson (:karlt)]

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.

Comment 20

[Frédéric Wang (:fredw)]

Attachment: Patch (final version)

https://tbpl.mozilla.org/?tree=Try&rev=e971e453958c

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/144a74108b59

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/144a74108b59