Bug 96843

Ojan and Tony are your men here. I'm sure they'd be happy to review adding more consumers of flexbox. :)

Created attachment 164258 [details] Patch

Sorry the patch is so large. Unfortunately most MathML tests are currently pixel tests, so there's a lot of metrics output that probably no one but I wants to look at. The actual code in the patch is much shorter. Also WebKit MathML is still in a prototype state - there are no real users of it yet, so we don't have to worry about breaking existing sites. Having said that, I suggest the following: 1. David Carlisle is the main w3c MathML editor, and Frédéric Wang is the main Firefox MathML developer. Would one of you two be willing to look at the .png files at the bottom of the "Review Patch" or "Formatted Diff" link above? The filename above each file is a link to the previous version of the file, before this patch. The only source test files I changed were: mathml/presentation/fenced.xhtml mathml/presentation/mo.xhtml mathml/presentation/row.xhtml mathml/xHeight.xhtml to wrap some <div>s in <mtext> elements to make them valid in MathML, with the intended layout. You can find the changes I made to these files, and links to the previous versions of them, if you really want, in the "Review Patch" or "Formatted Diff" pages as well. 2. Ojan and Tony, to read mathml.css and maybe some .h and .cpp files, you'll need the following brief MathML overview: <math> - toplevel MathML element, contains all others [<mn>, <mi>, <mtext> elements - numbers, identifiers, misc. text respectively; basically just inline elements] <mo> - operator, may be a column of glyphs for a tall symbol like the parentheses around a matrix <mrow> - a row of subexpressions (MathML elements) <mfenced> - a row of subexpressions, perhaps with "fences" (e.g. parentheses) and "separators" (e.g. commas) as attributes <mfrac> [numerator-element] [denominator-element] </mfrac> <msub> [base-element] [subscript-element] </msub> <msup> [base-element] [superscript-element] </msup> <msubsup> [base-element] [subscript-element] [superscript-element] </msubsup> - the superscript is supposed to be directly over the subscript, which we accomplish with a |m_scripts| anonymous box <munder> [base-element] [underscript-element] </munder> <mover> [base-element] [overscript-element] </mover> <munderover> [base-element] [underscript-element] [overscript-element] </munderover> <msqrt> [elements] </msqrt> - square root <mroot> [base-element] [index-element] </mroot> - nth root 3. Eric, Julien, and I believe Dan have some familiarity with the RenderMathML* classes and their code. Thanks in advance to everyone for any partial reviews!

