148326 - Attributes not correctly interpreted in <mpadded>

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[rbs]

There are a number of bugs that I have observed in <mpadded>

1. Attributes doesn't work at all. This is a regression. I saw it a long while
   ago, but was mystified as to its cause. It is only recently that I had a look
   in bonsai to see where it came from. If you look closely at this diff:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsMathMLmpaddedFrame.cpp&root=/cvsroot&subdir=mozilla/layout/mathml/base/src&command=DIFF_FRAMESET&rev1=1.9&rev2=1.10

It appears that the following line is removed:

  aString.Right(value, stringLength - i);                                       

This was an inadvertent removal when trying to re-indent the line tha followed.

Other bugs that have always been there, and which I hope to fix in this round:

2. There is an error in the code that parses attributes -- whitespace before the
   unit (e.g., "1 em") leads to troubles.

3. When attribute-values cause children to stick outiside (e.g., width="0"), the
   frame and thus its children aren't painted.

Proposed fixes:
===============

For 1. the fix is simply to re-instate the deleted line.

For 3. the fix is to make sure to set the NS_FRAME_OUTSIDE_CHILDREN bit when
children stick outside.

For 2. well, the fix is to get the parsing right...

Comment 1

[rbs]

Attachment: testcase

Comment 2

[rbs]

Attachment: patch to fix the bug

The patch is essentially a rewrite of the parsing code. In its premature
optimization, the previous code was overzealous and had some troubles. This one
takes a more secure and comprehensive route. Basically, the problem is to parse
an atribute with a formal value as follows:

[+|-] unsigned-number (% [pseudo-unit] | pseudo-unit | css-unit | namedspace)

where
  * "+/-", if there, means the value is an incremental value
  * "unsigned-number" can be a fixed-point number (e.g., 1.25)
  * "pseudo-unit" is something related to the current frame, e.g., one
    can do width="1 height" to force the frame to end-up as a square frame
    or witdh="200=% height" to force the frame to be twice as wide as its
height
    (it is because of this that children can actually stick outside)
  * "css-unit" is one of the usual 'ex', 'em', etc...  (there is an existing
    function to handle them)
  * "namedspace" is a string such as 'thinspace', 'thickspace', etc., for which

    the actual numeric value depends on the current style (there is an existing
     function to handle them)
  * ...and there can be space between the tokens
  * ...and errors must be handled gracefully (i.e., proceed as if the attribute

    wasn't there)

The patch does:
 1. simply compress the input (hence there is at most one space between tokens)

 2. tokenize while taking care of edge cases and other technicalities.

I have tested the patch thoroughly and it now appears to address the reported
problems fully.

Comment 3

[rbs]

Attachment: testcase with annotations (need a build with the patch to make sense)

Comment 4

[rbs]

Comment on attachment 86168 [details] [diff] [review]
patch to fix the bug

roc/waterson, r/sr?

(there are particular edge cases such as height="200%" to magnify the current
height by 200%).

Comment 5

[rbs]

Attachment: patch in sync with the tip of the trunk

Comment 6

[rbs]

Re-reading comment #4, I had the impression that one could read it that the edge 
case being mentioned was a remaining edge case that wasn't handled. In fact, I 
was merely drawing attention to an example of edge case. The code is handling 
all the edge cases to my knowledge.

Comment 7

[rbs]

Attachment: patch - take3

Rather than using "if (oldValue != newValue)" to detect if an attribute is
there, use a more encompassing test the "if (sign in [+- ])" because the sole
presence of an attribute should disable some other generous inter-frame spacing
that the MathML egine does by default, even if an oldValue coincides with a
newValue. Also added more explantatory comments.

Comment 8

[rbs]

Attachment: screenshot - rendering of the testcase

Comment 9

[rbs]

-> targeting m1.1b

Comment 10

[Chris Waterson]

Comment on attachment 86521 [details] [diff] [review]
patch - take3

sr=waterson

Comment 11

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

I think you need to se the FRAME_OUTSIDE_CHILDREN bit if (dx < 0 || dy < 0 ||
mOverflowArea.XMost() > aDesiredSize.XMost() || mOverflowArea.YMost() >
aDesiredSize.YMost()) ... know what I mean?

Comment 12

[rbs]

Attachment: patch - take 4

yep, I see what you mean. It had crossed my mind but I didn't gave it the due
time & attention that it deserved. Attached is a patch which needed a bit a
quietness to get the details right with the less amount of the code.

Comment 13

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

Comment on attachment 87639 [details] [diff] [review]
patch - take 4

r=roc+moz

Comment 14

[rbs]

Fix checked in.