Bug 120069 – [MathML] Implement the subscriptshift and superscriptshift attributes

• Component: MathML
• Status: RESOLVED FIXED

Webkit MATHML doesnot have support for subscriptshift and superscriptshift attributes.
http://www.w3.org/TR/MathML3/chapter3.html.

Depends on https://bugs.webkit.org/show_bug.cgi?id=99618

Created attachment 209346 [details]
Attaching File to show changes related subscriptshift and superscriptshift attribute

Hi Frederic. I have done changes related to subscriptshift and superscriptshift attributes but since its dependent on 99619 I cannot submit the patch. I am attaching the .cpp file. Let me know how should I go about now?

I have also added the attributes in mathattrs.in.

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

(In reply to comment #3)
> Hi Frederic. I have done changes related to subscriptshift and superscriptshift attributes but since its dependent on 99619 I cannot submit the patch. I am attaching the .cpp file. Let me know how should I go about now?
> 
> I have also added the attributes in mathattrs.in.

Thanks Gurpreet. Please attach a patch, that's much easier to see your changes and will be needed to ask a review anyway. I'm not sure what your problem is? You can for example commit the patch of bug 99619 in a local branch, then do your changes and finally execute a diff to create your patch. See the doc of svn/git or ask help on irc if you're not familiar with these tools.

Chris is reviewing my patch for mmultiscripts but it's a bit big so it will take some time. Working on the easier bug 118904 as I suggested by email is probably a better way for you to get familiar with the WebKit review process & unit test framework and it won't be blocked by other bugs.

Regarding your patch, the "for" loop is essentially checking all the subscript/superscript pairs (there can be many for mmultiscripts) and computing the maximum for top/bottom paddings. So I don't think incrementing these paddings by superscriptShift/subscriptShift at each step is what you want. Rather, I think the superscriptShift/subscriptShift values can be computed before entering the loop and then used in the computation of minBaseline and minExtendUnderBaseline at each step.

(In reply to comment #4)
> (In reply to comment #3)
> > Hi Frederic. I have done changes related to subscriptshift and superscriptshift attributes but since its dependent on 99619 I cannot submit the patch. I am attaching the .cpp file. Let me know how should I go about now?
> > 
> > I have also added the attributes in mathattrs.in.
> 
> Thanks Gurpreet. Please attach a patch, that's much easier to see your changes and will be needed to ask a review anyway. I'm not sure what your problem is? You can for example commit the patch of bug 99619 in a local branch, then do your changes and finally execute a diff to create your patch. See the doc of svn/git or ask help on irc if you're not familiar with these tools.
> 
> Chris is reviewing my patch for mmultiscripts but it's a bit big so it will take some time. Working on the easier bug 118904 as I suggested by email is probably a better way for you to get familiar with the WebKit review process & unit test framework and it won't be blocked by other bugs.
> 
> Regarding your patch, the "for" loop is essentially checking all the subscript/superscript pairs (there can be many for mmultiscripts) and computing the maximum for top/bottom paddings. So I don't think incrementing these paddings by superscriptShift/subscriptShift at each step is what you want. Rather, I think the superscriptShift/subscriptShift values can be computed before entering the loop and then used in the computation of minBaseline and minExtendUnderBaseline at each step.

Hi Frederic. I have made the changes as per your comments.

My problem is that when I merge the changes of bug 99618 to my code and on top of that do my changes, on doing svn diff/git diff it will take all the changes which I took from 99618. That diff will not have just my changes so I attached the .cpp file.

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

(In reply to comment #5)
> My problem is that when I merge the changes of bug 99618 to my code and on top of that do my changes, on doing svn diff/git diff it will take all the changes which I took from 99618. That diff will not have just my changes so I attached the .cpp file.

You can create a local git branch and commit the changes of bug 99618 on that local branch. Then make your changes and "git diff" will only create patch for these changes. I'm not sure about svn but I guess something similar is possible.

Created attachment 211752 [details]
Patch

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

Thanks Gurpreet, that looks good. A few comments:

- Check the indentation in the test files. Also the meta tag and xmlns:mml attributes are not really needed, so you can remove them.

- I think the shifts must not be negative, so you should call parseMathMLLength with allowNegative = false.

- I think setting the topPadding/bottomPadding to the shift values is not quite correct, that's likely to give too large values. Instead, you can still parse them to some superScriptShift and subScriptShift values outside the for loop. Then includes the shifts in the minBaseline/minExtendUnderBaseline computation:

            LayoutUnit minBaseline = max<LayoutUnit>(fontSize / 3 + 1 + superscriptBaseline, superscriptHeight + axis + superScriptShift);

and

            LayoutUnit minExtendUnderBaseline = max<LayoutUnit>(fontSize / 5 + 1 + subscriptUnderItsBaseline, subscriptHeight + subScriptShift - axis);

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

Also, you can not group several mismatch tests in a single test (if one passes the mismatch condition, that won't guarantee that the others do too). So you should split your test into several tests i.e. test each element and attribute independently. Note that mmultiscripts also support these shifts attributes.

You may also remove the FIXME comment that refers to this bug (including the mention of TeX heuristics). Instead, say that the script shifts are also parts of the "layout rules".

Created attachment 211786 [details]
Patch

(In reply to comment #9)
> Also, you can not group several mismatch tests in a single test (if one passes the mismatch condition, that won't guarantee that the others do too). So you should split your test into several tests i.e. test each element and attribute independently. Note that mmultiscripts also support these shifts attributes.
> 
> You may also remove the FIXME comment that refers to this bug (including the mention of TeX heuristics). Instead, say that the script shifts are also parts of the "layout rules".

Hi Frederic. Thanks for the review. Have done the changes as per the review comments.

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

Thanks. The changes look OK in general, although I think you still need to test the subscriptshift and superscript shifts independently. Alternatively, you can try to write an == reftest ; that will allow you to group the tests and to make the testing to be more accurate. One technique is to use absolute positions like this:

    <div style="position: absolute; top: 50px;">
      <math>
        <mspace height="100px" depth="100px"/>
        <msup subscriptshift="50px">
          <mspace width="20px" height="10px" depth="10px" mathbackground="blue"/>
          <mspace width="20px" height="10px" depth="10px" mathbackground="red"/>
        </msup>
      </math>
    </div>

    <div style="position: absolute; top: 0px;">
      <math>
        <mspace height="100px" depth="100px"/>
        <mspace width="100px" height="10px" depth="10px" mathbackground="black"/>
      </math>
    </div>

The red mspace in the first div should be shifted upwards by 50px. The second div is shift by -50px with respect to the first div. So the black rectangle should cover the red mspace (perhaps you'll need to tweak the height a bit to workaround rounding or antialiasing errors). That means that you can write an -expected file with e.g. "red" replaced by "green". If the script is positioned as expected, the two files should be equal. Of course you can do that for the other elements too.

BTW, I'm not an "official" WebKit reviewer, so e.g. Chris or Martin will have to review your patch.

[Darin Adler]

Comment on attachment 211786 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=211786&action=review

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:296
> +    LayoutUnit superScriptShiftValue = 0;
> +    LayoutUnit subScriptShiftValue = 0;

superscript and subscript are single words, not pairs of words, so the s in "script" should not be capitalized

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:300
> +    if (m_kind == Sub || m_kind == SubSup || m_kind == Multiscripts)
> +        parseMathMLLength(scriptElement->getAttribute(MathMLNames::subscriptshiftAttr), subScriptShiftValue, style(), false);
> +    if (m_kind == Super || m_kind == SubSup || m_kind == Multiscripts)
> +        parseMathMLLength(scriptElement->getAttribute(MathMLNames::superscriptshiftAttr), superScriptShiftValue, style(), false);

These can both be fastGetAttribute instead of just getAttribute.

(In reply to comment #12)
> Thanks. The changes look OK in general, although I think you still need to test the subscriptshift and superscript shifts independently. Alternatively, you can try to write an == reftest ; that will allow you to group the tests and to make the testing to be more accurate. One technique is to use absolute positions like this:
> 
>     <div style="position: absolute; top: 50px;">
>       <math>
>         <mspace height="100px" depth="100px"/>
>         <msup subscriptshift="50px">
>           <mspace width="20px" height="10px" depth="10px" mathbackground="blue"/>
>           <mspace width="20px" height="10px" depth="10px" mathbackground="red"/>
>         </msup>
>       </math>
>     </div>
> 
>     <div style="position: absolute; top: 0px;">
>       <math>
>         <mspace height="100px" depth="100px"/>
>         <mspace width="100px" height="10px" depth="10px" mathbackground="black"/>
>       </math>
>     </div>
> 
> The red mspace in the first div should be shifted upwards by 50px. The second div is shift by -50px with respect to the first div. So the black rectangle should cover the red mspace (perhaps you'll need to tweak the height a bit to workaround rounding or antialiasing errors). That means that you can write an -expected file with e.g. "red" replaced by "green". If the script is positioned as expected, the two files should be equal. Of course you can do that for the other elements too.
> 
> BTW, I'm not an "official" WebKit reviewer, so e.g. Chris or Martin will have to review your patch.

"although I think you still need to test the subscriptshift and superscript shifts independently."

I have separated those. For multiscripts and msubsup also need to test subscriptshift and superscript shifts independently?

(In reply to comment #13)
> (From update of attachment 211786 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211786&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:296
> > +    LayoutUnit superScriptShiftValue = 0;
> > +    LayoutUnit subScriptShiftValue = 0;
> 
> superscript and subscript are single words, not pairs of words, so the s in "script" should not be capitalized
> 
> > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:300
> > +    if (m_kind == Sub || m_kind == SubSup || m_kind == Multiscripts)
> > +        parseMathMLLength(scriptElement->getAttribute(MathMLNames::subscriptshiftAttr), subScriptShiftValue, style(), false);
> > +    if (m_kind == Super || m_kind == SubSup || m_kind == Multiscripts)
> > +        parseMathMLLength(scriptElement->getAttribute(MathMLNames::superscriptshiftAttr), superScriptShiftValue, style(), false);
> 
> These can both be fastGetAttribute instead of just getAttribute.

Thanks Darin.

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

(In reply to comment #14)
> I have separated those. For multiscripts and msubsup also need to test subscriptshift and superscript shifts independently?

Strictly speaking yes, since it could be that msup@superscriptshift, msub@subscriptshift, msubsup@subscriptshift work but not msubsup@superscriptshift. However, I'm not sure it's really necessary here.

A more serious issue is that an "expected-mismatch" reftest is not really accurate. For example if you enter subscriptshift="50px" and your code incorrectly interprets that as "200px", then that won't be detected by your test. If you use an "expected" reftest like the one I proposed, then you know that the script has moved in the location delimited by the black rectangle, so that's slightly more accurate. You could also probably do some Javascript test with getBoundingClientRect to verify the value of the script shift.

Anyway, your patch is OK to me and it has been approved by Darin so you can send it to the commit-queue if you want...

Created attachment 211890 [details]
Patch

(In reply to comment #16)
> (In reply to comment #14)
> > I have separated those. For multiscripts and msubsup also need to test subscriptshift and superscript shifts independently?
> 
> Strictly speaking yes, since it could be that msup@superscriptshift, msub@subscriptshift, msubsup@subscriptshift work but not msubsup@superscriptshift. However, I'm not sure it's really necessary here.
> 
> A more serious issue is that an "expected-mismatch" reftest is not really accurate. For example if you enter subscriptshift="50px" and your code incorrectly interprets that as "200px", then that won't be detected by your test. If you use an "expected" reftest like the one I proposed, then you know that the script has moved in the location delimited by the black rectangle, so that's slightly more accurate. You could also probably do some Javascript test with getBoundingClientRect to verify the value of the script shift.
> 
> Anyway, your patch is OK to me and it has been approved by Darin so you can send it to the commit-queue if you want...

Added new and modified test files.

(In reply to comment #13)
> (From update of attachment 211786 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211786&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:296
> > +    LayoutUnit superScriptShiftValue = 0;
> > +    LayoutUnit subScriptShiftValue = 0;
> 
> superscript and subscript are single words, not pairs of words, so the s in "script" should not be capitalized
> 
> > Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:300
> > +    if (m_kind == Sub || m_kind == SubSup || m_kind == Multiscripts)
> > +        parseMathMLLength(scriptElement->getAttribute(MathMLNames::subscriptshiftAttr), subScriptShiftValue, style(), false);
> > +    if (m_kind == Super || m_kind == SubSup || m_kind == Multiscripts)
> > +        parseMathMLLength(scriptElement->getAttribute(MathMLNames::superscriptshiftAttr), superScriptShiftValue, style(), false);
> 
> These can both be fastGetAttribute instead of just getAttribute.

Hi Darin. Uploaded new patch. Can you please have a look?

[WebKit Commit Bot]

Comment on attachment 211890 [details]
Patch

Clearing flags on attachment: 211890

Committed r156036: <http://trac.webkit.org/changeset/156036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

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

Mass change: add WebExposed keyword to help MDN documentation.