Bug 29529 - MathML Support for mover, munder, munderover, msubsup, and mfrac
: MathML Support for mover, munder, munderover, msubsup, and mfrac
Status: RESOLVED WONTFIX
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 3251
  Show dependency treegraph
 
Reported: 2009-09-20 07:45 PST by
Modified: 2010-02-10 11:31 PST (History)


Attachments
Patch to add rendering code (79.11 KB, patch)
2009-09-20 07:45 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff
Updated copyright notices to all be the same (100.51 KB, patch)
2009-09-21 10:05 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff
Fixed for style conformance and added pixel tests. (308.65 KB, patch)
2009-09-21 15:48 PST, Alex Milowski
hyatt: review-
Review Patch | Details | Formatted Diff | Diff
Cleaned up rendering code and removed extra copyright changes (289.83 KB, patch)
2009-09-22 14:21 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch containing tweaks to use MathMLNames for tag/attribute names (290.08 KB, patch)
2009-09-28 15:31 PST, Alex Milowski
hyatt: review-
Review Patch | Details | Formatted Diff | Diff
Refactoring to handle attributes properly (290.68 KB, patch)
2009-10-08 14:20 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch to latest code + fix for subsup (288.04 KB, patch)
2009-12-14 16:07 PST, Alex Milowski
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch to fix style errors (288.03 KB, patch)
2009-12-15 07:13 PST, Alex Milowski
eric: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-09-20 07:45:27 PST
Created an attachment (id=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.
------- Comment #1 From 2009-09-21 10:05:35 PST -------
Created an attachment (id=39859) [details]
Updated copyright notices to all be the same
------- Comment #2 From 2009-09-21 10:21:59 PST -------
(From update of attachment 39859 [details])
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.
------- Comment #3 From 2009-09-21 15:48:39 PST -------
Created an attachment (id=39890) [details]
Fixed for style conformance and added pixel tests.
------- Comment #4 From 2009-09-22 13:14:15 PST -------
(From update of attachment 39890 [details])
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.
------- Comment #5 From 2009-09-22 14:07:24 PST -------
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.
------- Comment #6 From 2009-09-22 14:21:46 PST -------
Created an attachment (id=39945) [details]
Cleaned up rendering code and removed extra copyright changes

The line height adjustments are still required to get the layout correct.
------- Comment #7 From 2009-09-28 15:31:33 PST -------
Created an attachment (id=40262) [details]
Updated patch containing tweaks to use MathMLNames for tag/attribute names
------- Comment #8 From 2009-10-02 13:37:14 PST -------
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.
------- Comment #9 From 2009-10-07 14:17:23 PST -------
(From update of attachment 40262 [details])
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).
------- Comment #10 From 2009-10-08 14:20:09 PST -------
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".
------- Comment #11 From 2009-10-16 05:32:20 PST -------
(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?
------- Comment #12 From 2009-10-16 07:18:41 PST -------
(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=40911) [details] [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.
------- Comment #13 From 2009-12-02 13:24:26 PST -------
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.
------- Comment #14 From 2009-12-14 15:20:33 PST -------
(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.
------- Comment #15 From 2009-12-14 16:07:14 PST -------
Created an attachment (id=44827) [details]
Updated patch to latest code + fix for subsup
------- Comment #16 From 2009-12-14 21:05:33 PST -------
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
------- Comment #17 From 2009-12-15 07:13:46 PST -------
Created an attachment (id=44876) [details]
Updated patch to fix style errors
------- Comment #18 From 2009-12-15 07:14:54 PST -------
style-queue ran check-webkit-style on attachment 44876 [details] without any errors.
------- Comment #19 From 2010-02-01 16:28:04 PST -------
(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.
------- Comment #20 From 2010-02-01 18:07:53 PST -------
(In reply to comment #19)
> (From update of attachment 44876 [details] [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.
------- Comment #21 From 2010-02-10 11:26:02 PST -------
(From update of attachment 44876 [details])
Removed review request since this patch is now outdated.
------- Comment #22 From 2010-02-10 11:31:14 PST -------
This bug has now be superseded by bugs 33964 34228 34275 34277 34278 34347 34741