Bug 29529 – MathML Support for mover, munder, munderover, msubsup, and mfrac

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Alex Milowski]

Created attachment 39833 [details]
Patch to add rendering code

This patch adds rendering support for munder, munderover, msubsup and mfrac via actual renderer classes.  It also contains tests for these classes.

[Alex Milowski]

Created attachment 39859 [details]
Updated copyright notices to all be the same

[Alexey Proskuryakov]

Comment on attachment 39859 [details]
Updated copyright notices to all be the same

Alex asked to a style review pass on IRC, which I'm happy to do.

+ *     * Neither the name of Alex Milowski nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.

Are you sure that you want the third clause? "it's contributors" looks particularly strange in this context.

+        // If the style is not inline-block, default to styled render object

This is a full sentence, so it needs a dot at the end. Same issue repeated elsewhere.

+namespace WebCore {
+    
+using namespace MathMLNames;

We usually put using directives before WebCore namespace.

+RenderFraction::RenderFraction(Node *fraction) 
+    Element* fraction = static_cast<Element *>(node());
+    RenderFraction(Node *fraction);
+RenderMathInlineContainer::RenderMathInlineContainer(Node *fenced) 
+RenderMathSubSup::RenderMathSubSup(Node *subsup) 
+    RenderObject *current = this->firstChild();

Stars are misplaced in some places.

+        attName = "numalign";

Might it be better to add these to MathMLNames? That way, you'd avoid repeated conversions from ASCII to UTF-16 and memory allocations.

+    if (equalIgnoringCase(align, "left")) {
+
+        // left text align
+        rowStyle->setTextAlign(LEFT);
+
+    } else if (equalIgnoringCase(align, "right")) {
+
+        // right text align
+
+        rowStyle->setTextAlign(RIGHT);
+    }

I do not think that these comments add anything. And there's definitely too much vertical whitespace here.

+        // FIXME: should this be a constant from somewhere?
+        rowStyle->setBorderTopWidth(3);

Yes, we prefer to define constants at the top of cpp files. We currently use "static const", even though there was some discussion recently, because "static" is unnecessary here.

+    return child->node() && child->node()->nodeType() == Node::ELEMENT_NODE ? true : false;

Braces would make this easier to understand.

+#include "RenderMathSubSup.h"
+#include "FontSelector.h"
+#include "MathMLNames.h"
+#include "RenderInline.h"

I'd add an empty line after RenderMathSubSup.h, because otherwise the list looks incorrectly sorted.

+namespace WebCore {
+    
+class RenderMathSubSup : public RenderMathInlineContainer {

In a header, code inside a namespace should be indented (yes, a lot of existing code doesn't do that yet).

+    virtual void addChild(RenderObject* child, RenderObject* beforeChild = 0);
+protected:
+    RenderTable* m_scripts;

We usually put a blank line before "protected" or "private".
+    if (expression->localName() == "mover") {
+        m_kind = Over;
+    } else if (expression->localName() == "munderover") {
+        m_kind = UnderOver;
+    }

There should be no braces around single line blocks.

+    double adjust = -0.4;

These magic values should also go to the top of cpp file.

+const UChar descending[] = {static_cast<UChar>('g') , static_cast<UChar>('j') , static_cast<UChar>('p') , static_cast<UChar>('q') , static_cast<UChar>('y') , 0 };

There shouldn't be spaces before commas.

+    for (int i=0; !descends && descending[i] > 0; i++) {

Should have spaces around '=' in "i=0".

+        // We shouldn't get here but we will treat this as under

You can use ASSERT_NOT_REACHED().

A final patch must include pixel test results. Unless there's a special case for MathML in run-webkit-tests that I'm not aware of, there is no way to make a non-pixel test that dumps a render tree.

[Alex Milowski]

Created attachment 39890 [details]
Fixed for style conformance and added pixel tests.

[Dave Hyatt]

Comment on attachment 39890 [details]
Fixed for style conformance and added pixel tests.

Don't indent class headers.  I sent mail out to webkit-dev about this and apologize for the style guide being wrong.

The code in RenderMathInlineContainer::setStyle is wrong.  You have lost any non-inherited properties with the way you're doing that.   Typically when a style needs to be adjusted we do this over in CSSStyleSelector's adjustRenderStyle method.  I think SVG does some adjustments in its own adjust method, so you may just want to do that over there.

You shouldn't have to set padding to 0 in makeBlockStyle.  Its default value is 0, and padding isn't inherited by default.  Same with margins.  The line-height thing really confuses me. By forcing the line-height to be exactly the font's height, you're not using the line-height that was built in to the font itself.  Is that really what you want?

Similar questions in RenderMathSuperSub regarding why padding/margins are being cleared out.

[Alex Milowski]

I've successfully removed the margin/padding settings.  I believe that is left over from when the stylesheet wasn't being properly loaded.

The line height adjustments are necessary because we want to remove vertical line spacing typically done for a paragraph of text in certain situations.

[Alex Milowski]

Created attachment 39945 [details]
Cleaned up rendering code and removed extra copyright changes

The line height adjustments are still required to get the layout correct.

[Alex Milowski]

Created attachment 40262 [details]
Updated patch containing tweaks to use MathMLNames for tag/attribute names

[Alex Milowski]

Here's my synopsis if that helps:

RenderMathInlineContainer.cpp:

   This class provides the basic support for inline containers (like mfrac)
   that contain other inlines.  It should render as an inline-block.  It exists
   to disallow rendering of text siblings as well as provide some helper
   methods.

RenderFraction.cpp:

   Renders a mfrac ( a fraction) using a inline-block container with
   block flows.  It wraps the containing children in RenderBlock
   instances.

RenderMathSubSup.cpp

   Handles rendering msubsup which represents a base expression that has 
   subscript and superscripts.  An inline table is used for the layout of 
   the scripts.

RenderMathUnderOver.cpp

  Handles rendering munder, mover, and munderover.  The base expression
  has expressions over, under, or both that are organized by properly
  ordering a inline-block container with block flows.  Essentially, one class
  handles all the different cases of re-ordering the rendering of the
  element children.

[Dave Hyatt]

Comment on attachment 40262 [details]
Updated patch containing tweaks to use MathMLNames for tag/attribute names

There is a pretty big conceptual mistake in this code, and it involves the handling of attributes.  You can't simply query for attributes in methods like addChild and just do a one-time fetch, since you basically have destroyed all dynamism by doing so.  You need to be more like HTML/SVG, which either map attributes to style (using the MappedAttribute concept in parseMappedAttribute and mapToEnty), or dirty in some way such that the renderer can regrab relevant info from the element (see various updateFromElement methods).

[Alex Milowski]

Created attachment 40911 [details]
Refactoring to handle attributes properly

I've moved the use of attributes out of the the addChild method and into updateFromElement or other places.  I've also simplified the rendering of munder/mover/munderover.

The class names now start with "RenderMathML".

[Dirk Schulze]

(In reply to comment #10)
> Created an attachment (id=40911) [details]
> Refactoring to handle attributes properly
> 
> I've moved the use of attributes out of the the addChild method and into
> updateFromElement or other places.  I've also simplified the rendering of
> munder/mover/munderover.
> 
> The class names now start with "RenderMathML".

Not sure about licensing but you change the license of  WebCore/mathml/MathMLInlineContainerElement.cpp into what?

[Alex Milowski]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=40911) [details] [details]
> > Refactoring to handle attributes properly
> > 
> > I've moved the use of attributes out of the the addChild method and into
> > updateFromElement or other places.  I've also simplified the rendering of
> > munder/mover/munderover.
> > 
> > The class names now start with "RenderMathML".
> 
> Not sure about licensing but you change the license of 
> WebCore/mathml/MathMLInlineContainerElement.cpp into what?
 
The BSD license.

[Dirk Schulze]

I tested the patch on WebKitGtk. Just some comments.

The first one: on subsup you mixed up the rows of the values. You should compare your results with firefox or opera.
The math values are not in line with the text in the paragraph on WebKitGtk. You can also see this "wrong" behavior on under.xhtml and partly on underover.xhtml. Depend your results on font and font-size?
Not sure if you want to change this in a following patch, but I think the default font style should be italic.

It was a great experience to see MathML on WebKit. I hope you'll continue the work on it Alex.

[Alex Milowski]

(In reply to comment #13)
> I tested the patch on WebKitGtk. Just some comments.
> 
> The first one: on subsup you mixed up the rows of the values. You should
> compare your results with firefox or opera.

No need to compare.  That's just a bug.  Not sure how I didn't notice that.  I'll
update the patch.

> The math values are not in line with the text in the paragraph on WebKitGtk.
> You can also see this "wrong" behavior on under.xhtml and partly on
> underover.xhtml. Depend your results on font and font-size?

The current inline math elements are set to align on the baseline.  The baseline
is calculated from the inline-block and not relative to the contents.  I'd have to
adjust the way things are done to align via the leading term.

> Not sure if you want to change this in a following patch, but I think the
> default font style should be italic.

I'll think about that but it won't go in this patch.

You can change that by CSS styling if you wish.  The code is setup to
allow you to style your math globally by a CSS stylesheet.

[Alex Milowski]

Created attachment 44827 [details]
Updated patch to latest code + fix for subsup

[WebKit Review Bot]

Attachment 44827 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/mathml/RenderMathMLSubSup.cpp:66:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLUnderOver.cpp:75:  Missing space after ,  [whitespace/comma] [3]
WebCore/mathml/RenderMathMLUnderOver.cpp:76:  Missing space after ,  [whitespace/comma] [3]
WebCore/mathml/RenderMathMLUnderOver.cpp:78:  Missing space after ,  [whitespace/comma] [3]
WebCore/mathml/RenderMathMLInlineContainer.cpp:50:  Missing spaces around !=  [whitespace/operators] [3]
WebCore/mathml/RenderMathMLFraction.cpp:49:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLFraction.cpp:54:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLFraction.cpp:71:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 8

[Alex Milowski]

Created attachment 44876 [details]
Updated patch to fix style errors

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44876 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 44876 [details]
Updated patch to fix style errors

Looks like this patch fails to apply and will need an update if the commit-queue is to land it.

[Alex Milowski]

(In reply to comment #19)
> (From update of attachment 44876 [details])
> Looks like this patch fails to apply and will need an update if the
> commit-queue is to land it.

I don't think patch should be committed.  It is now superseded by patches 34277, 34278, 34347.

There isn't a patch for mfrac yet but that is coming.

I should probably close this bug since it is now out of date and won't be updated.

[Alex Milowski]

Comment on attachment 44876 [details]
Updated patch to fix style errors

Removed review request since this patch is now outdated.

[Alex Milowski]

This bug has now be superseded by bugs 33964 34228 34275 34277 34278 34347 34741