928675 - Sup/superscripts are centred relative to each other

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Khaled Hosny]

Opening the MathML start page[1] with nightly build, in the last equation the superscript after the vertical bar is centred relative to the subscript.

This seems to be a regression after bug 827713 merged handling of all scripted items into mmultiscripts code. The code for multiscripts is intentionally centring sub/superscripts for reasons not clear to me; it have been like that since the initial MathML import[2], was changed then reintroduced with an “interesting” comment[3], and kept like that from there on.

I’m surprised that no one noticed/complained about this erratic behaviour of mmultiscripts all this year and feel like I’m missing something, but if I’m not, the attached patch should fix this issue (incidentally it is essentially reverting the change in [3]).

1. https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/Start
2. https://github.com/mozilla/mozilla-central/commit/ae8628f9e43b5f4eee4932cc68121d837eebd91c#diff-8da4b0af96cc4b1175d6be43fba2afedR221
3. https://github.com/mozilla/mozilla-central/commit/51e4afc749a1a1a3550eb9f92113bab80e436e47#diff-8da4b0af96cc4b1175d6be43fba2afedL401

Comment 1

[Khaled Hosny]

Attachment: Patch V1

Comment 2

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

The inconsistency was raised last year:
http://lists.w3.org/Archives/Public/www-math/2012Aug/0006.html

I don't see anything new in
http://www.w3.org/Math/draft-spec/chapter3.html#presm.mmultiscripts
so at the moment the exact alignment is unspecified and left to the implementer.

Comment 3

[Khaled Hosny]

OK, so with this patch both pre/postscripts are left aligned, which is closer to the desired behaviour. I think I’m going to try to right align the prescripts and if the spec later introduces alignment attributes, we can implement them (MS’ OpenType math engine right aligns prescripts, so that is a bonus).

Comment 4

[Khaled Hosny]

Attachment: Patch V2

This new patch right aligns prescripts. I think the position of postscript subscripts can be improved as well by subtracting the italic correction, but lets keep this for another issue.

Comment 5

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

> +            // presecripts should be right aligned

there is a typo here. Perhaps we should explain why we choose the left/right alignment and mention the MathML W3C thread for future work.

I'm not quite sure why sub/sup scripts are treated differently in your patch. I would have expected something like
- dx for postscripts (sup/sub)
- dx + width - supScriptSize.width for pre-superscript
- dx + width - subScriptSize.width for pre-subscript     ?

Also, we will probably need to add reftests for that alignment, so that we won't regress again in the future (see https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests). I suggest adding == tests like
<math>
  <mmultiscripts>
      <mspace width="50px" height="50px" mathbackground="red"/>
      <mrow>
        <mspace width="50px" height="10px" mathbackground="blue"/>
        <mspace width="50px" height="10px"/>
      </mrow>
      <mspace width="100px" height="10" mathbackground="green"/>
    <mmultiscripts>
</math>

VS

<math>
    <mmultiscripts>
      <mspace width="50px" height="50px" mathbackground="red"/>
      <mrow>
        <mspace width="50px" height="10px" mathbackground="blue"/>
      </mrow>
      <mspace width="100px" height="10" mathbackground="green"/>
    <mmultiscripts>
</math>

If the superscript are left aligned then these two <math> elements should render the same. Similarly for the other sup/sub and pre/post combinations.

Comment 6

[Khaled Hosny]

(In reply to Frédéric Wang (:fredw) from comment #5)
> > +            // presecripts should be right aligned
> 
> there is a typo here.

Fixed.

> Perhaps we should explain why we choose the left/right
> alignment and mention the MathML W3C thread for future work.

I added a link to this issue, I think it is even better than just linking to the W3C thread.

> I'm not quite sure why sub/sup scripts are treated differently in your
> patch.

Just sloppy thinking.


> Also, we will probably need to add reftests for that alignment, so that we
> won't regress again in the future (see
> https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests).

Thanks for the reftest idea, I considered this before but didn’t know where to look. I extended the test to cover the four cases of pre and postscripts. The tests are included in the updated patch, or should they be in a commit offf their own?

Comment 7

[Khaled Hosny]

Attachment: Patch V3

Comment 8

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

(In reply to Khaled Hosny from comment #6)
> > Also, we will probably need to add reftests for that alignment, so that we
> > won't regress again in the future (see
> > https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests).
> 
> Thanks for the reftest idea, I considered this before but didn’t know where
> to look. I extended the test to cover the four cases of pre and postscripts.
> The tests are included in the updated patch, or should they be in a commit
> offf their own?

I don't think there is an established rule, but in general we split into small patches when the patch is big in order to help the reviewers. Here the changes are small, so that's fine to put everything in one patch.

Comment 9

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

Comment on attachment 822638 [details] [diff] [review]
Patch V3

Review of attachment 822638 [details] [diff] [review]:
-----------------------------------------------------------------

The changes look good to me, thanks.

I've sent your patch to the try server:
https://tbpl.mozilla.org/?tree=Try&rev=51909f341cda

Comment 10

[Khaled Hosny]

Attachment: Patch for subscript position with italic correction

While at the topic of script positioning, another issue is the placement of postscript 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 garined control with MATH table in the future (where the placement of super and subscripts in both sides can be adjusted by the fonr develeoper).

Still trying to figure out how to write a test for this.

Comment 11

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

Attachment: test-italic-correction.html

Normally, we try to open new bugs for separate issues.

I attach a tentative test case to exhibit the difference with the italic correction (not tested, perhaps epsilon should be smaller).

There is a comment

// XXX need italic correction only *if* there are postscripts ?

I'm wondering how the red square is positioned here:

<math>
<mmultiscripts>
  <mi>f</mi>
  <mprescripts/>
  <mi>x</mi>
  <mi>y</mi>
</mmultiscripts>
  <mspace width="10px" height="10px" mathbackground="red"/>
</math>

Comment 12

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

> Still trying to figure out how to write a test for this.

@Khaled: were you able to use attachment 829522 [details] to write a test?. We can also handle this issue in a follow-up bug...

Comment 13

[Khaled Hosny]

Did not have time to check this, but I think we should move it to a separate bug.

Comment 14

[Khaled Hosny]

Opened bug 945254 to track the italic correction issue, this bug should be closed once the original issue (regression) is fixed.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1cba1f33a49

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1cba1f33a49