771904 - Update parsing and demo pages for mtable@align

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

See http://lists.w3.org/Archives/Public/www-math/2012Jun/0015.html

The syntax for mtable@align is

"\s*(top|bottom|center|baseline|axis)(\s*-?[0-9]+)?\s*"

The parsing is implemented here in Gecko:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmtableFrame.cpp#271

Note that the author does not seem to assume that there is a space and he is even saying that the lack of separator is a shame. The function essentially calls a "Cut" function to remove the alignment name then calls a "ToInteger" function to compute the rownumber. Apparently this "ToInteger" function ignores the space around the number and so Gecko accepts the syntax with optional spaces between the alignment name and the number as well as optional spaces at the end of the attribute value. But spaces at the beginning of the attribute value are not accepted.

We should also update

http://www.mozilla.org/projects/mathml/demo/mtable.html

to use the correct syntax.

Comment 1

[Matt Brubeck (:mbrubeck)]

Mohit Sinha is interested in working on this bug.

Comment 2

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

(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> Mohit Sinha is interested in working on this bug.

Yes, he contacted me yesterday and I gave me some hints to start with.

Actually, my first comment is wrong, the new syntax is

"\s*(top|bottom|center|baseline|axis)(\s+-?[0-9]+)?\s*"

with mandatory space between the alignment name and the row number. But given that some of the pages may use the former syntax e.g.

http://www.mozilla.org/projects/mathml/demo/mtable.html

it may be better to keep the space between the alignment name and the row number. Anyway "ToInteger" seems to allow space around the row number. So I think the only thing to do is to use "CompressWhitespace" to remove possible the leading space.

Comment 3

[Mohit Sinha]

Attachment: changed 'baseline-1' to 'baseline -1'

Comment 4

[Mohit Sinha]

Attachment: changed 'baseline-1' to 'baseline -1'

Comment 5

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

(In reply to Mohit Sinha from comment #4)
> Created attachment 641089 [details] [diff] [review]
> changed 'baseline-1' to 'baseline -1'

@Mohit: the validator also indicates an error with align="center-1"
http://validator.w3.org/nu/?doc=http%3A%2F%2Fwww.mozilla.org%2Fprojects%2Fmathml%2Fdemo%2Fmtable.html

Comment 6

[Mohit Sinha]

Attachment: changed 'baseline-1' to 'baseline -1' and 'centre-1' to 'centre -1'

Comment 7

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

Comment on attachment 641100 [details] [diff] [review]
changed 'baseline-1' to 'baseline -1' and 'centre-1' to 'centre -1'

Thank you, that looks good.

BTW, I think UI-review is for changes of the Firefox User Interface. For this kind of patch, "normal" review is fine.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 641100 [details] [diff] [review]
changed 'baseline-1' to 'baseline -1' and 'centre-1' to 'centre -1'

r107332

Comment 9

[Mohit Sinha]

Attachment: compressingwhitespace

Comment 10

[Mohit Sinha]

There are some problems in the reftest with 
layout/reftest/mathml/maction-dynamic-1.html & layout/reftest/mathml/maction-dynamic-1-ref.html

These problems are not due to my patch , but were there from before. 

The visible differences are clearly seen.

Can somebody verify it?

Comment 11

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

Comment on attachment 641611 [details] [diff] [review]
compressingwhitespace

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

For the reftests, maybe to make the comparison clearer I would put the "canonical" syntax "center -3" everywhere in mtable-align-whitespace-ref.html and the other syntax "center-3", "center -3  ", "   center -3" etc in mtable-align-whitespace.html. Also, that would be good to test other characters for whitespaces: " ", "	", "
" "&#xA".

::: layout/mathml/nsMathMLmtableFrame.cpp
@@ +275,1 @@
>  

Can you please replace "top5" by "top 5", "axis-1" by "axis -1" and add the comment

"The REC says that the syntax is '\s*(top|bottom|center|baseline|axis)(\s+-?[0-9]+)?\s*'. The parsing could have been simpler with that syntax, but for backward compatibility we make optional the whitespaces between the alignment name and the row number."?

@@ +288,5 @@
>    aRowIndex = 0;
>    aAlign = eAlign_axis;
>    PRInt32 len = 0;
> +
> +  aValue.CompressWhitespace(true,false);  

There should be a space after the comma (actually Trim will work too, although CompressWhitespace is better if we implement the right syntax later).

Can you please add a comment "We only need to remove leading whitespaces because ToInteger ignores the whitespaces around the number."

Comment 12

[Mohit Sinha]

Attachment: compressingwhitespace

I have changed the comments in nsMathMLmtableFrame.cpp file and after the comma put a whitespace.
I have made changes to the html's.

Comment 13

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

Thanks Mohit.

- Please read https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch again: you're supposed to set the review flag to "?" to ask a review to one of the peer here: https://wiki.mozilla.org/Modules/All#MathML. Alternatively, you can set feedback to "?" is you don't think your patch is finished and just want some feedback.
  
- https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
  Be sure that line length is 80 characters or less in the comments you added.

- Can you add tests for characters " ", "	", "
" "&#xA"? For example "&#xA
	 center&#xA
	 -3&#xA
	 ".

Comment 14

[Mohit Sinha]

Attachment: compressing whitespace , added test cases for 
 xD; x9; x20

Comment 15

[Mohit Sinha]

Attachment: compressing whitespace

Added test cases for 
 xD; x9; x20 and limited the length of comments per line

Comment 16

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

Comment on attachment 642224 [details] [diff] [review]
compressing whitespace

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

That looks good to me, thanks. Can you please give a better message than "bug 771904 :fix"? I thought the link I gave you explained that, but actually this is described: https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment. r=me with the commit message updated.

Then, you can use autoland to send your patch to the try server (I find this method somewhat slow, so be patient):
https://wiki.mozilla.org/ReleaseEngineering:Autoland

Comment 17

[Mohit Sinha]

Attachment: compressing whitespace

Comment 18

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

It seems that this autoland request from last Thursday failed. Any idea why?

Comment 19

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

(In reply to Frédéric Wang (:fredw) from comment #18)
> It seems that this autoland request from last Thursday failed. Any idea why?

I pushed the patch to try server, without using autoland:
https://tbpl.mozilla.org/?tree=Try&rev=d01de98565f3

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 643781 [details] [diff] [review]
compressing whitespace

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd32775559f

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/1fd32775559f

Comment 22

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 643781
	Branch: mozilla-central => try
Patch 643781 could not be applied to mozilla-central.
patching file layout/mathml/nsMathMLmtableFrame.cpp
Hunk #1 FAILED at 263
1 out of 1 hunks FAILED -- saving rejects to file layout/mathml/nsMathMLmtableFrame.cpp.rej
file layout/reftests/mathml/mtable-align-whitespace-ref.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/mathml/mtable-align-whitespace-ref.html.rej
file layout/reftests/mathml/mtable-align-whitespace.html already exists
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/mathml/mtable-align-whitespace.html.rej
patching file layout/reftests/mathml/reftest.list
Hunk #1 FAILED at 90
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/mathml/reftest.list.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Comment 23

[Mohit Sinha]

Attachment: removed rowline attributes in mtable-align-whitespace.html

Comment 24

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

Comment on attachment 650360 [details] [diff] [review]
removed rowline attributes in mtable-align-whitespace.html

Thanks Mohit, your patch has already been pushed to mozilla-central and this bug is fixed. You should rather create a new patch which modifies only the reftest pages and attach it to bug 781189.

Comment 25

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Firefox_17_for_developers#MathML
https://developer.mozilla.org/en-US/docs/MathML/Element/mtable#attr-align