Closed Bug 557474 (mpadded-voffset) Opened 11 years ago Closed 10 years ago

Not set
normal

RESOLVED FIXED
mozilla7

## Attachments

### (6 files, 8 obsolete files)

 757 bytes, text/html Details 709 bytes, text/html Details 3.28 KB, patch karlt : review+ Details | Diff | Splinter Review 6.03 KB, patch Details | Diff | Splinter Review 11.64 KB, patch karlt : review+ Details | Diff | Splinter Review 4.90 KB, patch karlt : review+ Details | Diff | Splinter Review
MathML3 adds a new attribute "voffset" to set the position of the child content. (it is the counterpart of lspace for vertical positioning).
Blocks: mathml-3
Summary: [MathML3] mspace: add support for the voffset attribute → [MathML3] mpadded: add support for the voffset attribute
Whiteboard: [good second bug]
Assignee: nobody → hage.jonathan
I just had a look to the code. I think the only file to change is layout/mathml/nsMathMLmpaddedFrame.cpp. I guess most of the places to modify can be found by searching "lspace" and adding the corresponding code for voffset.

The only thing I'm not sure is what is meant by the comment "should be *last* because lspace is overwritten!!" on lspace.

I suggest to break down the work into three steps (patches):

1) Add parsing for the voffset attribute.

2) Update positioning to take into account voffset. The code for this should only be located in nsMathMLmpaddedFrame::Place, I suppose.

3) Add reftests for mpadded. It should be possible to import the reftests I wrote for MathJax: https://github.com/fred-wang/MathJax-test/tree/master/MathMLToDisplay/Presentation/GeneralLayout/mpadded/
Attached patch Patch 1 (obsolete) — Splinter Review
The MathML REC says:

"Note that the lspace attribute does not affect the width of the mpadded element, and so the lspace attribute will also affect the space between the child content and following content, and may cause overprinting of the following content, unless the width is adjusted accordingly."

but currently, we are doing the following:

mBoundingMetrics.width = NS_MAX(0, lspace + width);

Similarly for voffset, the REC says:

"Note that the voffset attribute does not affect the height or depth of the mpadded element, and so the voffset attribute will also affect the space between the child content and neighboring content, and may cause overprinting of the neighboring content, unless the height or depth is adjusted accordingly."
Attached file testcase
Here is a testcase. You can compare the result with the one of MathJax by setting the useMathJax variable to true/false.

It seems that in Mozilla the depth does not use the color of mathbackground, so I've used menclose box, even if it's less precise.

Currently, lspace increases the width of the element, whereas the REC says it should not.
Attached patch Patch 1 version 2 (obsolete) — Splinter Review
Attachment #540474 - Attachment is obsolete: true
Attachment #540735 - Flags: review?(karlt)
Attached file testcase 2
> It seems that in Mozilla the depth does not use the color of mathbackground,
> so I've used menclose box, even if it's less precise.

I attach a second testcase which uses mathbackground. It seems that the computation of aDesiredSize.height only takes into account the ascent, not the descent (height and depth in the mpadded terminology). With the patch for the second part that Jonathan prepared today, it renders as I expect.
Attached patch Patch part 2 (obsolete) — Splinter Review
Attachment #540848 - Flags: review?(karlt)
Status: NEW → ASSIGNED
"The possible pseudo-units are the keywords height, depth, and width."

