Closed Bug 945254 Opened 6 years ago Closed 6 years ago

Take italic correction into account when positioning subscripts

Categories

(Core :: MathML, defect, P4)

25 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: dr.khaled.hosny, Assigned: fredw)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 8 obsolete files)

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).
Priority: -- → P4
@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.
Attachment #8341085 - Attachment is obsolete: true
(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.
Keywords: regression
Blocks: 407059
Assignee: nobody → fred.wang
Status: UNCONFIRMED → NEW
Ever confirmed: true
Depends on: 928675
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".
Flags: needinfo?(khaledhosny)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8361049 - Flags: feedback?(khaledhosny)
(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).
Flags: needinfo?(khaledhosny)
Attached patch Patch V2 (obsolete) — Splinter Review
Attachment #8360599 - Attachment is obsolete: true
Attachment #8361049 - Attachment is obsolete: true
Attachment #8361049 - Flags: feedback?(khaledhosny)
Attachment #8362062 - Flags: review?(roc)
Blocks: 961482
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.
Attachment #8362062 - Flags: review?(roc) → review+
Attached patch Patch V3 (obsolete) — Splinter Review
Other tests seem to assume that the italic correction is always added to the width and rightbearing, so let's keep that for now.
Attachment #8362062 - Attachment is obsolete: true
Attachment #8363541 - Flags: review?(karlt)
Attached patch Patch V4 (obsolete) — Splinter Review
Attachment #8363541 - Attachment is obsolete: true
Attachment #8363541 - Flags: review?(karlt)
Attached patch Patch V5 (obsolete) — Splinter Review
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).
Attachment #8365926 - Flags: review?(karlt)
Attachment #8364313 - Attachment is obsolete: true
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?
Attachment #8365926 - Flags: review?(karlt) → review-
(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.
Attached patch Patch V6 (obsolete) — Splinter Review
Attachment #8365926 - Attachment is obsolete: true
Attachment #8366499 - Flags: review?(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.
Attachment #8366499 - Flags: review?(karlt) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/144a74108b59
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.