Bug 79274 – Fix <msubsup> formatting, especially for a tall base, subscript, or superscript

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Dave Barton]

Fix <msubsup> formatting

[Dave Barton]

Created attachment 128286 [details]
Patch

[Eric Seidel (no email)]

What (particularly visually) am I looking for in this review?  Could you CC some MathML folks who could comment on if this is a visual improvement/spec-compliance improvement?  I can validate the code, but I have a difficult time validating the mathml-ness.

[Eric Seidel (no email)]

Looking at LayoutTests/platform/mac/mathml/presentation/subsup-expected.png it looks like sub/sup are less widly spaced, which looks nice.

[Dave Barton]

I may write more tomorrow, but basically Frederic Wang or anyone else could look at the 6 .png files at the bottom of the "Formatted Diff" link on the patch attachment above. These are the new results, and the old results are in
http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/mac/mathml/presentation/
(at least until Eric or someone commits the patch and the new results). The source .xhtml and .html files are in
http://svn.webkit.org/repository/webkit/trunk/LayoutTests/mathml/presentation/
in case you want to look at them in another browser. Thanks very much for any help!

Actually I may have misled Eric with the title of this bug. I'm really still doing code clean-up - moving the layout code from stretchToHeight() to layout(), removing the custom baselinePosition() estimate so the true baseline will be computed, etc. In the process, the output becomes slightly prettier. The integral sign in mo-stretch.html is no longer scaled up by an extra 1.2, baselines are more accurate so the base is higher in msubsup-sub-changed.png, etc. The problems before were caused partly by things like wrapping the subscript and superscript in (somewhat) anonymous blocks, where the font-size in the anonymous block was still 16 while the script's was only 12, leading to extra vertical space, etc. It's these coding problems I was really after, but also here are my basic rules for the new <msubsup> layout:

    // Don't let the superscript be below the axis (half x-height), or the subscript above it.
    // Also m_scripts has setVerticalAlign(TOP), and don't let the subscript bottom be above the base bottom.

Here m_scripts is the subscript/superscript pair, so the setVerticalAlign(TOP) means the top of the superscript will equal the top of the base, perhaps with extra padding added on top of the base.

If people do want to suggest other rules to tune this <msubsup> layout now, we could come up with larger examples that show different choices more clearly.

One detail: Eric, can you set the mime-type property on mo-stretch-expected.png? I tried to in svn but maybe it didn't land because I don't actually do the commit.(??)

Thanks again to everyone for all the help. I'll send e-mail asking for some more volunteers to look at the output tomorrow.

[Frédéric Wang (:fredw)]

In general, the MathML REC is intentionally pretty vague about the rendering of elements, so user agents have to use their own heuristic rules...

http://www.w3.org/TR/MathML3/chapter3.html#presm.msubsup

