Closed
Bug 557474
(mpadded-voffset)
Opened 14 years ago
Closed 12 years ago
[MathML3] mpadded: add support for the voffset attribute
Categories
(Core :: MathML, defect)
Core
MathML
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: fredw, Assigned: hage.jonathan)
References
(Blocks 2 open bugs, )
Details
(Keywords: dev-doc-complete, Whiteboard: [good second bug])
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).
Reporter | ||
Updated•14 years ago
|
Summary: [MathML3] mspace: add support for the voffset attribute → [MathML3] mpadded: add support for the voffset attribute
Reporter | ||
Updated•13 years ago
|
Whiteboard: [good second bug]
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → hage.jonathan
Reporter | ||
Comment 1•13 years ago
|
||
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/
Assignee | ||
Comment 2•13 years ago
|
||
Reporter | ||
Comment 3•13 years ago
|
||
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."
Reporter | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #540474 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #540735 -
Flags: review?(karlt)
Reporter | ||
Comment 6•13 years ago
|
||
Reporter | ||
Comment 7•13 years ago
|
||
> 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.
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #540848 -
Flags: review?(karlt)
Reporter | ||
Updated•13 years ago
|
Status: NEW → ASSIGNED
Comment 9•13 years ago
|
||
"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?
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #541054 -
Flags: review?(karlt)
Assignee | ||
Comment 11•13 years ago
|
||
Attachment #540735 -
Attachment is obsolete: true
Attachment #540848 -
Attachment is obsolete: true
Attachment #540735 -
Flags: review?(karlt)
Attachment #540848 -
Flags: review?(karlt)
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #541056 -
Flags: review?(karlt)
Assignee | ||
Updated•13 years ago
|
Attachment #541055 -
Flags: review?(karlt)
Reporter | ||
Comment 13•13 years ago
|
||
(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•13 years ago
|
||
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+
Updated•13 years ago
|
Keywords: helpwanted
Updated•13 years ago
|
Attachment #541055 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #541054 -
Attachment is obsolete: true
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #541385 -
Attachment is obsolete: true
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #541639 -
Flags: review?(karlt)
Comment 18•12 years ago
|
||
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.
Attachment #541056 -
Flags: review?(karlt) → review-
Comment 19•12 years ago
|
||
Comment on attachment 541639 [details] [diff] [review] Patch Reftests mpadded >+ div#bigsquare2 { >+ left: 113px; Why should this be at 113px?
Reporter | ||
Comment 20•12 years ago
|
||
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?
Reporter | ||
Comment 21•12 years ago
|
||
> "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?
Reporter | ||
Comment 22•12 years ago
|
||
> 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•12 years ago
|
||
(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.
Assignee | ||
Comment 24•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #541639 -
Attachment is obsolete: true
Attachment #541639 -
Flags: review?(karlt)
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #541056 -
Attachment is obsolete: true
Attachment #542460 -
Flags: review?(karlt)
Assignee | ||
Comment 26•12 years ago
|
||
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.
Assignee | ||
Comment 27•12 years ago
|
||
Comment on attachment 542456 [details] [diff] [review] Patch Reftests mpadded version 2 see the crashtest based on the comment 21
Assignee | ||
Updated•12 years ago
|
Attachment #542456 -
Flags: review?(karlt)
Comment 28•12 years ago
|
||
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.
Attachment #542460 -
Flags: review?(karlt) → review-
Updated•12 years ago
|
Attachment #542456 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 29•12 years ago
|
||
Attachment #542460 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #542734 -
Flags: review?(karlt)
Updated•12 years ago
|
Attachment #542734 -
Flags: review?(karlt) → review+
Reporter | ||
Comment 30•12 years ago
|
||
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]
Keywords: checkin-needed,
dev-doc-needed
Comment 31•12 years ago
|
||
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: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Reporter | ||
Comment 32•12 years ago
|
||
(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•12 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
Comment 34•12 years ago
|
||
(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•12 years ago
|
||
Fixed in http://hg.mozilla.org/mozilla-central/rev/f354ee7dfcec, thanks Dão.
Comment 36•12 years ago
|
||
Added to DOM fuzzer.
Reporter | ||
Updated•12 years ago
|
Blocks: mathml-in-mathjax
You need to log in
before you can comment on or make changes to this bug.
Description
•