Bug 153208

For more information about the rationale of this change check the following thread in the mailing list: https://lists.webkit.org/pipermail/webkit-dev/2015-December/027840.html

Created attachment 269230 [details] Patch

Comment on attachment 269230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269230&action=review > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:152 > + downcast<RenderMathMLOperator>(child)->stretchTo(stretchHeightAboveBaseline, stretchDepthBelowBaseline); I don't understand this. The previous code checked that child was a RenderMathMLBlock and then tried to get the unembellished RenderMathMLOperator.

(In reply to comment #3) > Comment on attachment 269230 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269230&action=review > > > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:152 > > + downcast<RenderMathMLOperator>(child)->stretchTo(stretchHeightAboveBaseline, stretchDepthBelowBaseline); > > I don't understand this. The previous code checked that child was a > RenderMathMLBlock and then tried to get the unembellished > RenderMathMLOperator. Is the patch ready to review or does this question block it?

Created attachment 270103 [details] testcase embellished operator The change breaks stretching embellished operators, I'm surprised that it is not tested yet. If we want to preserve the current behavior, we probably want something like if (is<RenderMathMLBlock>(child)) { if (auto renderOperator = downcast<RenderMathMLBlock>(child)->unembellishedOperator()) renderOperator->stretchTo(stretchHeightAboveBaseline, stretchDepthBelowBaseline); } But actually, we really only want to stretch operators that are "stretchy" and have direction "vertical", so an optimization is: if (is<RenderMathMLBlock>(child)) { auto renderOperator = downcast<RenderMathMLBlock>(child)->unembellishedOperator(); if (renderOperator && renderOperator->hasOperatorFlag(MathMLOperatorDictionary::Stretchy) && renderOperator->isVertical()) renderOperator->stretchTo(stretchHeightAboveBaseline, stretchDepthBelowBaseline); }

(In reply to comment #5) > Created attachment 270103 [details] > testcase embellished operator > > The change breaks stretching embellished operators, I'm surprised that it is > not tested yet. If we want to preserve the current behavior, we probably > want something like > > if (is<RenderMathMLBlock>(child)) { > if (auto renderOperator = > downcast<RenderMathMLBlock>(child)->unembellishedOperator()) > renderOperator->stretchTo(stretchHeightAboveBaseline, > stretchDepthBelowBaseline); > } > > But actually, we really only want to stretch operators that are "stretchy" > and have direction "vertical", so an optimization is: > > if (is<RenderMathMLBlock>(child)) { > auto renderOperator = > downcast<RenderMathMLBlock>(child)->unembellishedOperator(); > if (renderOperator && > renderOperator->hasOperatorFlag(MathMLOperatorDictionary::Stretchy) && > renderOperator->isVertical()) > renderOperator->stretchTo(stretchHeightAboveBaseline, > stretchDepthBelowBaseline); > } Thanks for the comment, I agree with you, I'll replace the code and upload a new patch.

Comment on attachment 269230 [details] Patch Removing patch from review queue for the moment.

(In reply to comment #5) > Created attachment 270103 [details] > testcase embellished operator > > The change breaks stretching embellished operators, I'm surprised that it is > not tested yet. If we want to preserve the current behavior, we probably > want something like > > if (is<RenderMathMLBlock>(child)) { > if (auto renderOperator = > downcast<RenderMathMLBlock>(child)->unembellishedOperator()) > renderOperator->stretchTo(stretchHeightAboveBaseline, > stretchDepthBelowBaseline); > } > > But actually, we really only want to stretch operators that are "stretchy" > and have direction "vertical", so an optimization is: > > if (is<RenderMathMLBlock>(child)) { > auto renderOperator = > downcast<RenderMathMLBlock>(child)->unembellishedOperator(); > if (renderOperator && > renderOperator->hasOperatorFlag(MathMLOperatorDictionary::Stretchy) && > renderOperator->isVertical()) > renderOperator->stretchTo(stretchHeightAboveBaseline, > stretchDepthBelowBaseline); > } Just tested the change does not work. The rationale is correct and without the anonymous and the flexbox we should be able to get the code to look like you say but not with this first patch I'm afraid, that is what makes all this tree structure so complex. I've used your initial proposal and changed the patch, I'll upload again a rebased version. Thanks for the comments.

Created attachment 270200 [details] Patch

Created attachment 270850 [details] testcase Arabic characters At the moment, the MathML code does not make any distinction between logical and ink vertical metrics and so the user agent stylesheet contains the rule "math { -webkit-line-box-contain: glyphs replaced; }" to force the use of ink bounds. This seems to work well in general but fails for Arabic characters. At least on WebKitGTK the computed height is 0 for me and so in the attached testcase, the background is not drawn at all for Arabic characters. With the refactoring done in this bug, the glyph height is really used to determine the logical height of MathML elements and so Arabic characters do not appear at all. I'm not sure that's something we should care for now, but we should think about that in the future. See bug 151592 for more discussions.

Comment on attachment 270200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270200&action=review > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:333 > + there is an extra space here > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:340 > + here too

Comment on attachment 270200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270200&action=review > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:165 > + LayoutUnit ascent = WebCore::marginBoxAscentForChild(*child); Is the WebCore:: prefix really needed? Javier & Rego removed it in a follow-up commit. > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:190 > + LayoutUnit ascent = WebCore::marginBoxAscentForChild(*child); ditto

Created attachment 271160 [details] Patch

(In reply to comment #12) > Comment on attachment 270200 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270200&action=review > > > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:165 > > + LayoutUnit ascent = WebCore::marginBoxAscentForChild(*child); > > Is the WebCore:: prefix really needed? Javier & Rego removed it in a > follow-up commit. > > > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:190 > > + LayoutUnit ascent = WebCore::marginBoxAscentForChild(*child); > > ditto Thanks for the comments, I've just uploaded a new version with your proposals.

(In reply to comment #14) > > > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:190 > > > + LayoutUnit ascent = WebCore::marginBoxAscentForChild(*child); > > > > ditto > > Thanks for the comments, I've just uploaded a new version with your > proposals. I was wrong, I did not test your proposal properly, we need them because we want to use the local version and not the one in flexbox, which is private. I'll review it properly.

Created attachment 271162 [details] Patch

Comment on attachment 271162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271162&action=review Looks good. Before landing, do you mind having people more familiar with flex box to double-check the bits from flex box. > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:105 > + // FIXME: What about the RenderBlocks? If this FIXME is no longer relevant, it should probably just be removed. > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:187 > + centerBlockOffset = - centerBlockOffset; Nit: = - should be =-

Comment on attachment 271162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271162&action=review > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:356 > + return style().writingMode() == TopToBottomWritingMode || style().writingMode() == LeftToRightWritingMode; I do not understand this part. Why does the the writing mode solely decides the flow direction for vertical flow? > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:400 > +} Similar code exists for borders, margins and paddings in RenderStyle and RenderBox. I wish we can have one place which does this confusing calculations. > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:415 > + return marginTop(); If you add the following function to RenderBox: const LayoutBoxExtent& margins() const { return m_marginBox; } This function can be written like this: return child.margins().before(transformedWritingMode()); > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:431 > + return marginBottom(); If you add the following function to RenderBox: const LayoutBoxExtent& margins() const { return m_marginBox; } This function can be written like this: return child.margins().after(transformedWritingMode()); > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:449 > + return TopToBottomWritingMode; I do not understand this part. Why does the text direction affect the writing mode transformation for horizontal flow?

Thanks for the review, I'm adding hyatt to check all the rationale of the patches to check if it is ok to proceed. FYI main idea with these initial set of patches is to remove the flexbox dependency, which means it is not a very clean code until we refactor without the flexbox. The other option would be a huge patch with all the refactors and rebases of the dump render tree tests. It seemed to us this iterative plan makes it easy to divide the work and smaller patches to review, because the amount of code controlling the old flexbox logic added in RenderMathMLBlock is not big and very well identified to try to remove it in the future. (In reply to comment #17) > Comment on attachment 271162 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271162&action=review > > Looks good. Before landing, do you mind having people more familiar with > flex box to double-check the bits from flex box. > Added hyatt to check if he can validate the code. The only part from flexbox are the functions added to RenderMathMLBlock, we tried to minimize that part. > > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:105 > > + // FIXME: What about the RenderBlocks? > > If this FIXME is no longer relevant, it should probably just be removed. > Yep, I'll do it. > > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:187 > > + centerBlockOffset = - centerBlockOffset; > > Nit: = - should be =- Yep. Thanks again.

(In reply to comment #18) > Comment on attachment 271162 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271162&action=review > > > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:356 > > + return style().writingMode() == TopToBottomWritingMode || style().writingMode() == LeftToRightWritingMode; > > I do not understand this part. Why does the the writing mode solely decides > the flow direction for vertical flow? > Good point, we are basically duplicating what flexbox is currently doing for these cases. If I understand correctly the reason it just uses the writing mode for the column flow is because it tries to get the inline base direction, to get the start and end distances, but I'm not an expert on that code so take my comment with a grain of salt, some references: https://www.w3.org/TR/css-flexbox-1/#valdef-flex-direction-column https://drafts.csswg.org/css-writing-modes-3/#text-flow > > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:400 > > +} > > Similar code exists for borders, margins and paddings in RenderStyle and > RenderBox. I wish we can have one place which does this confusing > calculations. > Great point, I'll try to check if we can use those. > > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:415 > > + return marginTop(); > > If you add the following function to RenderBox: const LayoutBoxExtent& > margins() const { return m_marginBox; } > > This function can be written like this: > > return child.margins().before(transformedWritingMode()); > I'll do it thanks. > > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:431 > > + return marginBottom(); > > If you add the following function to RenderBox: const LayoutBoxExtent& > margins() const { return m_marginBox; } > > This function can be written like this: > > return child.margins().after(transformedWritingMode()); > > > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:449 > > + return TopToBottomWritingMode; > > I do not understand this part. Why does the text direction affect the > writing mode transformation for horizontal flow? In this case we are also doing what flexbox is currently doing, we basically want to keep it like it was until we can replace the code with more specific code for MathML. Anyway, I'll try to check if we can reduce the amount of code using your suggestions and upload the patch. Thanks again :).

I'm going to upload a new patch with Martin's nits addressed. Regarding Said's feedback, here are my two cents: > I do not understand this part. Why does the the writing mode solely decides the flow direction for vertical flow? > I do not understand this part. Why does the text direction affect the writing mode transformation for horizontal flow? As Alejandro said, we are basically duplicating flexbox behavior as our idea was to do a smooth transition before a complete refactoring. One of the issue with the current MathML code is that on the one hand flexbox rules are more complex than what is really needed for MathML and on the other hand MathML layout does not always follow layout invariants. So it's often hard to understand the actual rendering behavior and thus to work on MathML & review patches. We plan to submit our simplification in future patches so that all will be much more understandable (in particular, only horizontal RTL and LTR will matter). For now, we have to trust that the flexbox implementation is correct... > Similar code exists for borders, margins and paddings in RenderStyle and RenderBox. I wish we can have one place which does this confusing calculations. Yes, that's bad. However, it's now exactly the same code (it uses "flow" functions) so I'm not sure they would produce the same behavior as flexbox. I suspect we may be able to do that at the current status of our MathML development branch, since we do not care about flow stuff anymore. > If you add the following function to RenderBox: const LayoutBoxExtent& margins() const { return m_marginBox; } > If you add the following function to RenderBox: const LayoutBoxExtent& margins() const { return m_marginBox; } We can certainly do that to simplify the code. However, that would be at the price of adding some code to RenderBox/RenderStyle for MathML's own sake and someone already complained on the webkit-dev mailing list that we should avoid that as much as possible. Again, the plan is to remove/rewrite these functions in the future so I guess it's acceptable to have this temporary functions, given that we will significantly reduce the code size in follow-up patches.

(In reply to comment #21) > it's now exactly *not*

Created attachment 272750 [details] Patch

I uploaded a new version of the patch which includes future refactoring/simplification/cleanup from the MathMLLayout branch. The advantage is that we no longer need to add all these flexbox-like functions that are unrelated to math layout (although I suspect they will be needed later in the other bugs opened by Alex). The disadvantages are that: 1) There are (maybe?) more changes to review in one go. 2) This needs some minor test adjustments. More specifically, the one stretching selection of horizontal stretchy operators has rendering changes that are not important. And I disabled one fraction reftest for now, which has a small 1px size difference (for some reason it only pass after the complete refactoring of RenderMathMLFraction from the MathMLLayout branch).

Created attachment 273700 [details] Patch I'm uploading updated version of the MathML refactoring patches to solve conflicts after bug 154388 and bug 155005 ; and to do other minor changes.

Created attachment 275283 [details] Patch

Comment on attachment 275283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275283&action=review Looks good with a couple nits. > Source/WebCore/ChangeLog:17 > + This is the first patch in the set to remove the flexbox > + dependency of the MathML layout without changing the render tree > + structure. We have done some temporary changes to allow overriding of > + layoutBlock and to implement paintChildren, but this will be remove in a > + follow-up patch. We also implement firstLineBaseline, > + computePreferredLogicalWidths and layoutBlock of RenderMathMLRow without > + using any flexbox functions. We adjust a bit the MathML CSS to take into > + account these changes. Finally, we delete the unused helper function to > + create anonymous RenderMathMLRow. > + Perhaps update this to reflect the most recent version of this patch? > Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:115 > + LayoutUnit horizontalOffset = borderAndPaddingStart(); Let's move this declarations down to right before they are first used (line 142).

Committed r198998: <http://trac.webkit.org/changeset/198998>