In Mozilla, we take into account subscriptshift/superscriptshift to position scripts vertically (don't know if it is implemented in Webkit). For all the rest, we use TeX rules. They may be quite complicated and involve properties of the frames such as MathML displaystyle or TeX compressed (with the mechanism to transmit private data that I talked about last time):

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmsubsupFrame.cpp#130

(another option is to use information from the Math table of Open Type fonts, but this is much more work)

[Eric Seidel (no email)]

Comment on attachment 128286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128286&action=review

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:159
> +    LineDirectionMode lineDirection = style()->isHorizontalWritingMode() ? HorizontalLine : VerticalLine;
> +    LayoutUnit baseAscent = base->baselinePosition(AlphabeticBaseline, true, lineDirection);
> +    LayoutUnit baseDescent = getBoxModelObjectHeight(base) - baseAscent;
> +    LayoutUnit axis = style()->fontMetrics().xHeight() / 2;
> +    LayoutUnit supHeight = getBoxModelObjectHeight(sup);
> +    LayoutUnit subHeight = getBoxModelObjectHeight(sub);
> +    // Don't let the superscript be below the axis (half x-height), or the subscript above it.
> +    // Also m_scripts has setVerticalAlign(TOP), and don't let the subscript bottom be above the base bottom.
> +    LayoutUnit basePaddingTop = supHeight + axis - baseAscent;
> +    LayoutUnit supPaddingBottom = max<LayoutUnit>(baseDescent + axis - subHeight, 0);
> +    if (basePaddingTop < 0) {
> +        supPaddingBottom += - basePaddingTop;
> +        basePaddingTop = 0;

Could we break some of this out into helper functions with nice names?  I feel like we may be moving slightly backwards in terms of readability of this ::layout() function.

[Dave Barton]

Created attachment 128504 [details]
Patch

[Dave Barton]

I added a test to the end of subsup.xhtml that shows the new layout vs. the old one pretty clearly. I can ask for MathML people to look at it, but I think with this test that won't be necessary. Let me know if you disagree.

I also added a FIXME comment to the code pointing to Frédéric's excellent comment #5 here. Thanks again for all the comments!

P.S. I am only now noticing Eric's comment #6. I'm not sure I agree or know what you mean. I think all this code you indicate needs to be in the same function somewhere. I may be missing your point though?

[Eric Seidel (no email)]

Comment on attachment 128504 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128504&action=review

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:175
> +    RefPtr<RenderStyle> newSubWrapperStyle = RenderStyle::createAnonymousStyle(sub->style());
> +    newSubWrapperStyle->setDisplay(BLOCK);
> +    subWrapper->setStyle(newSubWrapperStyle.release());

Don't we have a createAnonymousBlock() function to use here instead (replacing these 3 lines with one?)

[Eric Seidel (no email)]

Comment on attachment 128504 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128504&action=review

>> 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?

[Eric Seidel (no email)]

Comment on attachment 128504 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128504&action=review

> 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...

[Eric Seidel (no email)]

Comment on attachment 128504 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128504&action=review

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:143
> +    RenderObject* sup = supWrapper->firstChild();
> +    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. :)

[Dave Barton]

Comment on attachment 128504 [details]
Patch

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? Anyway, this code seemed to be the correct implementation, using current techniques.

[Dave Barton]

Julien/Eric/etc. re anonymous blocks in this bug report: To start from the beginning, MathML renderers need to position various DOM element children horizontally & vertically - subscripts, superscripts, underscripts, overscripts, numerators, denominators, etc. Often, however, DOM children just go in a simple row or matrix (mtable). So the closest CSS concept might be that MathML elements are like inline-block elements, including the shrink-to-fit sizing. Somewhat like absolute positioning, a smart renderer's layout() function could compute x and y coordinates for all its children, and then paint() and other methods (computePreferredLogicalWidths(), hit-testing? selections? other stuff in e.g. RenderReplaced.h/cpp??) would access these computed coordinates. This might all be done in a base class, and specific RenderMathML* classes could derive from it, each with their own virtual layout() method. However, I am new to WebKit and don't know how many other methods this would be, so following Alex Milowski's original WebKit MathML work, I'm leaning on RenderBlock instead, at least for now.

Alex's basic technique is to layout children inside nested anonymous RenderBlocks, using block layout to get vertical stacking, and essentially inline-block to get horizontal layout. He then sets CSS padding or sometimes margins to adjust these anonymous RenderBlocks horizontally & vertically if needed. For instance, <msubsup> is like base<sub>subscript</sub><sup>superscript</sup> except that the superscript is supposed to be over the subscript, i.e. start just to the right of the base. Alex's clever trick is to put the superscript & subscript together in an anonymous RenderBlock. The whole thing looks like this:

RenderMathMLSubSup (like inline-block, derives from RenderBlock)
    Anonymous inline-block RenderBlock (for setting its padding-top)
        base
    Anonymous inline-block RenderBlock, with { vertical-align: top }
        Anonymous block RenderBlock (for vertical/block layout, and to set its padding-bottom)
            superscript
        Anonymous block RenderBlock (for vertical/block layout)
            subscript

Then RenderMathMLSubSup::layout() computes and sets these paddings, to control each child's vertical position, followed by calling layout() on the anonymous blocks so the paddings take effect. This use of RenderBlock and CSS may seem like overkill just to set and adjust some horizontal and vertical positions. On the other hand, one could argue that while this use of anonymous blocks is different than their use in normal flow layout, their convenience is analogous. Your opinions are solicited.

If we stay with this approach, a couple complications arise in practice. First, the superscript and subscript can currently be inlines, as discussed in my comment #13 above (which hopefully makes more sense after this introduction). That comment also discusses how to deal with this, or says we can change them to inline-blocks and have a RenderWrapper (RenderPad? RenderMargin?) class that's a simplfied variant of the above absolute-positioning-like technique with exactly one child renderer.

Second, anonymous blocks currently do some things like inherit CSS from their parents, as discussed by Alex M. in https://lists.webkit.org/pipermail/webkit-dev/2011-June/017231.html. I think we can get around this by adding a check, or maybe a flag if necessary, once we have the right design chosen.

So what do you experts think of all this? Does Julien or someone want to write some kind of RenderWrapper class(es) that MathML could use or derive from? Or should we use the current inline-block technique, i.e. RenderBlock, anonymous when necessary? Please let me know your short- and long-term recommendation, and then I'll submit another patch. Thanks!!

[Dave Barton]

P.S. Julien, the other MathML anonymous block issue is that their current implementation confuses RenderBlock and can even cause crashes, as Alex mentions in the link above. I think this is the main reason that MathML isn't turned on in Chrome yet. So I would like to fix those bugs as well, once I get some guidance as to how or whether MathML should use anonymous blocks.

[Dave Barton]

Created attachment 128627 [details]
Patch

[Dave Barton]

In my latest patch in comment 16, a simple wrapper only inherits (copies) its style's inherited data from its child when necessary, i.e. when the child's inherited data (such as its font) has changed. Hope this helps.

[Julien Chaffraix]

If you could ask Alex for an explanation of how he envisioned MathML that should clear most of your questions.

Now RenderBlock is really targeted towards CSS (that defines what a block is). I don't know MathML at all but it really looks like it has a pretty different rendering model from CSS and you are trying to snap it on top of our existing CSS implementation. SVG has the same issue and they have their own class hierarchy so maybe it's something worth considering.

About the anonymous blocks in rendering, those are used when mandated by a spec or an internal constraint (see http://www.webkit.org/blog/115/webcore-rendering-ii-blocks-and-inlines/ for example). Also a lot of the code assumes a certain structure which MathML doesn't seem to fit into. This explains the crashes you are seeing. It's unfortunately not a MathML specific issue (table, run-in, ruby, generated content all have had their share of the same issues).

Please refrain from posting huge pile of text. Reviewers tends to be really busy with the incoming patch stream. I am happy to look into your patches but time reading comments is time I don't spend reviewing any patches.

[Julien Chaffraix]

> Now RenderBlock is really targeted towards CSS (that defines what a block is). I don't know MathML at all but it really looks like it has a pretty different rendering model from CSS and you are trying to snap it on top of our existing CSS implementation. SVG has the same issue and they have their own class hierarchy so maybe it's something worth considering.

Just to add some perspective, I am not familiar with MathML so this can be completely wrong.

[Julien Chaffraix]

Comment on attachment 128627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128627&action=review

There are several logical issues with the underlying code. It looks like MathML is abusing the CSS logic pretty badly.

Now the patch is making the situation slightly worse by re-creating 2 RenderStyle every time we layout (:-(). I would love to understand why.

As Eric mentioned, the padding computation should be moved to a new function for clarity. Thanks for reviving this code.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:52
>  PassRefPtr<RenderStyle> RenderMathMLBlock::createBlockStyle()

The naming is bad. If you are returning an anonymous style, please say so. Looking at the call site, you don't create anonymous renderer but they will get anonymous style which is wrong (sic!). I can't think of any nasty consequences but you are definitely abusing the rendering logic.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:66
> +    RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyle(child->style());
> +    newStyle->setDisplay(BLOCK);
> +    wrapper->setStyle(newStyle.release());

This should use the function above.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:84
> +    static void checkBlockWrapper(RenderObject* wrapper);

The naming is bad. It doesn't really "check" the block.

I have no idea why you would need to recreate your style *every time* you do a layout. This is likely a hidden bug in your implementation as you really shouldn't need to.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:139
> +    RenderObject* superscriptWrapper = m_scripts->firstChild();
> +    RenderObject* subscriptWrapper = m_scripts->lastChild();

Could we get some ASSERT here that you are actually getting the right objects? I fear this could break very badly if you allow CSS generated content (:before / :after) on MathML elements...

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:143
> +    RenderObject* superscript = superscriptWrapper->firstChild();
> +    RenderObject* subscript = subscriptWrapper->firstChild();

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:161
> +        superPaddingBottom += - basePaddingTop;

s/+= -/-=/

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:167
> +    baseWrapper->setNeedsLayout(true);

This is super dangerous (I see it was already present not strictly your fault). Here you will end up trying to mark your parents for layout and because you are in the middle of your layout that could confuse the delayed-layout code.

The good pattern is to mark *only* your children if you need to (see RenderTable::layout() for example which marks only its <caption> children if our logical width changes) and *before* actually laying them.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:171
> +    superscriptWrapper->setNeedsLayout(true);

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.h:-42
> -    virtual LayoutUnit baselinePosition(FontBaseline, bool firstLine, LineDirectionMode, LinePositionMode = PositionOnContainingLine) const;

IMHO this is a good change as our baseline is not impacted by subscript / superscript.

[Julien Chaffraix]

Comment on attachment 128627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128627&action=review

Sorry more comment after reading some more code.

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:84
>> +    static void checkBlockWrapper(RenderObject* wrapper);
> 
> The naming is bad. It doesn't really "check" the block.
> 
> I have no idea why you would need to recreate your style *every time* you do a layout. This is likely a hidden bug in your implementation as you really shouldn't need to.

I know now: we propagate the style change to our anonymous children in styleDidChange. I think you need to implement something similar here and call this function propagateStyleToWrappers with a FIXME saying that this should go away once your child are properly anonymous (it will be handled by propagateStyleToAnonymousChildren).

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:170
> +    superscriptWrapper->style()->setPaddingBottom(Length(superPaddingBottom, Fixed));

This logic looks very familiar to RenderTableCell's intrinsic padding. This is used to add more padding to accommodate vertical-align, I wonder if this would avoid having to set a fake paddingTop just for the purpose of correctly laying out your subscript or superscript. This would need a non-trivial refactoring (you need your very own layout method to account for this padding when laying out) so it should be left for later.

[Dave Barton]

Created attachment 128903 [details]
Patch

[Julien Chaffraix]

Comment on attachment 128903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128903&action=review

We are getting closer. One more iteration and it should be fine I would say.

> Source/WebCore/ChangeLog:3
> +        Fix <msubsup> formatting

BTW usually we prefer bugzilla bug titles to be more precise. Here it's difficult to understand what cases you want to fix (all of them? certain ones? in this case which ones?)

> Source/WebCore/ChangeLog:12
> +        Test: 6 existing test files in mathml/presentation now produce improved results,

Saying how improved they are would help us, people who don't know what MathML should look like. From the traces, it looks like they are more densely laid out but I don't know if it's good or not.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:141
> +    RenderObject* superscriptWrapper = m_scripts->firstChild();
> +    RenderObject* subscriptWrapper = m_scripts->lastChild();

First, the name should be proper camelCase ie superScriptWrapper! (unless those are supposed to be some keywords and I missed it in the spec)

Please add 2 ASSERTs here:
ASSERT(superScriptWrapper->isRenderMathMLBlock());
ASSERT(subScriptWrapper->isRenderMathMLBlock());

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:145
> +    RenderObject* superscript = superscriptWrapper->firstChild();
> +    RenderObject* subscript = subscriptWrapper->firstChild();

Ditto for the camelCase here too.

I really would like an ASSERT that those are *really* what you want. I doesn't look like you implement some isMathMLSubSup() function so it may not be good for this bug but should be either a FIXME or filed as a potential bug.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:151
> +    LayoutUnit baseAscent = base->baselinePosition(AlphabeticBaseline, true, lineDirection);
> +    LayoutUnit baseDescent = getBoxModelObjectHeight(base) - baseAscent;

You have to be super careful in your naming. Ascent and descent have a meaning with fonts which does not totally match what you are doing here: baselinePosition may take into account CSS margins for example.

The first variable is the baseBaseline. The second one is not something I know a standard name for: how about baseExtendUnderBaseline?

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:180
> +    baseWrapper->setNeedsLayout(true);

This should be setNeedsLayout(true, false).

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:183
> +    superscriptWrapper->setNeedsLayout(true);

Ditto.

[Dave Barton]

Created attachment 128922 [details]
Test case showing <msubsup> layout.

I should have submitted this test case first, at the beginning of the bug report.

[Dave Barton]

Comment on attachment 128903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128903&action=review

I'll make the other changes you suggest. They're great, thanks for the care.

The earlier patches had a wrapper inheriting its style from its child during layout. This was due to a misreading of a CSS line layout detail on my part, so this inheriting kludge has been dropped. On the other hand, your TableCell intrinsic padding through virtual padding functions is intriguing for MathML. I'll persue this in e-mail or another patch soon.

>> Source/WebCore/ChangeLog:3
>> +        Fix <msubsup> formatting
> 
> BTW usually we prefer bugzilla bug titles to be more precise. Here it's difficult to understand what cases you want to fix (all of them? certain ones? in this case which ones?)

Good point. I can't change the title now, right?

>> Source/WebCore/ChangeLog:12
>> +        Test: 6 existing test files in mathml/presentation now produce improved results,
> 
> Saying how improved they are would help us, people who don't know what MathML should look like. From the traces, it looks like they are more densely laid out but I don't know if it's good or not.

I've added the main test case as an attachment to the bug. The real issue is vertical positioning of the subscript and superscript, allowing for arbitrary sizes of the various parts (base, subscript, superscript), together with some code clean-up.

>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:141
>> +    RenderObject* subscriptWrapper = m_scripts->lastChild();
> 
> First, the name should be proper camelCase ie superScriptWrapper! (unless those are supposed to be some keywords and I missed it in the spec)
> 
> Please add 2 ASSERTs here:
> ASSERT(superScriptWrapper->isRenderMathMLBlock());
> ASSERT(subScriptWrapper->isRenderMathMLBlock());

subscript and superscript are definitely english words and standard mathematical ones (http://en.wikipedia.org/wiki/Subscript_and_superscript, MathML spec, etc.).

I'll add the ASSERT()s.

[Julien Chaffraix]

Comment on attachment 128903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128903&action=review

>>> Source/WebCore/ChangeLog:3
>>> +        Fix <msubsup> formatting
>> 
>> BTW usually we prefer bugzilla bug titles to be more precise. Here it's difficult to understand what cases you want to fix (all of them? certain ones? in this case which ones?)
> 
> Good point. I can't change the title now, right?

Sure you can. If you don't have EditBug permission on bugzilla, you can only change your own bug's name though.

It's always welcome to improve the title of a bug.

>>> Source/WebCore/ChangeLog:12
>>> +        Test: 6 existing test files in mathml/presentation now produce improved results,
>> 
>> Saying how improved they are would help us, people who don't know what MathML should look like. From the traces, it looks like they are more densely laid out but I don't know if it's good or not.
> 
> I've added the main test case as an attachment to the bug. The real issue is vertical positioning of the subscript and superscript, allowing for arbitrary sizes of the various parts (base, subscript, superscript), together with some code clean-up.

Allowing arbitrary subscript and superscript size sounds like something the bug title should reflect IMHO. The clean-up part can be mentioned in the ChangeLog.

>>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:141
>>> +    RenderObject* subscriptWrapper = m_scripts->lastChild();
>> 
>> First, the name should be proper camelCase ie superScriptWrapper! (unless those are supposed to be some keywords and I missed it in the spec)
>> 
>> Please add 2 ASSERTs here:
>> ASSERT(superScriptWrapper->isRenderMathMLBlock());
>> ASSERT(subScriptWrapper->isRenderMathMLBlock());
> 
> subscript and superscript are definitely english words and standard mathematical ones (http://en.wikipedia.org/wiki/Subscript_and_superscript, MathML spec, etc.).
> 
> I'll add the ASSERT()s.

OK, I am not a native speaker and I am not super familiar with mathml so feel free to push back if some suggestions don't make sense.

[Dave Barton]

Created attachment 128924 [details]
Patch

[WebKit Review Bot]

Comment on attachment 128924 [details]
Patch

Attachment 128924 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11630749

New failing tests:
inspector/protocol/console-agent.html

[Julien Chaffraix]

Comment on attachment 128924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128924&action=review

I think the test failure is a flake. The patch looks good to me, just a couple of tweaks and it is ready to get in.

> Source/WebCore/ChangeLog:3
> +        Fix <msubsup> formatting

You missed the bug title change in your ChangeLog's update.

> Source/WebCore/ChangeLog:12
> +        Test: 6 existing test files in mathml/presentation now produce improved results,

Nit: for posterity, you could copy the improvements you mentioned in the bug here.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:152
> +    // FIXME: Julien Chaffraix suggests that we add some way to check or ASSERT() that subscript and
> +    // superscript are what we think they are, not some :before or :after content or other (future?)
> +    // possibility.

Don't need to mention me (though I like the thought :)). We usually don't mention people in FIXMEs: people change, FIXME usually don't. Here it could be:

// FIXME: We should ASSERT that |subscript| and |superscript| are really the renderers we want (for example generated content could change our hierarchy and confuse us).

[Dave Barton]

Comment on attachment 128924 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128924&action=review

>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:152
>> +    // possibility.
> 
> Don't need to mention me (though I like the thought :)). We usually don't mention people in FIXMEs: people change, FIXME usually don't. Here it could be:
> 
> // FIXME: We should ASSERT that |subscript| and |superscript| are really the renderers we want (for example generated content could change our hierarchy and confuse us).

I don't really understand this, sorry. If the user (web page CSS) created some generated content and it somehow showed up here, shouldn't we just interpret it as a base or subscript or superscript, which is presumably what the user intended? The MathML spec does not prohibit this.

[Dave Barton]

Created attachment 129121 [details]
Patch

[Julien Chaffraix]

Comment on attachment 129121 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129121&action=review

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:152
> +    // FIXME: ASSERT if possible that |superscript| and |subscript| are really the renderers we
> +    // want, i.e. that our hierarchy hasn't been confused by leftover anonymous blocks,
> +    // generated content, etc.

Sorry for the slow answer. As mentioned by Dave offline, RenderMathMLSubSup cannot accept any generated content as it overrides isAllowedChild() to filter out anything that does not have an associated Element. This comment should be removed and sorry for the noise.

[Dave Barton]

Created attachment 129161 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129161 [details]
Patch

Clearing flags on attachment: 129161

Committed r109081: <http://trac.webkit.org/changeset/109081>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.