Closed
Bug 398038
Opened 17 years ago
Closed 12 years ago
<mpadded height="20 10"> triggers "###!!! ASSERTION: Unexpected Pseudo Unit"
Categories
(Core :: MathML, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: jruderman, Assigned: fredw)
References
Details
(Whiteboard: [fixed by bug 677036])
Attachments
(2 files, 3 obsolete files)
146 bytes,
application/xhtml+xml
|
Details | |
980 bytes,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
Loading the testcase triggers:
###!!! ASSERTION: Unexpected Pseudo Unit: '0', file /Users/jruderman/trunk/mozilla/layout/mathml/base/src/nsMathMLmpaddedFrame.cpp, line 331
Comment 1•17 years ago
|
||
Take only the first of two distinct values.
Attachment #282913 -
Flags: superreview?(rbs)
Attachment #282913 -
Flags: review?(rbs)
I will rather prefer that we reject/ignore such an ill-formed attribute (otherwise we are feeding into the kind of legacy/problems that tag-soup HTML did).
Attachment #282913 -
Flags: superreview?(rbs)
Attachment #282913 -
Flags: superreview-
Attachment #282913 -
Flags: review?(rbs)
Attachment #282913 -
Flags: review-
Comment 3•17 years ago
|
||
Rejects such attributes.
Attachment #282913 -
Attachment is obsolete: true
Attachment #282926 -
Flags: superreview?(rbs)
Attachment #282926 -
Flags: review?(rbs)
Comment on attachment 282926 [details] [diff] [review]
v2
What about just strengthening the if-test with this one-liner:
// see if the input was just a CSS value
number.Append(unit); // leave the sign out if it was there
- if (ParseNumericValue(number, aCSSValue))
+ if (ParseNumericValue(number, aCSSValue) &&
+ aCSSValue.IsLengthUnit())
return PR_TRUE;
Would that do the trick?
Comment 5•17 years ago
|
||
Comment on attachment 282926 [details] [diff] [review]
v2
Yup, I was not sure about shared state modification, but it seems ok as far as I can see.. But still <math:mpadded height="10 0000000000em"/> is kind of peculiar, but if it is a bug, it is a different one, I guess.
Attachment #282926 -
Attachment is obsolete: true
Attachment #282926 -
Flags: superreview?(rbs)
Attachment #282926 -
Flags: review?(rbs)
Let's try to also fix the peculiar thing while we are it. It probably comes from the fact that height="10 em" (that is a space before the unit) is supported (by ParseNumericValue), which is desired. We are better off strengthening it so that it doesn't support some unintended leftover digits before the unit. Speaking of this I suspect that the peculiar behavior may also be happening with fixed-point numbers too, height="10 1.2em"...
On further thought, the following could do the trick because we only have to bother checking |unit| since it is the sole culprit. Just make sure that it doesn't regress bug 148326.
+ // catch any ill-formed unit that disguises as a numeric value - bug 398038
+ if (nsCRT::IsAsciiDigit(unit[0]) || unit[0] == '.') {
+ aSign = NS_MATHML_SIGN_INVALID;
+ return PR_FALSE;
+ }
+
// see if the input was just a CSS value
number.Append(unit); // leave the sign out if it was there
if (ParseNumericValue(number, aCSSValue))
return PR_TRUE;
Comment 8•17 years ago
|
||
Comment 7 is enough because |gotPercent| will catch the '%' case and the unit will start right after it:
====================
// skip any space after the number
if (i < stringLength && nsCRT::IsAsciiSpace(aString[i]))
i++;
// see if this is a percentage-based value
if (i < stringLength && aString[i] == '%') {
i++;
gotPercent = PR_TRUE;
// skip any space after the '%' sign
if (i < stringLength && nsCRT::IsAsciiSpace(aString[i]))
i++;
}
// the remainder now should be a css-unit, or a pseudo-unit, or a named-space
aString.Right(unit, stringLength - i);
==================
Comment 10•17 years ago
|
||
Attached as a patch.
Updated•15 years ago
|
QA Contact: ian → mathml
Assignee | ||
Updated•15 years ago
|
Assignee: rbs → nobody
Assignee | ||
Updated•13 years ago
|
Keywords: helpwanted
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → hage.jonathan
Status: NEW → ASSIGNED
Comment 11•13 years ago
|
||
The patch proposed still allows whitespaces after the sign
Comment 12•13 years ago
|
||
nsMathMLmpaddedFrame::ParseAttribute allows whitespace between the sign and the number and between the number and the unit. nsMathMLFrame::ParseNumericValue allows whitespaces only between the sign and the number. So the two behaviors are inconsistent.
For example, in the testcase the bug comes from the fact that nsMathMLmpaddedFrame::ParseAttribute concatenates the two numbers, then it calls nsMathMLFrame::ParseNumericValue, consequently nsMathMLFrame::ParseNumericValue accepts the attribute's value.
In my opinion the best solution is the way of nsMathMLFrame::ParseNumericValue (i.e. allowing spaces only at the beginning and end of the attribute value). Note that the MathML rec says: "spaces are not allowed within these values, but implementers may wish to ignore such spaces to maximize backward compatibility."
Maybe we should also unify the code of the two functions. Therefore this bug will depend on Bug 411227.
Perhaps we should add a flag allow_pseudo_unit to use in nsMathMLMmpadded and implement the parsing of pseudo unit directly in nsMathMLElement::ParseNumericValue.
Comment 13•13 years ago
|
||
Unless there are very strong compat reasons here and a well-defined processing model for the spaces, I propose just not allowing the spaces...
Assignee | ||
Comment 14•13 years ago
|
||
The MathML REC says:
"Note that the examples in the Version 2 of the MathML specification showed spaces within the attribute values, suggesting that this was the intended format. Formally, spaces are not allowed within these values, but implementers may wish to ignore such spaces to maximize backward compatibility. "
Except in these former examples, I don't know if this syntax is really used. Why do we allow spaces in nsMathMLElement::ParseNumericValue? Is it consistent with what CSS does?
Comment 15•13 years ago
|
||
CSS does not allow spaces between the number and the unit (or between the sign and the number, for that matter).
Assignee | ||
Comment 16•13 years ago
|
||
(In reply to comment #15)
> CSS does not allow spaces between the number and the unit (or between the
> sign and the number, for that matter).
And what about the spaces at the beginning and the end of the attribute value?
Comment 17•13 years ago
|
||
CSS allows whitespace between any two tokens, typically, so something like:
height: 5px ;
is perfectly fine.
Assignee | ||
Comment 18•13 years ago
|
||
(In reply to comment #17)
> CSS allows whitespace between any two tokens, typically, so something like:
>
> height: 5px ;
>
> is perfectly fine.
OK, I meant when the values are used in attributes, such that height=" 5px "?
Comment 19•13 years ago
|
||
If you mean in HTML, then HTML attributes that set lengths don't take units; they're always pixels. So height=" 5px " is the same as height=" 5 happy unicorns", and both are the same as height="5".
If you're still talking about CSS, then I'm not sure what you're asking...
Assignee | ||
Comment 20•13 years ago
|
||
(In reply to comment #19)
> If you mean in HTML, then HTML attributes that set lengths don't take units;
> they're always pixels. So height=" 5px " is the same as height=" 5 happy
> unicorns", and both are the same as height="5".
>
> If you're still talking about CSS, then I'm not sure what you're asking...
Sorry, I'm mixing up with all the syntaxes. But for example, I think SVG shares styling attributes with CSS, and some SVG elements can have height="5px".
Comment 21•13 years ago
|
||
We parse those SVG attributes with the CSS parser. So they allow leading and trailing whitespace.
Assignee | ||
Comment 22•13 years ago
|
||
(In reply to comment #21)
> We parse those SVG attributes with the CSS parser. So they allow leading
> and trailing whitespace.
OK, so Jonathan's proposal in comment 12 (allowing only leading/trainling whitespaces) seems the best option to me.
The XML spec says:
"If the attribute type is not CDATA, then the XML processor MUST further process the normalized attribute value by discarding any leading and trailing space (#x20) characters, and by replacing sequences of space (#x20) characters by a single space (#x20) character."
"All attributes for which no declaration has been read SHOULD be treated by a non-validating processor as if declared CDATA."
The (non normative) MathML DTD does declare attributes as NMTOKENS, so they are CDATA by default.
The MathML REC says that "There should be no space between the number and the unit of a length." for attribute length. For leading/trailing whitespace, this seems less strict: "Moreover, leading and trailing whitespace in attribute values should be avoided. ".
The backward compatibility comment on mpadded attribute, seems to be there only because the authors made a mistake in the spec examples.
Assignee | ||
Comment 23•13 years ago
|
||
We can unify the code and move the parsing of nsMathMLFrame::ParseNamedSpaceValue and nsMathMLmpaddedFrame::ParseAttribute into nsMathMLElement::ParseNumericValue.
For nsMathMLFrame::ParseNamedSpaceValue, I've opened bug 673759.
I think we need to modify the aFlags parameter of nsMathMLElement::ParseNumericValue to become a reference (or add more arguments to this function that will be references). Then we can get [out] parameters about the sign and pseudo unit, that are used in mpadded. We should also add a [in] parameter ALLOW_MPADDED_SYNTAX to indicate that we can use the extended syntax instead of simply a standard CSS value.
In nsMathMLmpaddedFrame::ParseAttribute, there is a XXX syntax asking why we don't look for an attribute on an mstyle ancestor when calling nsMathMLFrame::ParseNamedSpaceValue. We don't need to do that, since the right attribute value is assumed to have been passed by the caller nsMathMLmpaddedFrame::ProcessAttributes. This XXX comment will naturally be removed when we unify the code of the three functions. Actually, it is specified in nsMathMLmpaddedFrame::ProcessAttributes that we don't access mstyle. The REC comment can be updated now we support voffset:
"MathML specifies that when the attributes height, depth or width are specified on an mstyle element, they apply only to mspace elements, and not to the corresponding attributes of mglyph, mpadded, or mtable. [...] When the lspace attribute is set with mstyle, it applies only to the mo element and not to mpadded. To be consistent, the voffset attribute of the mpadded element can not be set on mstyle."
Assignee | ||
Updated•13 years ago
|
Assignee: hage.jonathan → fred.wang
Assignee | ||
Comment 24•13 years ago
|
||
Here is a crashtest.
This bug should be fixed by patch of bug 677036.
Attachment #283118 -
Attachment is obsolete: true
Attachment #587416 -
Flags: review?(karlt)
Updated•13 years ago
|
Attachment #587416 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 25•12 years ago
|
||
Keywords: helpwanted → checkin-needed
Whiteboard: [please push with the patch from bug 677036]
Comment 26•12 years ago
|
||
Flags: in-testsuite+
Keywords: assertion,
checkin-needed,
testcase
Whiteboard: [please push with the patch from bug 677036] → [fixed by bug 677036]
Target Milestone: --- → mozilla15
Comment 27•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•