Closed Bug 823939 Opened 7 years ago Closed 7 years ago

Incorrect MathML "invalid-markup" with <mmultiscripts>

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: fredw, Assigned: badanomaly)

References

()

Details

(Keywords: helpwanted, Whiteboard: [mentor=fredw][lang=c++])

Attachments

(2 files, 12 obsolete files)

73.02 KB, text/plain
Details
6.82 KB, patch
Pike
: review+
fredw
: review+
Details | Diff | Splinter Review
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
Hi, I am new to development in firefox. Can you give me a general idea about how I should proceed?
(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.
@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>".
Assignee: nobody → mohit.www
Status: NEW → ASSIGNED
Attachment #698402 - Flags: review?(fred.wang)
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).
Attachment #698402 - Flags: review?(fred.wang)
Attachment #698402 - Flags: review-
Attachment #698402 - Flags: feedback+
Attachment #698402 - Attachment is obsolete: true
Attached file The test files (obsolete) —
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.
Attachment #698437 - Flags: review?(fred.wang)
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.
Attachment #698437 - Flags: review?(fred.wang)
Attachment #698437 - Flags: review-
Attachment #698437 - Flags: feedback+
Blocks: 827713
Attached patch Changes in code. (obsolete) — Splinter Review
Attachment #698437 - Attachment is obsolete: true
Attachment #701293 - Flags: feedback?(fred.wang)
Attached patch Localisation changes (obsolete) — Splinter Review
Attached file Test file (obsolete) —
Attachment #698438 - Attachment is obsolete: true
Comment on attachment 701293 [details] [diff] [review]
Changes in code.

This patch looks ok, thanks.
Attachment #701293 - Flags: feedback?(fred.wang) → feedback+
Attachment #701294 - Attachment is patch: true
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/>".
Attachment #701294 - Flags: feedback+
Attachment #701293 - Attachment is obsolete: true
Attachment #701295 - Attachment is obsolete: true
Attachment #701294 - Attachment is obsolete: true
Attachment #706840 - Flags: review?(fred.wang)
Attached file Reftest Output (obsolete) —
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>.
Attachment #706840 - Flags: review?(fred.wang)
Attachment #706840 - Flags: review-
Attachment #706840 - Flags: feedback+
Attachment #706840 - Attachment is obsolete: true
Attachment #706842 - Attachment is obsolete: true
Attached file Reftest Output
Attachment #706879 - Flags: review?(fred.wang)
Attachment #706879 - Attachment is obsolete: true
Attachment #706879 - Flags: review?(fred.wang)
Attachment #706882 - Flags: review?(fred.wang)
Attachment #706882 - Attachment is obsolete: true
Attachment #706882 - Flags: review?(fred.wang)
Attachment #706892 - Flags: review?(fred.wang)
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
Attachment #706892 - Flags: review?(l10n)
Attachment #706892 - Flags: review?(fred.wang)
Attachment #706892 - Flags: review+
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..." ?
Attachment #706892 - Flags: review?(l10n) → review-
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?
Attachment #706892 - Attachment is obsolete: true
Attachment #710154 - Flags: review?(fred.wang)
Attachment #710719 - Flags: review?(l10n) → review+
@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.
Attachment #710719 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/196d06d151a9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.