Status: RESOLVED FIXED [good second bug] dev-doc-complete Core Components MathML (show other bugs) Trunk All All -- normal (vote) mozilla7 Jonathan Hage Anthony Jones (:kentuckyfriedtakahe, :k17e) 672444 mathml-3 mathml-in-mathjax Show dependency tree / graph

Reported: 2010-04-06 03:16 PDT by Frédéric Wang (:fredw)
Modified: 2011-09-20 04:01 PDT (History)
4 users (show)
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Attachments
Patch 1 (7.32 KB, patch)
2011-06-20 08:17 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
testcase (757 bytes, text/html)
2011-06-21 04:56 PDT, Frédéric Wang (:fredw)
no flags Details
Patch 1 version 2 (4.92 KB, patch)
2011-06-21 07:00 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
testcase 2 (709 bytes, text/html)
2011-06-21 09:38 PDT, Frédéric Wang (:fredw)
no flags Details
Patch part 2 (6.36 KB, patch)
2011-06-21 13:00 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Do not allow lspace as a pseudounit (6.01 KB, patch)
2011-06-22 08:16 PDT, Jonathan Hage
karlt: review+
Details | Diff | Splinter Review
add support for the voffset attribute part 1 (3.28 KB, patch)
2011-06-22 08:17 PDT, Jonathan Hage
karlt: review+
Details | Diff | Splinter Review
add support for the voffset attribute part 2 (3.36 KB, patch)
2011-06-22 08:18 PDT, Jonathan Hage
karlt: review-
Details | Diff | Splinter Review
Do not allow lspace as a pseudounit version 2 (6.01 KB, patch)
2011-06-23 08:15 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Do not allow lspace as a pseudounit version 3 (6.03 KB, patch)
2011-06-23 10:54 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Patch Reftests mpadded (8.50 KB, patch)
2011-06-24 02:33 PDT, Jonathan Hage
no flags Details | Diff | Splinter Review
Patch Reftests mpadded version 2 (11.64 KB, patch)
2011-06-28 07:10 PDT, Jonathan Hage
karlt: review+
Details | Diff | Splinter Review
add support for the voffset attribute (part 2) version 2 (4.93 KB, patch)
2011-06-28 07:23 PDT, Jonathan Hage
karlt: review-
Details | Diff | Splinter Review
add support for the voffset attribute (part 2) version 3 (4.90 KB, patch)
2011-06-29 00:33 PDT, Jonathan Hage
karlt: review+
Details | Diff | Splinter Review

 Frédéric Wang (:fredw) 2010-04-06 03:16:32 PDT MathML3 adds a new attribute "voffset" to set the position of the child content. (it is the counterpart of lspace for vertical positioning). Frédéric Wang (:fredw) 2011-06-18 00:57:17 PDT 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/ Jonathan Hage 2011-06-20 08:17:46 PDT Created attachment 540474 [details] [diff] [review] Patch 1 Frédéric Wang (:fredw) 2011-06-21 00:28:05 PDT 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." Frédéric Wang (:fredw) 2011-06-21 04:56:01 PDT Created attachment 540713 [details] 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. Jonathan Hage 2011-06-21 07:00:03 PDT Created attachment 540735 [details] [diff] [review] Patch 1 version 2 Frédéric Wang (:fredw) 2011-06-21 09:38:44 PDT Created attachment 540776 [details] testcase 2 Frédéric Wang (:fredw) 2011-06-21 09:41:36 PDT > 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. Jonathan Hage 2011-06-21 13:00:34 PDT Created attachment 540848 [details] [diff] [review] Patch part 2 Karl Tomlinson (:karlt) 2011-06-22 00:01:28 PDT "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? Jonathan Hage 2011-06-22 08:16:00 PDT Created attachment 541054 [details] [diff] [review] Do not allow lspace as a pseudounit Jonathan Hage 2011-06-22 08:17:07 PDT Created attachment 541055 [details] [diff] [review] add support for the voffset attribute part 1 Jonathan Hage 2011-06-22 08:18:35 PDT Created attachment 541056 [details] [diff] [review] add support for the voffset attribute part 2 Frédéric Wang (:fredw) 2011-06-22 13:24:43 PDT (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. Karl Tomlinson (:karlt) 2011-06-23 02:52:54 PDT 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. Jonathan Hage 2011-06-23 08:15:32 PDT Created attachment 541385 [details] [diff] [review] Do not allow lspace as a pseudounit version 2 Jonathan Hage 2011-06-23 10:54:51 PDT Created attachment 541431 [details] [diff] [review] Do not allow lspace as a pseudounit version 3 Jonathan Hage 2011-06-24 02:33:06 PDT Created attachment 541639 [details] [diff] [review] Patch Reftests mpadded Karl Tomlinson (:karlt) 2011-06-26 18:52:35 PDT 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. Karl Tomlinson (:karlt) 2011-06-26 18:58:56 PDT Comment on attachment 541639 [details] [diff] [review] Patch Reftests mpadded >+ div#bigsquare2 { >+ left: 113px; Why should this be at 113px? Frédéric Wang (:fredw) 2011-06-27 11:29:21 PDT 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? Frédéric Wang (:fredw) 2011-06-27 11:36:21 PDT > "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? Frédéric Wang (:fredw) 2011-06-27 11:39:39 PDT > 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? Karl Tomlinson (:karlt) 2011-06-27 13:45:38 PDT (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. Jonathan Hage 2011-06-28 07:10:29 PDT Created attachment 542456 [details] [diff] [review] Patch Reftests mpadded version 2 Jonathan Hage 2011-06-28 07:23:16 PDT Created attachment 542460 [details] [diff] [review] add support for the voffset attribute (part 2) version 2 Jonathan Hage 2011-06-28 07:32:40 PDT 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. Jonathan Hage 2011-06-28 07:47:52 PDT Comment on attachment 542456 [details] [diff] [review] Patch Reftests mpadded version 2 see the crashtest based on the comment 21 Karl Tomlinson (:karlt) 2011-06-28 14:38:40 PDT 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. Jonathan Hage 2011-06-29 00:33:35 PDT Created attachment 542734 [details] [diff] [review] add support for the voffset attribute (part 2) version 3 Frédéric Wang (:fredw) 2011-06-29 01:10:42 PDT 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] Dão Gottwald [:dao] 2011-07-03 01:01:22 PDT 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 Frédéric Wang (:fredw) 2011-07-03 01:19:53 PDT (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. Florian Scholz [:fscholz] (MDN) 2011-07-03 02:50:07 PDT 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 Karl Tomlinson (:karlt) 2011-07-03 13:57:13 PDT (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 Karl Tomlinson (:karlt) 2011-07-03 14:00:29 PDT Fixed in http://hg.mozilla.org/mozilla-central/rev/f354ee7dfcec, thanks Dão. Jesse Ruderman 2011-07-17 14:21:40 PDT Added to DOM fuzzer.

 Note You need to log in before you can comment on or make changes to this bug.