[Webkit-unassigned] [Bug 79274] Fix <msubsup> formatting

Thu Feb 23 12:04:21 PST 2012

https://bugs.webkit.org/show_bug.cgi?id=79274

--- Comment #13 from Dave Barton <dbarton at mathscribe.com>  2012-02-23 12:04:21 PST ---
(From update of attachment 128504)
View in context: https://bugs.webkit.org/attachment.cgi?id=128504&action=review

>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:143
>> +    RenderObject* sub = subWrapper->firstChild();
> 
> Although these are the spec names, sup vs. sub is very difficult to read. The single character difference is not immeidately obvious to my eyes. :)

I will fix this and submit another patch.

>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:165
>> +    baseWrapper->style()->setPaddingTop(Length(basePaddingTop, Fixed));
> 
> I guess we don't have custom renderers for these things, so we're setting CSS properties on them instead?  I guess layout happens from root to leaves, so we may not have laid out our children yet, meaning we won't end up doing two layouts here...

We probably will do two layouts. In fact this function starts by calling RenderBlock::layout(). This whole technique of setting padding to move renderers vertically and horizontally may not be best, but it's used throughout the current MathML implementation. I will soon send an e-mail about this to you & J. Chaffraix re the whole anonymous block issue, as used in RenderMathML* classes. We could replace this code with a better solution when we have one, but I wanted to first write the code that clearly demonstrates the use case. Also efficiency doesn't seem like a critical consideration here & now. Mostly I am trying to change one thing at a time in the current code. But if you want to get into this whole issue now, that's fine with me.

>>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:175
>>> +    subWrapper->setStyle(newSubWrapperStyle.release());
>> 
>> Don't we have a createAnonymousBlock() function to use here instead (replacing these 3 lines with one?)
> 
> I see, we're not creating a new block, we're setting the style on the old block.  Why would we replace the RenderStyle on the old block?

This is tricky and took a while for me to debug it. The sup/sub wrappers are basically needed so we can make them { display: block } and also set padding when needed. The problem is that they start out with their parent's font, hence line-height, baseline position, etc. But the subcript and superscript typically have a smaller font size, so extra white space gets generated. I mentioned this in my comment #4 on this whole bug. The correct font size to use is probably 75% of the parent's font size, but this could be affected by a minimum font size cut-off, or even custom font sizes put by the user on the subscript and superscript. So these anonymous wrappers want to inherit styles from their *children*, not their parents. Also the style on the child (subscript or superscript) may have changed, so we may need to re-inherit it, as we do here.

Note we also need the wrappers currently because e.g. <mn> and <mi> (math numbers & identifiers) turn into inline elements, so they need to be in a RenderBlock somewhere. We could eliminate this by making them inline-block but that might be less efficient in common cases(?). What we really need for a start is a way to get an anonymous "wrapper" renderer with a single child, where the child might currently be an inline, such that the wrapper shrinks to fit around its child, with a specified extra padding on certain sides, and such that the wrapper could be made block or inline-block as desired. If the wrapper is a RenderBlock and the child an inline, this means the wrapper's font-size/line-height/baseline must match the child's. Perhaps CSS paddding isn't the best technique to use for all this. If we required the child to be inline-block or block, then we could define a new RenderWrapper class that just added some padding-like space to specified side(s) around its child? Anywa
 y, this code seemed to be the correct implementation, using current techniques.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.