Sup/superscripts are centred relative to each other

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

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

Tracking

({regression})

Trunk
mozilla28
All
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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
Posted patch Patch V1 (obsolete) — Splinter Review
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.
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).
Posted patch Patch V2 (obsolete) — Splinter Review
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.
Attachment #819404 - Attachment is obsolete: true
> +            // 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.
(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?
Posted patch Patch V3Splinter Review
Attachment #819473 - Attachment is obsolete: true
(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.
Assignee: nobody → khaledhosny
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: regression
OS: Linux → Windows XP
Hardware: x86_64 → All
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
Attachment #822638 - Flags: review+
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.
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>
> 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...
Did not have time to check this, but I think we should move it to a separate bug.
Attachment #826144 - Attachment is obsolete: true
Opened bug 945254 to track the italic correction issue, this bug should be closed once the original issue (regression) is fixed.
https://hg.mozilla.org/mozilla-central/rev/a1cba1f33a49
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Blocks: 945254
You need to log in before you can comment on or make changes to this bug.