823939 - Incorrect MathML "invalid-markup" with <mmultiscripts>

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

The spec says that the syntax of mmultiscripts is

<mmultiscripts>
    base
     (subscript superscript)*
     [ <mprescripts/> (presubscript presuperscript)* ]
</mmultiscripts>

However, we generate an invalid-markup message with this one:

   <math>
      <mmultiscripts>
	<mtext>base</mtext>
	<mprescripts/>
      </mmultiscripts>
    </math>

That should be easy to fix. If someone wants to work on this, search for ReflowError in

layout/mathml/nsMathMLmmultiscriptsFrame.cpp

Comment 1

[Aditya]

Hi, I am new to development in firefox. Can you give me a general idea about how I should proceed?

Comment 2

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

(In reply to Aditya from comment #1)
> Hi, I am new to development in firefox. Can you give me a general idea about
> how I should proceed?

Hi Aditya, I just sent you general info by mail. Regarding this bug, you may find description and examples for mmultiscripts here:

https://developer.mozilla.org/en-US/docs/MathML/Element/mmultiscripts
https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/mmultiscripts
http://www.w3.org/TR/MathML/chapter3.html#presm.mmultiscripts

I guess the correct behavior for

<math>
  <mmultiscripts>
    <mtext>base</mtext>
      <mprescripts/>
  </mmultiscripts>
</math>

would be to render the same as a <mmulticripts> with "<none/>" scripts (in particular that gives you an idea for a reftest). Currently this render as an "invalid-markup". As I said, this message is created by a call to ReflowError in layout/mathml/nsMathMLmmultiscriptsFrame.cpp. You'll have to figure out which one.

Comment 3

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

@Aditya: I don't know if you've made any progress on this bug, but for your information Mohit started to look at this.

@Mohit: Thanks for your analysis. I would suggest you to write some tests, do some experiments and attach a patch to the bug. That will be more convenient to discuss the changes.

The syntax

<mmultiscripts>
    base
     (subscript superscript)*
     [ <mprescripts/> (presubscript presuperscript)* ]
</mmultiscripts>

means that there is always a base, followed by a repetition of 0 or more (subscript superscript) pairs, optionally followed by the bracket content. The bracket content is a <mprescripts/> followed by a repetition of 0 or more (presubscript presuperscript) pairs. The case that we don't handle is when we have the optional bracket content but that the two repetitions are empty i.e. "<mmultiscripts> base <mprescripts/> </mmultiscripts>".

Comment 4

[[:badanomaly]]

Attachment: fix - "invalid-markup" with <mmultiscripts> fixed.

Comment 5

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

Comment on attachment 698402 [details] [diff] [review]
fix - "invalid-markup" with <mmultiscripts> fixed.

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

OK, that looks good. So the invalid-markup will now just appears when sub/sup pairs do not match (0 != width) or when there is no base (!baseFrame). However with your change, an error will only be reported to the console in the first case. I think you should replace the "NoSubSup" message by a "NoBase" message that will be reported in the second case. See what has been done in bug 553917. In particular, you'll also need to update the Mochitest added in attachment 695862 [details] [diff] [review], otherwise your change will make it fail. As said above, I think that would be good to add reftests to compare the new cases accepted against the equivalent constructions with <none/> scripts ("base"+"mprescripts" as above or "base" alone).

For the documentation about how to write & run the tests, see
https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests
https://developer.mozilla.org/en-US/docs/Mochitest

To make review easier, I suggest you to organize the patches as in bug 553917:
- 1 patch for the code
- 1 patch for the tests
- 1 patch for the localization changes
(https://developer.mozilla.org/en/Mercurial_Queues should be helpful).

Comment 6

[[:badanomaly]]

Attachment: fix - "invalid-markup" with <mmultiscripts> fixed.

Comment 7

[[:badanomaly]]

Attachment: The test files

Comment 8

[[:badanomaly]]

I dont think i understand how to test the patch I have attached ,
so I am attaching the test files that I think are correct.

Comment 9

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

Comment on attachment 698437 [details] [diff] [review]
fix - "invalid-markup" with <mmultiscripts> fixed.

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

Normally you should only ask a "review" when you are sure that your patches are complete. However, for quick feedback on work-in-progress patches you may use the "feedback" field instead.

I assume that you have built Firefox with your changes and tested them. For example, you should no longer see the invalid-markup for the two cases previously mentioned. For the remaining "invalid-markup" cases, check the error console to see if the messages are correctly sent.

The MathML reftests are located in layout/reftests/mathml/. That's where you must add your tests and update the reftest.list. See the reftest documentation to run the reftests (you can use TEST_PATH=layout/reftests/mathml/reftest.list to run the MathML tests only). This will also run your tests after you add them to layout/reftests/mathml/reftest.list.

The MathML mochitests are in layout/mathml/tests/, (again, see the mochitest documentation to run them and you can use TEST_PATH=layout/mathml/tests/). Note that your patch will make the test test_bug553917.html fail and so you must update this test page accordingly.

::: layout/mathml/nsMathMLmmultiscriptsFrame.cpp
@@ +364,4 @@
>      // report an error, encourage people to get their markups in order
>      if (aPlaceOrigin) {
> +      if (0 != width) {
> +        ReportErrorToConsole("SubSupMismatch");

Equivalently and with less modifications, you could just replace use "!baseFrame" in the if and replace "NoSubSup" by "NoBase".

Note that the localization messages are in

dom/locales/en-US/chrome/mathml/mathml.properties

you need to remove the "NoSubSup" string and add a "NoBase" string instead. Also, you'll need to ask a review to i18n folks, so you'd better do the modification to mathml.properties in a separate patch.

Comment 10

[[:badanomaly]]

Attachment: Changes in code.

Comment 11

[[:badanomaly]]

Attachment: Localisation changes

Comment 12

[[:badanomaly]]

Attachment: Test file

Comment 13

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

Comment on attachment 701293 [details] [diff] [review]
Changes in code.

This patch looks ok, thanks.

Comment 14

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

Comment on attachment 701294 [details] [diff] [review]
Localisation changes

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

::: dom/locales/en-US/chrome/mathml/mathml.properties
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  ChildCountIncorrect=Invalid markup: Incorrect number of children for <%1$S/> tag.
>  DuplicateMprescripts=Invalid markup: More than one <mprescripts/> in <mmultiscripts/>.
> +NoBase=Invalid markup: Expected exactly one Base.  Found none.

For consistency with the other messages, I guess you need to mention "in <mmultiscripts/>".

Comment 15

[[:badanomaly]]

Attachment: fix - patch complete with code and localisation chages and test files

Comment 16

[[:badanomaly]]

Attachment: Reftest Output

Comment 17

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

Comment on attachment 706840 [details] [diff] [review]
fix - patch complete with code and localisation chages and test files

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

> fix - patch complete with code and localisation chages and test files

The title of your patch will be used as a commit message. Can you please describe what your patch fixes and follow the format here:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Commit_message_restrictions

::: layout/reftests/mathml/mathml-mmultiscript-base-ref.html
@@ +12,5 @@
> +			</mrow>
> +			</mmultiscripts>
> +		</math>
> +	</body>
> +</html>

Is the <mrow> necessary? i.e. does just <mmultiscripts><mtext>base</mtext></mmultiscripts> work?

::: layout/reftests/mathml/mathml-mmultiscript-mprescript.html
@@ +15,5 @@
> +				<none/>
> +				<none/>
> +				</mrow>
> +			</mmultiscripts/>
> +		</math>

This is not well-formed: "</mmultiscripts/>" should be "</mmultiscripts>". Also, the <mmultiscripts> has only one child (the <mrow>) which is treated as the base. This is not what you want. Try removing the <mrow>.

Comment 18

[[:badanomaly]]

Attachment: <mmultiscripts> now doesnt consider base followed by only <mprescripts> and no (presubscript presuperscript) pair as invalid-markup.

Comment 19

[[:badanomaly]]

Attachment: Reftest Output

Comment 20

[[:badanomaly]]

Attachment: <mmultiscripts> now doesnt consider base followed by only <mprescripts> and no (presubscript presuperscript) pair as invalid-markup.

Comment 21

[[:badanomaly]]

Attachment: <mmultiscripts> now doesnt consider base followed by only <mprescripts> and no (presubscript presuperscript) pair as invalid-markup.

Comment 22

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

Comment on attachment 706892 [details] [diff] [review]
<mmultiscripts> now doesnt consider base followed by only <mprescripts> and no (presubscript presuperscript) pair as invalid-markup.

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

Thanks, I think that version is good.

Asking l10n review for the string change in

dom/locales/en-US/chrome/mathml/mathml.properties

Comment 23

[Axel Hecht [:Pike]]

Comment on attachment 706892 [details] [diff] [review]
<mmultiscripts> now doesnt consider base followed by only <mprescripts> and no (presubscript presuperscript) pair as invalid-markup.

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

I think the error message can be a bit more expressive about the "Base" technical part, and use a localization note on how to handle the translation of it. See the comment below, but that's just after an ad-hoc glance at the google search results.

::: dom/locales/en-US/chrome/mathml/mathml.properties
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  ChildCountIncorrect=Invalid markup: Incorrect number of children for <%1$S/> tag.
>  DuplicateMprescripts=Invalid markup: More than one <mprescripts/> in <mmultiscripts/>.
> +NoBase=Invalid markup: Expected exactly one Base in <mmultiscripts/>.  Found none.

I think we need a comment here on what "Base" is. I couldn't tell if I should translate base or not. I suspect from glancing at some google search results that it is. Should we say "... exactly one base element..." ?

Comment 24

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

I think "base element" is fine. What about "The first child of <mmultiscript/> is the base, that is the element to which scripts are attached". Should it be a "LOCALIZATION NOTE" or a normal comment?

Comment 25

[[:badanomaly]]

Attachment: <mmultiscripts> now doesnt consider base followed by only <mprescripts> and no (presubscript presuperscript) pair as invalid-markup.

Comment 26

[[:badanomaly]]

Attachment: <mmultiscripts> now doesnt consider base followed by only <mprescripts> and no (presubscript presuperscript) pair as invalid-markup

Comment 27

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

@Mohit: what is the status of this bug? I think you can send this patch to the try server to be sure than the reftests work correctly. And ask a review to Karl Tomlinson after that.

Comment 28

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

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/196d06d151a9

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/196d06d151a9