Add Support for mspace element

• Component: MathML
• Status: RESOLVED FIXED

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

I thought this was already supported and didn't report it before, but according to

https://trac.webkit.org/wiki/MathML%20Status

that's not the case. Supporting at least width/height/depth would be useful to help MathML generators to tweak the spacing of formulas. MathJax also uses negative spacing (negative width).

Here is a convenient dynamic test:

https://www.mozilla.org/projects/mathml/demo/mspace.html

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

Just FYI, I started to work on this today. It looks like a parsing function like

http://mxr.mozilla.org/mozilla-central/source/content/mathml/content/src/nsMathMLElement.cpp#310

will be necessary. This is already needed for mfrac.

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

Created attachment 201993 [details]
WIP Patch

Experimental support for the mspace element.

I guess negative spacing will not be considered in this bug, as that will require special layout of elements not available in CSS.

Currently, there are hard coded default values width=30px, height=10px and depth=20px but it's just to verify that the patch works correctly. As said in comment 1, a parsing function is necessary for MathML length (similar to CSS values, but with some differences):

http://www.w3.org/TR/MathML/chapter2.html#type.length

I'm not familiar with WebKit String manipulation or Style structures, so I'll need some help to implement such a parsing function properly.

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

Created attachment 202118 [details]
Patch V2

OK, I added a parsing function for MathML Length, similar to the one in Gecko and I used that to parse the mspace attributes (and mfrac@linethickness). For the moment, unitless, % and px values should work, but I'm still not quite sure about how to handle the other CSS units.

@Martin: Any idea about how to convert the other CSS units to pixels? I tried things with style()->fontMetrics().xHeight(), style()->font().spaceWidth() or even style()->fontMetrics().unitsPerEm(), but that does not seem to work very well. There are probably functions in Webkit to do that conversion?

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

> @Martin: Any idea about how to convert the other CSS units to pixels?

Never mind, I think I figured it out. I'll come back when I have tests.

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

Created attachment 202203 [details]
Patch V3

Additional note: I did as in Gecko and provided separate ParseNumericValue and ParseNamedSpaceValue functions. IIRC, mpadded attribute parsing is a bit different and it was convenient to have separate functions.

[EFL EWS Bot]

Comment on attachment 202203 [details]
Patch V3

Attachment 202203 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/498295

[EFL EWS Bot]

Comment on attachment 202203 [details]
Patch V3

Attachment 202203 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/491715

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

Oops, I think I forgot to edit Source/WebCore/WebCore.vcproj/WebCore.vcproj...

[Build Bot]

Comment on attachment 202203 [details]
Patch V3

Attachment 202203 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/493650

[Build Bot]

Comment on attachment 202203 [details]
Patch V3

Attachment 202203 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/495679

[Build Bot]

Comment on attachment 202203 [details]
Patch V3

Attachment 202203 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/494688

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

Created attachment 202204 [details]
Patch V4

[Build Bot]

Comment on attachment 202204 [details]
Patch V4

Attachment 202204 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/497395

[EFL EWS Bot]

Comment on attachment 202204 [details]
Patch V4

Attachment 202204 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/490748

[EFL EWS Bot]

Comment on attachment 202204 [details]
Patch V4

Attachment 202204 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/494693

[Build Bot]

Comment on attachment 202204 [details]
Patch V4

Attachment 202204 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/493661

[Build Bot]

Comment on attachment 202204 [details]
Patch V4

Attachment 202204 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/495684

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

Created attachment 202212 [details]
Patch V5

[EFL EWS Bot]

Comment on attachment 202212 [details]
Patch V5

Attachment 202212 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/492774

[EFL EWS Bot]

Comment on attachment 202212 [details]
Patch V5

Attachment 202212 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/500257

[Build Bot]

Comment on attachment 202212 [details]
Patch V5

Attachment 202212 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/493719

[Build Bot]

Comment on attachment 202212 [details]
Patch V5

Attachment 202212 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/488885

[Build Bot]

Comment on attachment 202212 [details]
Patch V5

Attachment 202212 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/493720

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

Created attachment 202388 [details]
Patch V5 - bis

[EFL EWS Bot]

Comment on attachment 202388 [details]
Patch V5 - bis

Attachment 202388 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/422172

[EFL EWS Bot]

Comment on attachment 202388 [details]
Patch V5 - bis

Attachment 202388 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/524141

[Build Bot]

Comment on attachment 202388 [details]
Patch V5 - bis

Attachment 202388 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/527099

[Build Bot]

Comment on attachment 202388 [details]
Patch V5 - bis

Attachment 202388 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/525126

[Build Bot]

Comment on attachment 202388 [details]
Patch V5 - bis

Attachment 202388 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/498686

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

Created attachment 202612 [details]
Patch V5 - try to build again

[Build Bot]

Comment on attachment 202612 [details]
Patch V5 - try to build again

Attachment 202612 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/582001

[Build Bot]

Comment on attachment 202612 [details]
Patch V5 - try to build again

Attachment 202612 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/559009

[Build Bot]

