Bug 88476

Created attachment 146164 [details] Patch

This bug actually blocks a lot of MathML bug fixes - layout corrections, use of -webkit-line-box-contain, even security issues. Basically I need to correct MathML's usage of anonymous blocks before submitting other patches. Any review would be appreciated!

Comment on attachment 146164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146164&action=review Overall, this is a huge progression. Some comments (mostly about forms that the core). > Source/WebCore/ChangeLog:9 > + "generated". Standard WebCore practice is to mark such a renderer as isAnonymous(). "generated" is bit different than "anonymous" AFAICT: generated refers to generated content, which is CSS created content (thus anonymous as it is not a Node's renderer). > Source/WebCore/rendering/mathml/RenderMathMLBlock.h:85 > Do we want to also override createAnonymousBoxWithSameTypeAs for MathMLBlock? (that would ensure that we create the right type of objects in the RenderBlock logic) > Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:157 > +// FIXME: Change createMathMLOperator() above to create an isAnonymous() operator, and remove this styleDidChange() function. Nit: an anonymous operator would read better IMHO. > Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:126 > + updateFromElement(); This doesn't look good. updateFromElement is normally called from the DOM side to update the renderer. It's kind of a blunt notification to mean that there are non-style related changes that the renderer should respond to. Calling it in styleDidChange is not advisable. If you need to patch your style (which I assume looking at updateFromElement), this should be moved to a common function. That said, if you intent to remove styleDidChange soon, a FIXME underlining the previous violation is fine by me. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:77 > + updateFromElement(); Same remark here. > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:124 > + if (child->firstChild() != base) > + child->style()->setTextAlign(CENTER); We probably will want to replace that with a custom selector at some point. > LayoutTests/platform/mac/mathml/presentation/mo-stretch-expected.txt:212 > + RenderMathMLBlock (anonymous) at (0,16) size 14x18 As you are changing all text baselines, it would be neat to actually dump useful info here. The new output makes it harder to know what you are actually dumping (especially since a lot of the renderer in MathML land are RenderMathMLBlock). It's just a matter of overriding renderName and doing the proper treatment.

Comment on attachment 146164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146164&action=review Thanks for the encouragement, instruction, and review! >> Source/WebCore/ChangeLog:9 >> + "generated". Standard WebCore practice is to mark such a renderer as isAnonymous(). > > "generated" is bit different than "anonymous" AFAICT: generated refers to generated content, which is CSS created content (thus anonymous as it is not a Node's renderer). Makes sense, thanks. I'll update this ChangeLog. >> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:85 >> > > Do we want to also override createAnonymousBoxWithSameTypeAs for MathMLBlock? (that would ensure that we create the right type of objects in the RenderBlock logic) I don't think so. If one inserts a non-inline in a MathML container, the child is supposed to be wrapped in an anonymous <mtext>, presumably with { display: inline-block }. So automatic box splitting and combining shouldn't happen with MathML elements or renderers, especially since argument (child) position is usually significant. >> Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:157 >> +// FIXME: Change createMathMLOperator() above to create an isAnonymous() operator, and remove this styleDidChange() function. > > Nit: an anonymous operator would read better IMHO. By my definition above, technically (arguably) it's an anonymous renderer now, since it isn't its node's main renderer. I could change if you insist, but I want to emphasize that isAnonymous() is what's desired. >> Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:126 >> + updateFromElement(); > > This doesn't look good. updateFromElement is normally called from the DOM side to update the renderer. It's kind of a blunt notification to mean that there are non-style related changes that the renderer should respond to. > > Calling it in styleDidChange is not advisable. If you need to patch your style (which I assume looking at updateFromElement), this should be moved to a common function. > > That said, if you intent to remove styleDidChange soon, a FIXME underlining the previous violation is fine by me. There are many issues with current MathML updateFromElement methods, ranging from simple bugs to MathML spec issues about the interaction of MathML with CSS. I don't think styleDidChange will be removed soon, but updateFromElement will be reworked, and this patch is already large, so I'd prefer to add a FIXME (before updateFromElement) at this point. Right now addChild and even layout() also call updateFromElement, so I'm not making things much worse, and I'd like to postpone the cleanup and do them all at once. >> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:124 >> + child->style()->setTextAlign(CENTER); > > We probably will want to replace that with a custom selector at some point. I don't know much about custom selectors. Why would that be better? This code is just saying that in mathematics, underscripts and overscripts are always centered horizontally. >> LayoutTests/platform/mac/mathml/presentation/mo-stretch-expected.txt:212 >> + RenderMathMLBlock (anonymous) at (0,16) size 14x18 > > As you are changing all text baselines, it would be neat to actually dump useful info here. The new output makes it harder to know what you are actually dumping (especially since a lot of the renderer in MathML land are RenderMathMLBlock). It's just a matter of overriding renderName and doing the proper treatment. Will do.

Created attachment 147159 [details] Patch

Comment on attachment 147159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147159&action=review r=me, with comments. > I don't know much about custom selectors. Why would that be better? This code is just saying that in mathematics, underscripts and overscripts are always centered horizontally. Custom selectors enables you to specify a value for the object in CSS without needing the rendering code to know about those details. This removes a lot of burden as the CSS code already handles everything for you (including if people want ways to override it) but it's an overkill for now. More something to remember in the future :) > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:200 > +const char* RenderMathMLBlock::renderName() const I would advise you to dump if the object is "anonymous". It may not seem important but CSS is not consistent on that and it makes debugging difficult. I am a strong advocate of having consistency on that (meaning that we should have a core function doing the core annotation as it's too easy to miss). > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:203 > + bool hasPadding = m_intrinsicPaddingBefore || m_intrinsicPaddingAfter || m_intrinsicPaddingStart || m_intrinsicPaddingEnd || style()->hasPadding(); Not sure how important is the padding and I will let you weight on that. If I were in your shoes, I would not dump just "padded" as it would be better to dump the exact padding values (not that for legacy reason table cells don't do it...). Dumping the exact values is pretty easy and just involves hacking RenderTreeAsText. > Source/WebCore/rendering/mathml/RenderMathMLRow.h:38 > + static RenderMathMLRow* createAnonymousMRowWithParentRenderer(const RenderObject*); The original naming matches the one in the rendering directory. It was chosen to be generic as you call it: RendererClass::createAnonymousWithParentRenderer() (note that you know the type as you call it on the associated class). I would rather keep the original naming for coherency.

Created attachment 147420 [details] Patch

Sorry Julien, I need one more line of review. I made the changes you suggested, but also added a line to RenderMathMLOperator::styleDidChange. We need to only call updateFromElement there (to essentially apply the style change to the renderer's children) if the renderer has any children. Otherwise the renderer gets initialized before its node (element) has had its child added, and the operator may never get displayed. I added a test for this in over.xhtml. Thanks! (This will all be fixed more sanely soon, when RenderMathMLOperator::isChildAllowed is changed to return true, so that renderer children can be added at the normal RenderObject::addChild() time.)

Comment on attachment 147420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147420&action=review > Source/WebCore/rendering/RenderTreeAsText.cpp:383 > + // We want to check for CSS or intrinsic padding, so we can't just check o.style()->hasPadding(). I think this would better read: // We want to check the layout padding (that includes computed and intrinsic paddings) so we can't ask the renderer's style. Feel free to tweak it to your needs. > Source/WebCore/rendering/RenderTreeAsText.cpp:386 > + LayoutUnit cssTop = box.computedCSSPaddingTop(), cssRight = box.computedCSSPaddingRight(), cssBottom = box.computedCSSPaddingBottom(), cssLeft = box.computedCSSPaddingLeft(); I thought it was a rule in the coding style guide to have one declaration per line but it doesn't seem to be. For consistency, it would be better to do it though. > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:207 > + // We won't reach here currently. You can enforce that with ASSERT_NOT_REACHED() and give a 'why' comment (like 'We force our display to be one of the above so this shouldn't be reached').

Committed r120492: <http://trac.webkit.org/changeset/120492>