(In reply to comment #3) > 1. David Carlisle is the main w3c MathML editor, and Frédéric Wang is the main Firefox MathML developer. Would one of you two be willing to look at the .png files at the bottom of the "Review Patch" or "Formatted Diff" link above? Do you expect any specific feedback from us? (Personally, I don't really know all these tests)

Comment on attachment 164258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review Honestly, this change looks fine. Its certainly removing a lot of code, which si a step forward. I'll give others a few more days to comment, but I'm happy to rubber stamp this. > Source/WebCore/css/mathml.css:97 > + left: 0; > + top: 0; I think these are default when position absolute.

Comment on attachment 164258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review >> Source/WebCore/css/mathml.css:97 >> + left: 0; >> + top: 0; > > I think these are default when position absolute. According to http://dev.w3.org/csswg/css3-positioning/#box-offsets-trbl their default is 'auto'. (You may know better.) The difference comes when one over-constrains things I think, by setting e.g. all of the left & right & width. Anyway, I was just moving these lines from a little later in the file, to group all the flexbox layout rules together.

(In reply to comment #4) > (In reply to comment #3) > > 1. David Carlisle is the main w3c MathML editor, and Frédéric Wang is the main Firefox MathML developer. Would one of you two be willing to look at the .png files at the bottom of the "Review Patch" or "Formatted Diff" link above? > > Do you expect any specific feedback from us? (Personally, I don't really know all these tests) I have now e-mailed a zip file to David C. & Fred with the old & new .png files & the source to the 13 tests whose .png changed. I told them this patch simplifies the code a lot and fixes several things, thus I'm just asking them to verify that the patch doesn't make the MathML rendering significantly worse on these 13 tests.

(In reply to comment #7) > I have now e-mailed a zip file to David C. & Fred with the old & new .png files & the source to the 13 tests whose .png changed. I told them this patch simplifies the code a lot and fixes several things, thus I'm just asking them to verify that the patch doesn't make the MathML rendering significantly worse on these 13 tests. That's one fancy image comparison page setup:-) I looked at all the tests, there are of course some minor differences as shown in the diff pngs, but as far as I can see the only differences that would be noticeable if it were not for the side by side comparison would be the behaviour of stretchy operators on content with large height where the new behaviour seems better. So no objections to this from here.

Comment on attachment 164258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review Took a quick look. Don't see any glaring problems. > Source/WebCore/css/mathml.css:57 > + -webkit-order: -1; Is this meant to be left-most? If someone does -webkit-order: -2 it will be before this. > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:208 > + LayoutUnit baseline = firstLineBoxBaseline(); // FIXME: This may be unnecessary after flex baselines are implemented. Maybe add a link to the flexbox baseline bug?

Comment on attachment 164258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review Thanks for the quick review! >> Source/WebCore/css/mathml.css:57 >> + -webkit-order: -1; > > Is this meant to be left-most? If someone does -webkit-order: -2 it will be before this. Yes (actually top-most). However, if someone wants to move a subscript or overscript to below an expression, it won't hurt anything except make the layout non-standard, I think. Is there a way to guarantee top-most, other than giving each child an { order: ... !important }? I don't think that's necessary here (you may know better). >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:208 >> + LayoutUnit baseline = firstLineBoxBaseline(); // FIXME: This may be unnecessary after flex baselines are implemented. > > Maybe add a link to the flexbox baseline bug? Excellent, I'll add that when I land the patch.

Comment on attachment 164258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review This looks technically fine to me. I trust that Ojan/Tony gave this at least a cursory flex-box review, and the MathML experts seem happy with your pixel results! Thank you again for all your hard work on this. Please consider the nits below before landing. I look forward to seeing this on in Chrome! > Source/WebCore/rendering/mathml/RenderMathMLBlock.h:123 > +class RenderMathMLTable : public RenderTable { So this whole override will not be necessary once the flexbox baseline bug is fixed? If so, we should note that in a comment (so others can know when to remove this.) > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:58 > + child->style()->setFlexDirection(FlowColumn); I'm not familiar with what this does. Presumably Ojan/Tony are and thought it was OK. :) > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:137 > + LayoutUnit baseBaseline = base->firstLineBoxBaseline(); > + if (baseBaseline == -1) > + baseBaseline = baseHeight; It seems awkward that firstLineBoxBaseline uses this in-band signaling/sentinal value. :( It also seems like this pattern of "lookup or return the height" could be abstracted into a static inline where you pass the baseHeight and the base memeber and get back a single baseBaseline result. It would be used in at least 3 places in this function alone. :)

Comment on attachment 164258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review >> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:123 >> +class RenderMathMLTable : public RenderTable { > > So this whole override will not be necessary once the flexbox baseline bug is fixed? If so, we should note that in a comment (so others can know when to remove this.) No, we'll still need it because the baseline for an <mtable> is different than for a <table>. Actually, that (the need for the virtual function override) wouldn't be true if flexboxes had an analog to vertical-align, but one of the points to the flexbox spec I think is to be simple, and not have complications like vertical-align. (I suppose this could be an issue for the flexbox CSS WG in the future.) Maybe I should point out here that firstLineBoxBaseline() can return different values for <mtable> and <mfrac> and other MathML elements than for old HTML elements like <table>. For instance, a MathML element is considered to be on a single big line, where the baseline of that line may fall in the middle of the element somewhere, not the baseline of the first row of a matrix, for instance. I think I'll add a couple comments about this. >> Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:58 >> + child->style()->setFlexDirection(FlowColumn); > > I'm not familiar with what this does. Presumably Ojan/Tony are and thought it was OK. :) This just says that fractions are in a vertical column, with the numerator over the denominator. With the old box / normal flow model, one could only create a column by wrapping items in blocks, or by using a table. (I guess now one could use the new multi-column layout model, but we want other flexbox features like alignment and order as well.) I can't resist quoting, for MathML purposes, from <http://dev.w3.org/csswg/css3-flexbox/#overview>: Flex layout is superficially similar to block layout. It lacks many of the more complex text- or document-centric properties that can be used in block layout, such as floats and columns. In return it gains simple and powerful tools for distributing space and aligning content in ways that webapps and complex web pages often need. ... [including easy rows & columns, child order changes, alignment, collapsing, etc. - all of which should be of great interest to the MathML committee & implementors] >> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:137 >> + baseBaseline = baseHeight; > > It seems awkward that firstLineBoxBaseline uses this in-band signaling/sentinal value. :( It also seems like this pattern of "lookup or return the height" could be abstracted into a static inline where you pass the baseHeight and the base memeber and get back a single baseBaseline result. It would be used in at least 3 places in this function alone. :) I agree, I think it'd be better if firstLineBoxBaseline() and lastLineBoxBaseline() took a value to return in the case of no baseline, probably with a better default than -1. Outside MathML, I think you usually want to check for that value and do something different, instead of just using the logicalHeight(), by the way. Maybe I'll add your static inline here, and a comment that RenderBox.h & others could be changed in a different bug & patch.

Comment on attachment 164258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:264 > + return (logicalHeight() + style()->fontMetrics().xHeight()) / 2; Shouldn't you take into account any logicalTop() and any margin before into your baseline? > Source/WebCore/rendering/mathml/RenderMathMLBlock.h:127 > + virtual LayoutUnit firstLineBoxBaseline() const OVERRIDE; As a side note, <mtable> will not be contributing to an inline-block's baseline with this implementation, which is probably OK.

Comment on attachment 164258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review >>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:137 >>> + LayoutUnit baseBaseline = base->firstLineBoxBaseline(); >>> + if (baseBaseline == -1) >>> + baseBaseline = baseHeight; >> >> It seems awkward that firstLineBoxBaseline uses this in-band signaling/sentinal value. :( It also seems like this pattern of "lookup or return the height" could be abstracted into a static inline where you pass the baseHeight and the base memeber and get back a single baseBaseline result. It would be used in at least 3 places in this function alone. :) > > I agree, I think it'd be better if firstLineBoxBaseline() and lastLineBoxBaseline() took a value to return in the case of no baseline, probably with a better default than -1. Outside MathML, I think you usually want to check for that value and do something different, instead of just using the logicalHeight(), by the way. > > Maybe I'll add your static inline here, and a comment that RenderBox.h & others could be changed in a different bug & patch. I've changed my mind on this detail. A result of -1 should rarely happen, but when it does occur it often requires a longer calculation. In the interests of efficiency and clarity, I think it's better to have that calculation be in a separate conditional clause, not passed as a parameter to the baseline calculating function. I do agree maybe MIN_LAYOUT_UNIT is a better sentinel than -1, though a bit more complicated. I don't know if it's worth changing RenderBox.h and all other firstLineBoxBaseline() and lastLineBoxBaseline() occurrences.

Committed r128837: <http://trac.webkit.org/changeset/128837>

Comment on attachment 164258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:264 >> + return (logicalHeight() + style()->fontMetrics().xHeight()) / 2; > > Shouldn't you take into account any logicalTop() and any margin before into your baseline? I don't think so. We're not converting from a table row or section's baseline to our coordinate system here, we're just working in our coordinate system. I actually added in marginBefore() at one point, and failing tests led me to what I think is right thinking now. :) >> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:127 >> + virtual LayoutUnit firstLineBoxBaseline() const OVERRIDE; > > As a side note, <mtable> will not be contributing to an inline-block's baseline with this implementation, which is probably OK. Right. In legal MathML, <mtable> & other MathML elements are all contained in other MathML elements, until ultimately a <math> element at the top. These should all be flexboxes now. I added a comment about this to RenderMathMLTable::firstLineBoxBaseline in the RenderMathMLBlock.cpp file that I landed.

There deterioration in rendering of the below test pages with this change. https://eyeasme.com/Joe/MathML/MathML_browser_test https://www.mozilla.org/projects/mathml/demo/texvsmml.html The spacing between the numerator and denominator in fraction(mfrac) is more that what is required. Similar issue wiht superscript (msup)

(In reply to comment #17) > There deterioration in rendering of the below test pages with this change. > > https://eyeasme.com/Joe/MathML/MathML_browser_test > https://www.mozilla.org/projects/mathml/demo/texvsmml.html > > The spacing between the numerator and denominator in fraction(mfrac) is more that what is required. Similar issue wiht superscript (msup) Looking at these two pages, I think the new WebKit rendering is closer to TeX & Firefox than the old one was. It's true that there is a little vertical space above & below the fraction bar, but that seems like a good thing. By the way, you should be able to eliminate the space if you want by overriding these built-in mathml.css lines: mfrac > :first-child { -webkit-margin-after: 0.2em; } mfrac > :last-child { -webkit-margin-before: 0.2em; } I believe you can override them with margin-bottom and margin-top, if you'd prefer that.