Comment on attachment 202612 [details]
Patch V5 - try to build again

Attachment 202612 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/579001

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

So it seems the latest version builds on everything but Mac/Windows. Martin, can you please try to edit the XCode files with your Mac?

--

Dave just mentioned the following code in mathml.css

mspace[linebreak="newline"] {
  display: block;
}

that used to be used to create a newline but no longer works with the flex boxes. So that can be removed (the attribute is deprecated in MathML BTW ; line breaking is done on mo operators in MathML3)

[Martin Robinson]

Here's the error on Windows: 7>WebCore.lib(MathMLAllInOne.obj) : error LNK2019: unresolved external symbol "public: __thiscall WebCore::RenderMathMLSpace::RenderMathMLSpace(class WebCore::Element *)" (??0RenderMathMLSpace@WebCore@@QAE@PAVElement@1@@Z) referenced in function "private: virtual class WebCore::RenderObject * __thiscall WebCore::MathMLTextElement::createRenderer(class WebCore::RenderArena *,class WebCore::RenderStyle *)" (?createRenderer@MathMLTextElement@WebCore@@EAEPAVRenderObject@2@PAVRenderArena@2@PAVRenderStyle@2@@Z)
7>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin\WebKit.dll : fatal error LNK1120: 1 unresolved externals

Perhaps you need to add the new file to the MathMLAllInOne.cpp file?

[Martin Robinson]

It seems you also removed something that looks like a BOM from the beginning of the file. I'm not sure if that's important or not...

[Martin Robinson]

I cannot see anything immediately wrong with the additional lines in the XCode file, but you might try running xcodebodge and see if it produces something noticeably different: http://svn.jacekowski.org/chromium/trunk/tools/xcodebodge/xcodebodge.py

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

Created attachment 203729 [details]
Patch V5 - ter

> Perhaps you need to add the new file to the MathMLAllInOne.cpp file?

I doubt this is the problem. The new files are for the mspace element and are in rendering/ while MathMLAllInOne does not contain any reference for other elements (mfrac, mfenced, msqrt etc).

> It seems you also removed something that looks like a BOM from the beginning of the file. I'm not sure if that's important or not...

That could well be the issue. I just refreshed the patch and will try again without this modification. If that does not work I'll try the Python script later.

[Build Bot]

Comment on attachment 203729 [details]
Patch V5 - ter

Attachment 203729 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/711501

[Build Bot]

Comment on attachment 203729 [details]
Patch V5 - ter

Attachment 203729 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/673572

[Build Bot]

Comment on attachment 203729 [details]
Patch V5 - ter

Attachment 203729 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/714560

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

Created attachment 203758 [details]
Patch V5 - quater

[chris fleizach]

Comment on attachment 203758 [details]
Patch V5 - quater

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

You should separate the value parsing from the MathMLSpace patch into two patch I think, so you can address the line thickness issue separately.

Thanks

> LayoutTests/mathml/presentation/mfrac-linethickness-expected-mismatch.html:4
> +    <title>mfrac linethickness</title>

what is this testing? there's nothing related to mfrac in here

> LayoutTests/mathml/presentation/mspace-expected.html:1
> +<!DOCTYPE html>

Your layout tests probably shouldn't be suffixed with -expected, because that's the suffix for the expected results.

I also don't see any results from running these layout tests...

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:-1
> -<?xml version="1.0" encoding="utf-8"?>

inccorrect addition here?

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:317
> +    return false;

It seems a shame this is specific to MathML and not re-usable. I see code in CSSPrimitiveValue that handles some of these cases it appears

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:322
> +    int i = 0;

you should choose a better name for this variable, plus it looks like it should be a float anyway

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

