Attributes not correctly interpreted in <mpadded>

RESOLVED FIXED in mozilla1.1beta



16 years ago
16 years ago


(Reporter: rbs, Assigned: rbs)



Firefox Tracking Flags

(Not tracked)



(3 attachments, 4 obsolete attachments)



16 years ago
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:

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

16 years ago
Created attachment 85729 [details]

Comment 2

16 years ago
Created attachment 86168 [details] [diff] [review]
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)

  * "+/-", 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
    (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

16 years ago
Created attachment 86170 [details]
testcase with annotations (need a build with the patch to make sense)
Attachment #85729 - Attachment is obsolete: true

Comment 4

16 years ago
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

16 years ago
Created attachment 86483 [details] [diff] [review]
patch in sync with the tip of the trunk


16 years ago
Attachment #86168 - Attachment is obsolete: true

Comment 6

16 years ago
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

16 years ago
Created attachment 86521 [details] [diff] [review]
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.
Attachment #86483 - Attachment is obsolete: true

Comment 8

16 years ago
Created attachment 86522 [details]
screenshot - rendering of the testcase

Comment 9

16 years ago
-> targeting m1.1b
Target Milestone: --- → mozilla1.1beta

Comment 10

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

Attachment #86521 - Flags: superreview+
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

16 years ago
Created attachment 87639 [details] [diff] [review]
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.
Attachment #86521 - Attachment is obsolete: true

Comment 14

16 years ago
Fix checked in.
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.