411227 - nsMathMLFrame::ParseNumericValue needs to be tightened up

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=355548#c119

nsMathMLFrame::ParseNumericValue should take a aRequireLengthUnit parameter (and all callers should set it appropriately). Callers should not check IsLengthUnit on the result, since this will treat 0 incorrectly.

Comment 1

[Alvin]

The function nsMathMLFrame::ParseNumericValue just calls nsMathMLElement::ParseNumericValue with last parameter being PR_FALSE. I am wondering why don't we just call nsMathMLElement::ParseNumericValue directly inside the implementation of nsMathMLFrame? Is it for readability purpose?

This is my first bug, so please forgive me if I am asking silly questions. 

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The idea is to replace calls to nsMathMLFrame::ParseNumericValue with direct calls to nsMathMLElement::ParseNumericValue. Some of those calls should pass PR_TRUE for aRequireLengthUnit. You'll need to read the MathML spec and some of our MathML code to figure out which ones.

Comment 3

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

Attachment: XXXfredw comments

Now that bug 677036 is fixed, the parsing is more consistent with the MathML3 spec:

http://www.w3.org/TR/MathML3/chapter2.html#fund.units
http://www.w3.org/TR/MathML3/chapter3.html#id.3.3.6.2

I attach the result of the command

grep -A4 -n XXXfredw layout/mathml/*.cpp content/mathml/content/src/nsMathMLElement.cpp

that locates the places where some work remains to do.

Comment 4

[James Kitchener (:jkitch)]

I'm interested in working on this bug, although I  have a few questions.

Is the overall goal from question 2 still valid?  (I'm aware the details have changed as the aRequireLengthUnit argument appears to have been replaced by some flags)

Just to be explicit regarding the XXXfredw comments, do I need to update the allow unitless/negative flags to match the specification?

Are there any tests I should be running/updating?

Comment 5

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

After bug 677036, the structure has changed a bit. Now nsMathMLFrame::ParseNumericValue not only calls nsMathMLElement::ParseNumericValue but also converts the result to absolute, relative or default length. So the goal is no longer to get rid of nsMathMLFrame::ParseNumericValue but to verify that the flags for negative/unitless values are correct. Some of the changes may be easy to do (e.g forbid negative values for script shifts) while others should better be handled in separate bugs. 

In general, I think the RelaxNG schema allows unitless and negative values

http://www.w3.org/Math/RelaxNG/mathml3/mathml3-presentation.rnc

so they are syntactically correct. But we want to be sure that they make sense for each attributes so you'll have to read the MathML3 REC to understand what they are supposed to do. According to the MathML3 REC, unitless values are supposed to represent a multiple of the default value. But sometimes this definition is not really clear in the MathML3 REC so I suggest you to refer to the WG draft instead and ask clarification to the Math WG if necessary:

http://www.w3.org/Math/draft-spec/

Another useful thing would be to write reftests for these attributes that use nsMathMLFrame::ParseNumericValue (not only those marked XXXfredw). For example, an obvious reftest for unitless values is to compare it against a % or against an absolute value set to N times the default (once you figured out what the default is). For example

<mfrac linethickness="200%"><mi>x</mi><mn>2</mn></mfrac>

against

<mfrac linethickness="2"><mi>x</mi><mn>2</mn></mfrac>

Invalid values should be ignored so a typical reftest would be

<mfrac linethickness="-2px"><mi>x</mi><mn>2</mn></mfrac>

against

<mfrac><mi>x</mi><mn>2</mn></mfrac>

That will also allow to verify potential assertion errors in debug builds (for instance those due to negative values).

Note that there may be conflicts with this bug and the work in bug 553917 or bug 794554. I recommend you to start to write reftests like those mentioned above, so that you can get an idea of the MathML syntax if you are not already familiar with it and you won't have to worry about the changes in the other bugs. The doc is there:

https://developer.mozilla.org/en/Creating_reftest-based_unit_tests

The MathML tests are in layout/reftests/mathml/ (reftests), layout/mathml/tests and layout/mathml/crashtests.

Don't hesitate to ask if you need more information.

Comment 6

[James Kitchener (:jkitch)]

Attachment: reftests

Reftests are attached.  I've also had a look at the XXXfredw comments.

sub/supscripts.
No mention of negative values in specifications.  As a minimum adjustment, we can forbid negative values and still remain in compliance.

nsMathMLmoFrame:
Forbid negative values.  The elements supporting negative spacing were explicitly enumerated in 3.1.8.2.
I'm undecided about relative values.  In 2.1.5.2 they are defined as multiples of the default value, so applying it to the current leading/trailing space may not be correct.

nsMathMLElement
 > // XXXfredw Does a relative unit give a multiple of the default value?
According to section 2.1.5.2, this is correct.  A unitless length is essentially a special case of a percentage value and both are multiples of the default/reference value.
I'm inclined to allow unitless values.  A scaling factor makes sense in this instance and I think the code allows % values.

The PARSE_ALLOW_UNITLESS flag could be generalised to PARSE_ALLOW_RELATIVE, covering % as well, but that should probably be left to another bug.

Did you want me to upload a patch now, or wait until the other bugs are fixed?

Comment 7

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

(In reply to James Kitchener from comment #6)
> Created attachment 669923 [details] [diff] [review]
> reftests
> 
> Reftests are attached.  I've also had a look at the XXXfredw comments.
> 

Thanks, I'll have a look at the reftests. After the changes, the XXXfredw should be removed and probably replaced by appropriate spec reference on which we base our decision.

> sub/supscripts.
> No mention of negative values in specifications.  As a minimum adjustment,
> we can forbid negative values and still remain in compliance.

Yes, I think it does not make sense to have them. We can forbid negative values. The cases where the shift is < 0 is ignored somewhere else in the code anyway (so this verification could be removed if we control it during the parsing). It could be a good idea to write tests checking negative shifts.

> 
> nsMathMLmoFrame:
> Forbid negative values.  The elements supporting negative spacing were
> explicitly enumerated in 3.1.8.2.

I agree that we can do that, although I don't see stated explicitly that lspace/rspace can not be made negative and the RelaxNG schema does not forbid that. I think we should ask the Math WG (I also wonder how <mtext> can be used to make negative spaces... do they mean <mo>?).

> I'm undecided about relative values.  In 2.1.5.2 they are defined as
> multiples of the default value, so applying it to the current
> leading/trailing space may not be correct.

This is the kind of change that I suspect would require more work and I guess we don't need to handle it in this bug.

> 
> nsMathMLElement
>  > // XXXfredw Does a relative unit give a multiple of the default value?
> According to section 2.1.5.2, this is correct.  A unitless length is
> essentially a special case of a percentage value and both are multiples of
> the default/reference value.
> I'm inclined to allow unitless values.  A scaling factor makes sense in this
> instance and I think the code allows % values.
> 
> The PARSE_ALLOW_UNITLESS flag could be generalised to PARSE_ALLOW_RELATIVE,
> covering % as well, but that should probably be left to another bug.
> 

I believe that each time % values are allowed then so are unitless values. The point is to check whether relative (% or unitless) values really give a multiple of the default value. For example, the obvious modification is not likely to work for lspace/rspace discussed above. But when modifications of this kind are easily made and give the correct result, we can just do them in this bug (and perhaps write the corresponding tests).

> Did you want me to upload a patch now, or wait until the other bugs are
> fixed?

That take a long time before patches land to trunk. I suggest you to try mercurial queues:

https://developer.mozilla.org/en-US/docs/Mercurial_Queues

then you can download the other patches in you mercurial queue and create your new patches on top of these. That way, you can continue the work without waiting for the other bugs to be fixed.

Comment 8

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

Comment on attachment 669923 [details] [diff] [review]
reftests

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

The tests looks good, thanks. Maybe you can add tests of this kind for the attributes you are going to work on in this bug.

::: layout/reftests/mathml/number-size-1.xhtml
@@ +1,4 @@
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +<body>
> +  <math xmlns="http://www.w3.org/1998/Math/MathML">
> +    <mn mathsize="200%">2</mn>

So IIUC, you are basically assuming that the default size is 12pt. I don't know if it's true, but you can force this by adding style="font-size: 12pt;" on the <math> element (here and in the reference page). Alternatively, you can also just replace the mathsize by style="font-size: 200%;" in the reference page.

@@ +2,5 @@
> +<body>
> +  <math xmlns="http://www.w3.org/1998/Math/MathML">
> +    <mn mathsize="200%">2</mn>
> +    <mo>=</mo>
> +    <mn mathsize="24pt">2</mn>	

This node does not really test anything since it is equal to the one in the reference. Perhaps you mean mathsize="2"?

Comment 9

[James Kitchener (:jkitch)]

Attachment: wip

The number-size-1 reftest doesn't pass yet

mathsize="2" and mathsize="200%" are giving different answers.

I've added PARSE_ALLOW_UNITLESS to the mathsize section of nsMathMLElement.cpp, but is more work needed to properly support it?

Comment 10

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

(In reply to James Kitchener from comment #9)
> I've added PARSE_ALLOW_UNITLESS to the mathsize section of
> nsMathMLElement.cpp, but is more work needed to properly support it?

Yes, I think so. Read what is done in nsMathMLElement::ParseNumericValue: at the end unitless values are converted to a float. For attributes on the layout/ side, this value is used to compute a multiple of the default value in nsMathMLFrame::ParseNumericValue.

But mathsize is parsed on the content/ side, so you need to do as for "%" and use nsCSSValue::SetPercentValue instead of nsCSSValue::SetFloatValue. I guess you can do this by adding an additional flag CONVERT_UNITLESS_TO_PERCENT to nsMathMLElement::ParseNumericValue.

Comment 11

[James Kitchener (:jkitch)]

Attachment: patch

Negative script shifts are ignored
Negative leading/trailing spaces for operators are ignored.  I interpreted 'MathML elements that permit "negative spacing", namely mspace, mpadded, and mtext' in 3.1.8.2 as an exhaustive list.

No action taken regarding relative operator spacing.  Enabling it may give the wrong result at times, and %/unitless behaviour is currently consistent.

mathsize/scriptminsize treat unitless values like %

Tests for the affected operations have been added.

Comment 12

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

(In reply to James Kitchener from comment #11)
> Created attachment 671361 [details] [diff] [review]
> patch
> 
> Negative script shifts are ignored
> Negative leading/trailing spaces for operators are ignored.  I interpreted
> 'MathML elements that permit "negative spacing", namely mspace, mpadded, and
> mtext' in 3.1.8.2 as an exhaustive list.
> 
> No action taken regarding relative operator spacing.  Enabling it may give
> the wrong result at times, and %/unitless behaviour is currently consistent.
> 
> mathsize/scriptminsize treat unitless values like %
> 
> Tests for the affected operations have been added.

Thanks, that looks great. I don't think I'll have time to come back to this before next week but I have asked the Math WG for clarification about lspace/rspace:

http://lists.w3.org/Archives/Public/www-math/2012Oct/0014.html

Comment 13

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

Regarding lspace/rspace, David Carlisle said (unofficial answer) that in section  3.1.8.2, mtext should be replaced by mo and so negatives values are authorized. In that case that would not be a good idea to send an error to the console for such values (although we can still ignore these values for the moment).

When we get and official answer from the Math WG, we can open a new bug to fix the implementation of negative and relative lspace/rspace values and mention the bug in the C++ source.

Comment 14

[James Kitchener (:jkitch)]

Attachment: patch v2

Negative values for operator lspace/rspace are now silently ignored.  

In addition to setting nsMathMLElement::PARSE_ALLOW_NEGATIVE, there is an explicit check to ensure a non-negative value.  This was added to ensure negative values had no effect, as opposed to being rounded to +1 pixel.

The alternative was to set PARSE_SUPPRESS_WARNINGS, although this would have hidden unit related errors as well.

Comment 15

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

Comment on attachment 672220 [details] [diff] [review]
patch v2

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

Neil gave more feedback on the Math WG, confirming David's response.

I would prefer to keep attachment 671361 [details] [diff] [review] and I've opened bug 805926 for future work on mo@lspace/mo@rspace. Can you please just update attachment 671361 [details] [diff] [review] to have the comments in the C++ source refer to bug 805926?

Comment 16

[James Kitchener (:jkitch)]

Attachment: patch 671361 updated

patch 671361 with updated comments

Comment 17

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

Comment on attachment 675933 [details] [diff] [review]
patch 671361 updated

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

Thanks, that looks good. But for the rspace comment, I think you can just apply the same changes as you did for lspace. r=me with this modification.

Are you able to test your patch on the TryServer?

Comment 18

[James Kitchener (:jkitch)]

Corrected patch.

I do not have Tryserver access.  Would you mind submitting it?

Comment 19

[James Kitchener (:jkitch)]

Attachment: revised patch

patch attached this time.

Comment 20

[James Kitchener (:jkitch)]

I apologise for the repeated messages, but I forgot to mention that this patch depends upon the patches from bug 553917.

Comment 21

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

(In reply to James Kitchener from comment #20)
> I apologise for the repeated messages, but I forgot to mention that this
> patch depends upon the patches from bug 553917.

Right, I forgot about that. So let's wait for bug 553917 to be fixed before taking this patch.

Comment 22

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

@James: I guess bug 553917 is going to be fixed soon and so we will be able to take your patch. I just sent your changes to the TryServer:

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

and it seems that

layout/reftests/bugs/355548-2.xml
layout/reftests/bugs/355548-3.xml

fail. Can you please verify why? I suspect it is something related to unitless scriptlevel and that the tests should be fixed.

Comment 23

[James Kitchener (:jkitch)]

I didn't realise there were some mathml tests that lived outside of the mathml directory.

>layout/reftests/bugs/355548-3.xml

This test assumed a unitless value for mathsize was invalid.  Fixing this is trivial, although it exposed another bug.

The current behaviour for unitless and percent values is to calculate a multiple of the *current* value, not the default one according to the specifications.   

Should I leave unitless values consistent with percent values even if incorrect? Disabling unitless will not touch percent behaviour.  (Fixing this behaviour was left to bug 805926 in the case of <mo>)

>layout/reftests/bugs/355548-2.xml

It turns out that scriptminsize has no awareness of unitless or percent values.  The code within nsRuleNode accepts a subset of units that exclude percent values.  The test fails because the special case for '0' is no longer applied.

Depending on how supporting percent values is implemented, it would probably suffer from the current/default value bug.

Is this also to be left to a follow-up bug?  In the meantime should I remove the unitless flag?  It doesn't do anything except break a special case for '0'.

Comment 24

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

Comment on attachment 675943 [details] [diff] [review]
revised patch

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

It is said in chapter 2 that percent and unitless behave the same and that they give a multiple of the default value. In chapter 3, the default value of mathsize is "inherited" which I interpret as being the "current" value. Hence I think your change is correct and that the test should just be fixed.

::: content/mathml/content/src/nsMathMLElement.cpp
@@ +502,5 @@
>      nsCSSValue* scriptMinSize = aData->ValueForScriptMinSize();
>      if (value && value->Type() == nsAttrValue::eString &&
>          scriptMinSize->GetUnit() == eCSSUnit_Null) {
> +      ParseNumericValue(value->GetStringValue(), *scriptMinSize,
> +                        PARSE_ALLOW_UNITLESS | CONVERT_UNITLESS_TO_PERCENT,

The default value of scriptminsize is "8pt". I think after the call to ParseNumericValue, you should test whether scriptminsize->GetUnit() == eCSSUnit_Percent. If so, I guess something like

scriptminsize->SetFloatValue(8.0 * scriptminsize->GetPercentValue(), eCSSUnit_Point);

will give the right value? Then the style system won't need additional support for percent values.

(then fix the tests if necessary)

Comment 25

[James Kitchener (:jkitch)]

Attachment: patch with fixed tests

Fixes have been applied.

I've run the complete set of reftests and the change hasn't broken anything else.

Comment 26

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

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

Comment 27

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

Changes to document:

- unitless values for scriptminsize, mathsize and fontsize are now allowed.
- percent values for scriptminsize are now allowed.
- percent & unitless values of scriptminsize are interpreted as a multiple of "8pt".

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Ed Morley [:emorley]]

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

Comment 30

[Florian Scholz (Open Web Docs)]

General release notes:
https://developer.mozilla.org/en-US/docs/Firefox_20_for_developers#MathML

Notes added to attribute overview page:
https://developer.mozilla.org/en-US/docs/MathML/Attributes

Attribute documentation on element pages:

scriptminsize:
https://developer.mozilla.org/en-US/docs/MathML/Element/mstyle#Gecko-specific_notes

mathsize:
https://developer.mozilla.org/en-US/docs/MathML/Element/mi#Gecko-specific_notes
https://developer.mozilla.org/en-US/docs/MathML/Element/mn#Gecko-specific_notes
https://developer.mozilla.org/en-US/docs/MathML/Element/mo#Gecko-specific_notes
https://developer.mozilla.org/en-US/docs/MathML/Element/ms#Gecko-specific_notes
https://developer.mozilla.org/en-US/docs/MathML/Element/mtext#Gecko-specific_notes