(In reply to comment #43)
> You should separate the value parsing from the MathMLSpace patch into two patch I think, so you can address the line thickness issue separately.

Yes that's probably what I'll do once MathJax 2.3 (hopefully this summer) is released and I can work on WebKit MathML again. Thanks for the review!

> 
> Thanks
> 
> > LayoutTests/mathml/presentation/mfrac-linethickness-expected-mismatch.html:4
> > +    <title>mfrac linethickness</title>
> 
> what is this testing? there's nothing related to mfrac in here

This is a != reftest as indicated by the "expected-mismatch" suffix. The explanation is in the corresponding test LayoutTests/mathml/presentation/mfrac-linethickness.html

> 
> > LayoutTests/mathml/presentation/mspace-expected.html:1
> > +<!DOCTYPE html>
> 
> Your layout tests probably shouldn't be suffixed with -expected, because that's the suffix for the expected results.

Not sure what you mean... That's a == reftest to compare with LayoutTests/mathml/presentation/mspace.html so the -expected suffix is necessary?

> 
> I also don't see any results from running these layout tests...
> 
> > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:-1
> > -<?xml version="1.0" encoding="utf-8"?>
> 
> inccorrect addition here?

Ooops, I thought I had fixed that one.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:317
> > +    return false;
> 
> It seems a shame this is specific to MathML and not re-usable. I see code in CSSPrimitiveValue that handles some of these cases it appears

That's what I asked above but couldn't find these CSS primitive alone. However some work is needed to parse the MathML-specific stuff like the unitless case you implemented, so a separate function is necessary anyway. Note that using a MathML-specific routine is also what Gecko does.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:322
> > +    int i = 0;
> 
> you should choose a better name for this variable, plus it looks like it should be a float anyway

True. (I just copied that from the Gecko code...)

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

Just to be sure we agree, here is the document I read about WebKit convention regarding reftests: http://trac.webkit.org/wiki/Writing%20Reftests

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

Created attachment 205506 [details]
Patch V6

[EFL EWS Bot]

Comment on attachment 205506 [details]
Patch V6

Attachment 205506 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/967343

[Build Bot]

Comment on attachment 205506 [details]
Patch V6

Attachment 205506 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/916719

[EFL EWS Bot]

Comment on attachment 205506 [details]
Patch V6

Attachment 205506 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/919270

[kov's GTK+ EWS bot]

Comment on attachment 205506 [details]
Patch V6

Attachment 205506 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/946382

[Build Bot]

Comment on attachment 205506 [details]
Patch V6

Attachment 205506 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/978824

[Build Bot]

Comment on attachment 205506 [details]
Patch V6

Attachment 205506 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/901226

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

Created attachment 205723 [details]
Patch V7

Some remarks:

- Dynamically changes via Javascript does not seem to work. That seems to be a general issue in WebKit MathML (e.g. with linethickness), so that should better be fixed in a separate bug.

- The parsing of pt and pc was broken when I replaced float constants by integers in the MathML length patch. This is fixed and tested here.

- I had hard time to edit the XCode make file by hand and finally tried a Python script suggested by Martin. However, the changes do not seem really consistent with the existing source code so I'd appreciate if someone with a Mac can edit that file with XCode.

[chris fleizach]

Comment on attachment 205723 [details]
Patch V7

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

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:299
> +        lengthValue = 4 * floatValue / 3;

use parenthesis to make it clear what the order of operations are

> Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:30
> +#include "RenderMathMLSpace.h"

this should go right below config

> Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:48
> +bool RenderMathMLSpace::isChildAllowed(RenderObject*, RenderStyle*) const

this can be put in the header

> Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:57
> +    m_maxPreferredLogicalWidth = m_minPreferredLogicalWidth = m_width;

don't do cascading equals. put on separate line

> Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:67
> +    m_width = m_height = m_depth = 0;

ditto about equals

> Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:73
> +    if (m_width <= 0)

this should just be m_width < 0

> Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:77
> +    if (m_height + m_depth <= 0)

< 0 instead of <= 0. I would also add parens

> Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:78
> +        m_height = m_depth = 0;

ditto about the equals

> Source/WebCore/rendering/mathml/RenderMathMLSpace.h:37
> +    RenderMathMLSpace(Element*);

explicit needed before the constructor

> Source/WebCore/rendering/mathml/RenderMathMLSpace.h:43
> +    virtual void updateFromElement();

are these also OVERRIDES?

can all these virtual methods move into private space

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

Created attachment 205733 [details]
Patch V8

[Build Bot]

Comment on attachment 205733 [details]
Patch V8

Attachment 205733 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/987486

New failing tests:
mathml/presentation/mspace-units.html

[Build Bot]

Created attachment 205740 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

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

Created attachment 205741 [details]
Patch V9

It seems that there is a small pixel failure in Mac due to rounding errors. Trying again with a modified length computation.

[Build Bot]

Comment on attachment 205741 [details]
Patch V9

Attachment 205741 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/981846

New failing tests:
mathml/presentation/mspace-units.html

[Build Bot]

Created attachment 205751 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205741 [details]
Patch V9

Attachment 205741 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/913443

New failing tests:
mathml/presentation/mspace-units.html
svg/batik/filters/feTile.svg

[Build Bot]

Created attachment 205762 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

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

Created attachment 205764 [details]
Patch V10

[WebKit Commit Bot]

Comment on attachment 205764 [details]
Patch V10

Rejecting attachment 205764 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 205764, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/860684

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

Created attachment 205774 [details]
Patch Final Version

[WebKit Commit Bot]

Comment on attachment 205774 [details]
Patch Final Version

Rejecting attachment 205774 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 205774, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ubmit
    return self.open(self.click(*args, **kwds))
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

Full output: http://webkit-queues.appspot.com/results/887325

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

Comment on attachment 205774 [details]
Patch Final Version

Not sure why the commit failed. Apparently everything is green, so that seemed to be an infrastructure problem.

[WebKit Commit Bot]

Comment on attachment 205774 [details]
Patch Final Version

Clearing flags on attachment: 205774

Committed r152235: <http://trac.webkit.org/changeset/152235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

This caused some failures on debug bots: https://bugs.webkit.org/show_bug.cgi?id=118298

[Alexey Proskuryakov]

Testing bugzilla reopen functionality, sorry for the noise.

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

Mass change: add WebExposed keyword to help MDN documentation.