557474 - (mpadded-voffset) [MathML3] mpadded: add support for the voffset attribute

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

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

MathML3 adds a new attribute "voffset" to set the position of the child content. (it is the counterpart of lspace for vertical positioning).

Comment 1

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

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/

Comment 2

[Jonathan Hage]

Attachment: Patch 1

Comment 3

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

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."

Comment 4

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

Attachment: 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.

Comment 5

[Jonathan Hage]

Attachment: Patch 1 version 2

Comment 6

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

Attachment: testcase 2

Comment 7

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

> 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.

Comment 8

[Jonathan Hage]

Attachment: Patch part 2

Comment 9

[Karl Tomlinson (:karlt)]

"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?

Comment 10

[Jonathan Hage]

Attachment: Do not allow lspace as a pseudounit

Comment 11

[Jonathan Hage]

Attachment: add support for the voffset attribute part 1

Comment 12

[Jonathan Hage]

Attachment: add support for the voffset attribute part 2

Comment 13

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

(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 14

[Karl Tomlinson (:karlt)]

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.

Comment 15

[Jonathan Hage]

Attachment: Do not allow lspace as a pseudounit version 2

Comment 16

[Jonathan Hage]

Attachment: Do not allow lspace as a pseudounit version 3

Comment 17

[Jonathan Hage]

Attachment: Patch Reftests mpadded

Comment 18

[Karl Tomlinson (: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.ascent += dy;
>+  aDesiredSize.ascent = mBoundingMetrics.ascent;
>   aDesiredSize.width = mBoundingMetrics.width;
>-  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
aDesiredSize.height after it has been overwritten.  This can be addressed by
reordering the statements.

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 541639 [details] [diff] [review]
Patch Reftests mpadded

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

Why should this be at 113px?

Comment 20

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

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?

Comment 21

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

> "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?

Comment 22

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

> 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?

Comment 23

[Karl Tomlinson (:karlt)]

(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.

(In reply to comment #21)
> > "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).

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.

(In reply to comment #20)
> 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.

Comment 24

[Jonathan Hage]

Attachment: Patch Reftests mpadded version 2

Comment 25

[Jonathan Hage]

Attachment: add support for the voffset attribute (part 2) version 2

Comment 26

[Jonathan Hage]

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 27

[Jonathan Hage]

Comment on attachment 542456 [details] [diff] [review]
Patch Reftests mpadded version 2

see the crashtest based on the comment 21

Comment 28

[Karl Tomlinson (: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.ascent += dy;
>   aDesiredSize.width = mBoundingMetrics.width;
>-  aDesiredSize.height += dy + depth - mBoundingMetrics.descent;
>+  aDesiredSize.height += dy + depth - old_value;
>   aDesiredSize.mBoundingMetrics = mBoundingMetrics;

"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 aDesiredSize.height and aDesiredSize.mBoundingMetrics assignments.

The rest looks good thanks.

Comment 29

[Jonathan Hage]

Attachment: add support for the voffset attribute (part 2) version 3

Comment 30

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

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]

Comment 31

[Dão Gottwald [::dao]]

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

Comment 32

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

(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.

Comment 33

[Florian Scholz (Open Web Docs)]

I think doc updates are done.
Feel free to add, correct or review contents in the wiki:
https://developer.mozilla.org/en/MathML/Element/mpadded
https://developer.mozilla.org/en/Firefox_7_for_developers#MathML

Comment 34

[Karl Tomlinson (:karlt)]

(In reply to comment #32)
> The reftest patch had a "\ No newline at end of file" so mixed up with the
> mfrac-linethickness-1 line.

i.e.
http://hg.mozilla.org/mozilla-central/diff/2f509e44172b/layout/reftests/mathml/reftest.list

Comment 35

[Karl Tomlinson (:karlt)]

Fixed in http://hg.mozilla.org/mozilla-central/rev/f354ee7dfcec, thanks Dão.

Comment 36

[Jesse Ruderman]

Added to DOM fuzzer.