537916 - MathML mfenced: Whitespaces in separators are not ignored

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

Attachment: Testcase

The MathML REC indicates:

"The value of separators is a sequence of zero or more separator characters (or entity references), optionally separated by whitespace."

In the testcase, the separators should be a comma, semi-comma and a period. Currently, the spaces are treated as separator.

See http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmfencedFrame.cpp#170

Comment 1

[Saint Wesonga]

Attachment: Patch

Comment 2

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

(In reply to comment #1)
> Created an attachment (id=420873) [details]
> Patch

Thanks.

The MathML REC says "In MathML, as in XML, "whitespace" means simple spaces, tabs, newlines, or carriage returns, i.e., characters with hexadecimal Unicode codes U+0020, U+0009, U+000A, or U+000D, respectively." but I think your code only removes spaces. I think you can simply use nsAutoString::StripWhitespace

BTW, I'm not even sure that what is made for opening/closing fences (value.Trim(" ")) is correct. The REC says "The "opening-fence" and "closing-fence" are arbitrary strings. (Since they are used as the content of mo elements, any whitespace they contain will be trimmed and collapsed as described in Section 2.4.6 Collapsing Whitespace in Input.)"

Comment 3

[Saint Wesonga]

Attachment: Patch

Definitely didn't look at all the nsAutoString methods...

Comment 4

[Saint Wesonga]

(In reply to comment #2)
> BTW, I'm not even sure that what is made for opening/closing fences
> (value.Trim(" ")) is correct. The REC says "The "opening-fence" and
> "closing-fence" are arbitrary strings. (Since they are used as the content of
> mo elements, any whitespace they contain will be trimmed and collapsed as
> described in Section 2.4.6 Collapsing Whitespace in Input.)"

Shouldn't that be Section 2.3.5? So, based on this interpretation we need to ensure opening/closing fence whitespaces are "collapsed" internally in addition to the value.Trim(" ") that we are doing, right?

Comment 5

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

> Shouldn't that be Section 2.3.5?

2.4.6 is this section:
http://www.w3.org/TR/MathML/chapter2.html#fund.collapse

>So, based on this interpretation we need to
> ensure opening/closing fence whitespaces are "collapsed" internally in addition
> to the value.Trim(" ") that we are doing, right?

Yes, but again I think value.Trim(" ") only eliminates spaces and not other whitespaces (tab, newlines etc).
I don't know all the string functions, but nsAutoString::CompressWhitespace seems to do the job:

https://developer.mozilla.org/En/NsAutoString#CompressWhitespace

Comment 6

[Saint Wesonga]

(In reply to comment #5)
> > Shouldn't that be Section 2.3.5?
> 
> 2.4.6 is this section:
> http://www.w3.org/TR/MathML/chapter2.html#fund.collapse
> 
I
Seems we should file this as a separate bug, no?

Comment 7

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

(In reply to comment #6)
> (In reply to comment #5)
> > > Shouldn't that be Section 2.3.5?
> > 
> > 2.4.6 is this section:
> > http://www.w3.org/TR/MathML/chapter2.html#fund.collapse
> > 
> I
> Seems we should file this as a separate bug, no?

Yes, probably.

Comment 8

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

Comment on attachment 420907 [details] [diff] [review]
Patch

Thanks!! How about a test?

Comment 9

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

(In reply to comment #8)
> (From update of attachment 420907 [details] [diff] [review])
> Thanks!! How about a test?

Attachment 420067 [details] is currently displayed "(a, b  ; c  d)" without patch applied.
The correct rendering should be "(a, b; c. d)".

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 420907 [details] [diff] [review] [details])
> > Thanks!! How about a test?
> 
> Attachment 420067 [details] is currently displayed "(a, b  ; c  d)" without patch
> applied.
> The correct rendering should be "(a, b; c. d)".

Saint, are you working on making a reftest or can I make one from attachment 420067 [details]?

Comment 11

[Saint Wesonga]

Attachment: RefTest

Comment 12

[Saint Wesonga]

(In reply to comment #10)
> Saint, are you working on making a reftest or can I make one from attachment
> 420067?

Sorry for the lag here...

Comment 13

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

(In reply to comment #11)
> Created an attachment (id=424443) [details]
> RefTest

Thanks. In addition to checking that whitespaces are collapsed, you are testing the equivalence of <mo>'s and separators. I guess the only thing you need for this bug is to compare two <mfenced/>'s with different values for "separators". However, your idea to check the equivalence is good, so maybe we could create many mfence-*.xhtml tests, including one for bug 538965. I will probably open another bug to list some ideas of reftests for mfence, but for the present bug, I suppose the following is enough:

- compare separators="WS;WS" with ";"
- compare separators=",;." and separator=",WS;WS." 

where WS are whitespaces that include other characters than spaces (tab, linefeed etc)

Also, I don't believe you need all the header and <p>'s generated by Amaya. Just a minimal tree html->body->math as the other refests do.

Comment 14

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

Comment on attachment 424443 [details] [diff] [review]
RefTest

per comment #13

Comment 15

[Saint Wesonga]

Attachment: RefTest

What is the best way to handle inserting newlines in content that's added as a patch? I'm getting this odd interaction between the content and the patch format.

Comment 16

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

(In reply to comment #15)
> Created an attachment (id=428107) [details]
> RefTest

Maybe you could add a comment before the two <mfenced/>'s of bug537916.xhtml to indicate that each blank is a whitespace sequence "U+0020 U+0009 U+000a U+000d" and must not be modified.

Also, you should update the <title/>'s to say that this reftest checks whether the whitespaces are ignored in the "separators" attribute of <mfenced/>.

Otherwise it looks good, thanks.

Comment 17

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

Also, I think it would be fine if you could rename bug537916*.xhtml to mfenced-1*.xhtml to be consistent with the other reftests.

Comment 18

[Saint Wesonga]

Attachment: RefTest

How about this?

Comment 19

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

Comment on attachment 433853 [details] [diff] [review]
RefTest

Looks good to me. Thanks.

Comment 20

[Saint Wesonga]

I'm curious..., what tool (if any) would anyone recommend for editing whitespaces changes in text files such as the attached test cases? Can this be efficiently done without a hex editor?

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e9b7e0b5821d

Comment 22

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

(In reply to comment #20)
> I'm curious..., what tool (if any) would anyone recommend for editing
> whitespaces changes in text files such as the attached test cases? Can this be
> efficiently done without a hex editor?

I don't really know... the "classical" editors emacs/vim have an hexadecimal mode and other tools probably also have one.