Is there any reason to have NS_MATHML_PSEUDO_UNIT_LSPACE and NS_MATHML_PSEUDO_UNIT_VOFFSET?
They would be 0 anyway, right?
Attachment #541054 - Flags: review?(karlt)
Attachment #540735 - Attachment is obsolete: true
Attachment #540848 - Attachment is obsolete: true
Attachment #540735 - Flags: review?(karlt)
Attachment #540848 - Flags: review?(karlt)
Attachment #541056 - Flags: review?(karlt)
Attachment #541055 - Flags: review?(karlt)
(In reply to comment #9)
> "The possible pseudo-units are the keywords height, depth, and width."
>
> Is there any reason to have NS_MATHML_PSEUDO_UNIT_LSPACE and
> NS_MATHML_PSEUDO_UNIT_VOFFSET?
> They would be 0 anyway, right?

I think you are right and so Jonathan removed the possibility to use them as pseudo units. Thanks for pointing this out.
Comment on attachment 541054 [details] [diff] [review]
Do not allow lspace as a pseudounit

I see that MathML2 had an lspace pseudo unit.

http://www.w3.org/TR/MathML2/chapter3.html#id.3.3.6.2
"The possible pseudo-units are the keywords width, lspace, height, and depth;"

But its definition made no sense:
"they each represent the length of the same-named dimension of the mpadded element's content (not of the mpadded element itself)"
but the lspace is more or less said to not be a property of the content:
"Unlike the other dimensions, lspace does not correspond to a real property of a bounding box, but exists only transiently during the computations done by each instance of mpadded."

MathML3 has apparently decided that there is no need to accept lspace for
backward compatibity, and I can't think of any good reason why anyone would
use lspace for a pseudo-unit, so I'm fine with this as is.

The result of anyone using lspace as a pseudo-unit would be that the
particular attribute is ignored.  That would be equivalent to the zero we
would use for relative lspace-unit attributes now.  It may differ from the
zero we would use for absolute lspace-unit attributes now if the default value
of the attribute is non-zero, but I can't think why anyone would rely on this
behavior.

>   // update lspace -- should be *last* because lspace is overwritten!!

Can you remove the " -- should be *last* because lspace is overwritten!!", please, because lspace is no longer used before this point and so it now doesn't matter that lspace gets overwritten.
Attachment #541054 - Flags: review?(karlt) → review+
Keywords: helpwanted
Attachment #541055 - Flags: review?(karlt) → review+
Attachment #541054 - Attachment is obsolete: true
Attachment #541385 - Attachment is obsolete: true
Attached patch Patch Reftests mpadded (obsolete) — Splinter Review
Attachment #541639 - Flags: review?(karlt)
Comment on attachment 541056 [details] [diff] [review]
add support for the voffset attribute part 2

>   if (mLeftSpaceSign != NS_MATHML_SIGN_INVALID ||
>       mWidthSign != NS_MATHML_SIGN_INVALID) { // there was padding on the right
>     // dismiss the right italic correction now (so that our parent won't correct us)
>-    mBoundingMetrics.width = NS_MAX(0, lspace + width);
>+    mBoundingMetrics.width = width;

Should remove "mLeftSpaceSign != NS_MATHML_SIGN_INVALID" from the condition, I
assume.

"the effective bounding box of an mpadded element always has non-negative
dimensions", so I assume NS_MAX(0, width) should be retained.

>-  nscoord dy = height - mBoundingMetrics.ascent;
>   nscoord dx = lspace;
>
>   mBoundingMetrics.ascent = height;
>   mBoundingMetrics.descent = depth;
>
>-  aDesiredSize.height += dy + depth - mBoundingMetrics.descent;
>+  aDesiredSize.height = mBoundingMetrics.ascent + mBoundingMetrics.descent;

It's a little difficult to say exactly what should happen here because there are two different kinds of units involved.

mBoundingMetrics.ascent/descent/leftBearing/rightBearing are all "ink"
metrics.  They describe a box enclosing the extents all glyphs drawn.  These
metrics are useful for working out what needs to be redrawn (including
scrolling).

aDesiredSize.ascent/height/width are "logical" (for want of a better word)
metrics.  They describe the space required for this frame and so generally
influence placement.

mBoundingMetrics.width should always be the same as aDesiredSize.width.

Consider a U+0020 space character: ink metrics are an empty rectangle, but
logical metrics would be the advance width needed to form the space and
the font's global typological ascent/descent.

The spec seems to almost try to avoid specifying the metrics to which
"bounding box" refers: 'Note that MathML doesn't define the precise
relationship between "ink", bounding boxes and positioning points, which are
implementation specific.'

However, this statement clearly indicates that width corresponds to the
logical/placement metric: "Setting the width to zero causes following content
to be positioned at the positioning point of the mpadded element."

It would seem sensible then that height and depth also refer to logical
metrics.  This is also indicated by the statement about the bounding box always being non-negative.  If depth were an ink-based metric it would be negative for characters that sit above the baseline, such as the minus sign.

However, this mpadded implementation has chosen to treat height and depth as if the modify the ink metrics.  Perhaps this is because, in some situations, ink
metrics affect vertical positioning.  In a table I would expect only logical
metrics to be relevant, but in a (non-bevelled) fraction, I assume ink metrics
are sometimes considered.  Another possible reason might be that our GDI font
implementation doesn't really return sensible vertical metrics for some fonts:
it returns the ascent/descent of the largest glyph in the font rather than the
typological design metrics.  (EmAscent is usually more useful that MaxAscent.)  Or perhaps the spec was less clear or even clearly different when this code was written.

Perhaps we could try to improve how height/depth are interpreted here, but I
suggest that, at this stage, only the coding error is fixed rather than
redesigning the approach.

The coding error is that mBoundingMetrics.descent is used to set
reordering the statements.
Attachment #541056 - Flags: review?(karlt) → review-
Comment on attachment 541639 [details] [diff] [review]

>+      div#bigsquare2 {
>+        left: 113px;

Why should this be at 113px?
OK, we may leave the logical bounding box computation as it is now and just fix the error. In that case, the mBoundingMetrics does not always correspond to the ink bounding box when mpadded is resized. So I suppose we don't need to take lspace/voffset into account to update mBoundingMetrics? And anyway maybe it would be problematic if the ink bounding box was outside the logical bounding box?
> "the effective bounding box of an mpadded element always has non-negative
> dimensions", so I assume NS_MAX(0, width) should be retained.

There is already something to verify that at the end of the UpdateValue function. But it seems that it does not prevent to set dimensions to negative value if they are already zero (for example if they are set to zero by an inner mpadded). What do you think about removing this code at the end of UpdateValue, and checking that each dimension are non-negative in Place, just after going out from the UpdateValue calls?
> Should remove "mLeftSpaceSign != NS_MATHML_SIGN_INVALID" from the condition,
> I assume.

I haven't looked the code in details, but I'm wondering why we check the sign correctness for the vertical dimensions but not for the horizontal ones?
(In reply to comment #22)

This is checking whether horizontal positioning attributes have been specified.
If so, it is changing the leftBearing/rightBearing to match the logical metrics.
This leads to inconsistencies IMO, but let's not try to correct that here.

> > "the effective bounding box of an mpadded element always has non-negative
> > dimensions", so I assume NS_MAX(0, width) should be retained.
>
> There is already something to verify that at the end of the UpdateValue
> function. But it seems that it does not prevent to set dimensions to
> negative value if they are already zero (for example if they are set to zero

Exactly.

> What do you think about removing this code at the end
> of UpdateValue, and checking that each dimension are non-negative in Place,
> just after going out from the UpdateValue calls?

Yes, sounds good.

> OK, we may leave the logical bounding box computation as it is now and just
> fix the error. In that case, the mBoundingMetrics does not always correspond
> to the ink bounding box when mpadded is resized. So I suppose we don't need
> to take lspace/voffset into account to update mBoundingMetrics? And anyway
> maybe it would be problematic if the ink bounding box was outside the
> logical bounding box?

Not sure I understand all you are asking here but:

nsMathMLContainerFrame::GatherAndStoreOverflow takes care of child ink bounds,
so there is flexibility in the setting of mBoundingMetrics here.

Yes, mBoundingMetrics for the mpadded does not in general correspond to child ink bounds; the code is setting it to affect spacing wrt adjacent elements in cases where the ink metrics would be considered in positioning.
lspace/voffset affect only child positions and so do not change aDesiredSize or mBoundingMetrics.

The ink bounding box can be outside the logical bounding box.  This can happen with italic fonts with curly tails for example.  Sometimes this can lead to glyphs overlapping, but often it leads to better positioning if the logical bounds are not increased to match the ink bounds.
Attachment #541639 - Attachment is obsolete: true
Attachment #541639 - Flags: review?(karlt)
Attachment #541056 - Attachment is obsolete: true
Attachment #542460 - Flags: review?(karlt)
Comment on attachment 542460 [details] [diff] [review]
add support for the voffset attribute (part 2) version 2

When I built the program without this patch in debug mode, I remarked several assertions which are not present when the patch is applied.
Comment on attachment 542456 [details] [diff] [review]

see the crashtest based on the comment 21
Attachment #542456 - Flags: review?(karlt)
Comment on attachment 542460 [details] [diff] [review]
add support for the voffset attribute (part 2) version 2

>+  nscoord old_value = mBoundingMetrics.descent;
>   mBoundingMetrics.ascent = height;
>   mBoundingMetrics.descent = depth;
>
>-  aDesiredSize.height += dy + depth - mBoundingMetrics.descent;
>+  aDesiredSize.height += dy + depth - old_value;

"old_value" is not a very descriptive name, given all the values that a being tracked.
A better fix is to move the mBoundingMetrics.ascent/descent assignments to between

The rest looks good thanks.
Attachment #542460 - Flags: review?(karlt) → review-
Attachment #542456 - Flags: review?(karlt) → review+
Attachment #542460 - Attachment is obsolete: true
Attachment #542734 - Flags: review?(karlt)
Attachment #542734 - Flags: review?(karlt) → review+
Patches should be pushed in that order:

attachment 541431 [details] [diff] [review]
attachment 541055 [details] [diff] [review]
attachment 542734 [details] [diff] [review]
attachment 542456 [details] [diff] [review]
http://hg.mozilla.org/mozilla-central/rev/50e94b9988e9
http://hg.mozilla.org/mozilla-central/rev/fe3a45dfe43c
http://hg.mozilla.org/mozilla-central/rev/b28932f43641
http://hg.mozilla.org/mozilla-central/rev/2f509e44172b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(In reply to comment #31)
> http://hg.mozilla.org/mozilla-central/rev/50e94b9988e9
> http://hg.mozilla.org/mozilla-central/rev/fe3a45dfe43c
> http://hg.mozilla.org/mozilla-central/rev/b28932f43641
> http://hg.mozilla.org/mozilla-central/rev/2f509e44172b

The reftest patch had a "\ No newline at end of file" so mixed up with the mfrac-linethickness-1 line.
I think doc updates are done.
Feel free to add, correct or review contents in the wiki:
https://developer.mozilla.org/en/Firefox_7_for_developers#MathML
(In reply to comment #32)
http://hg.mozilla.org/mozilla-central/diff/2f509e44172b/layout/reftests/mathml/reftest.list
Fixed in http://hg.mozilla.org/mozilla-central/rev/f354ee7dfcec, thanks Dão.
Added to DOM fuzzer.