276267 - MathML elements as nsMathMLElement

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Daniel Kraft]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a6) Gecko/20041219
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a6) Gecko/20041219

I don't know how MathML rendering exactly works (the various nsMath*Frames are
applied), but MathML elements are currently represented as normal nsXMLElements.
There should be, similiar to SVG, special classes for MathML elements (also to
implement MathML-DOM). Currently, the only specific behaviour when encountering
a MathML element in the tree is to ensure that mathml.css is loaded and to
create a nsXMLElement.

Reproducible: Always

Steps to Reproduce:
Apply alert(...) to a MathML-element.
Actual Results:  
You'll just see "[object Element]"

Expected Results:  
Something like, for example, "[object MathMLfracElement]".

Comment 1

[Daniel Kraft]

Attachment: Testcase

Testcase which alerts a MathML-mn-element and a HTML-p-element to see a
difference.

Comment 2

[rbs]

The issue is not just to have MathML-elements for sake of having them. I am well
aware that they have to be added at some point. But so far, there hasn't been
any compelling reasons to drop everything else and go add MathML-elements in all
haste. It will be bloat for the sake of bloat. As the current code demonstrates,
rendering can be done without that undue extra.

What is more influential is the MathML DOM and that is bug 143842.

Comment 3

[Daniel Kraft]

One reason why I think this should be done IS MathML-DOM, and the second is to
allow us to include some special methods we can use for implementation of
Content Markup (Bug #276028).
Of course it would be nice also link in some way to the nsMath*Frame-classes (so
that MathML-elements could do their rendering) but at the moment I don't really
know how those frames work, so I can't do that task at the moment.
But I'd try to:
 1) Build classes for MathML
 2) Implement the MathML-DOM with these
if you think this is ok.

Comment 4

[rbs]

Rendering can be (and has been) done without implementing separate
nsMathMLElements (or even the MathML DOM). But that is not to say that they
shouldn't be considered. It depends on timing/priority/usefulness.

I was thinking of supporting Content Markup via some XBL wizardry (like in the
xbl-maquee). Feel free to look into that and/or take up the bug to implement the
MathML DOM.

Comment 5

[Daniel Kraft]

At the moment I've no idea how to implement Content Markup via XBL (of course, I
also don't know much XBL...), but I think it's important to have the Content
Markup in the XML-Tree, so that Mozilla may be extended to support automatic
solution of equations or something like that, which may be useful in some way
(or maybe a CAS with MathML/SVG exporting to Composer to allow scientific
editing at advanced level). But Content Markup isn't important yet.
As you suggested, I'll try implementing separated elements and MathML-DOM.

Comment 6

[Daniel Kraft]

Attachment: nsMathMLElement implemented

This patch (tar-gzipped together with a couple of new files in new directories)
implements the nsMathMLElement. Currently, for every element in the
MathML-namespace a nsMathMLElement instance is created, and it only supports
GetXref, because the main tasks I worked on were to check my class into
Mozilla's DOM, so that it works together with JavaScript (and finding out how
to do). In this tests, my patch worked well.
I think I've experience at C++ programming and can implement the MathML-DOM,
but (at the moment) I don't know much neither about Mozilla's internal
structure nor about the build system, both topics I had to work on for this
patch. I just tried until it worked, so please may anyone who knows about that
look at the patch and tell me, if and what I did wrong?

Comment 7

[Anne (:annevk)]

I am certainly not the person who can review this, though I think you could make
it much more useable if you make it a real patch and not a downloadable file.

Comment 8

[Daniel Kraft]

Attachment: Uses nsMathMLAtoms

This is mostly the same as the privious patch (also tar-gzipped), but now
nsMathMLAtoms are used (and I moved them from /layout/ to /content/, so you may
delete those three files in /layout/mathml/content/src/!). Also a bit of
enhancement in implementation of nsIDOMMathMLElement, but as I wrote above this
isn't the topic I'm worried about.

Comment 9

[Alex Vincent [:WeirdAl]]

Please don't tar.gz or otherwise compress patches.  :) Interested parties want
to be able to read patches directly on Bugzilla without having to decompress them.

Comment 10

[Daniel Kraft]

Sorry, but I didn't find out how to make patches which create whole new
directories! If you know a solution, please let me know!

Comment 11

[Daniel Kraft]

Attachment: nsMathMLNodeList

Do you want me to request review when I've half finished, or for small groups
of interfaces implemented? In the latter case, of coure, the patches will not
be as big if the former were checked in after (super-)review. (And I won't have
to tar-gzip, because you could create mathml-subdirectories in the repository).

Comment 12

[Daniel Kraft]

Attachment: Some more enhancements

This implements the most important basic interfaces: MathMLElement,
MathMLNodeList, MathMLContainer and MathMLMathElement. I'd like to see this bug
closed with that (as nsMathMLElement is implemented) and move further
developement to bug 143842.
As you might have guessed, this is another tar (not gzipped this time for
readability). My problem is, that I'm not able to make my local CVS believe in
the new directories...

Comment 13

[Daniel Kraft]

Attachment: the same as real patch

Comment 14

[Daniel Kraft]

Comment on attachment 173152 [details] [diff] [review]
the same as real patch

Sorry rbs, I forgot some files in the diff

Comment 15

[Daniel Kraft]

Attachment: this one should be more complete

Comment 16

[Daniel Kraft]

Attachment: style fixes

Added missing changes in layout/mathml to the patch, and maily a lot of coding
style fixes. No code changed directly.

Comment 17

[Daniel Kraft]

rbs, what's about my patch?

Comment 18

[rbs]

Daniel, first let me apologize for the delay. I should have communicated what I had in mind. What I felt was that the benefit with respect to the added code size wasn't compelling enough at the time. This was particularly reinforced by the fact that people don't request/mention the MathML DOM at all in the newsgroups/forums related to MathML. Another reason is that I still have this inner hope that MathML rendering will become mainstream in small-devices, and therefore I feared that adding a big DOM code (unrequested by users as I indicated) might kill any chance for MathML getting considered for something like Minimo for example.

But I am now thinking that there is a possibility not to let this entire patch go to waste. We could try to get the nsMathMLlement bit, and perhaps have an #ifdef MOZ_MATHML_DOM for everything else so that the code can at least leave in the Mozilla tree. Are you in a position to iterate on your patch? Again, sorry for not communicating what I was thinking.

Comment 19

[Daniel Kraft]

Well, no problem.  As far as I remember is the patch not yet "that good written" and missing a lot of things, but I'd come back to this and maybe complete MathML DOM support - and of course I know that it is not that important and could therefore be made optional (#ifdef...).

However, I don't have the insight into Mozilla's build system to integrate such a configure option or something like that - for this I will need some help, I think.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I should note that without this or some equivalent (e.g. content sink support via nodeinfos?) it's impossible to support CSS classes for MathML.  That in itself would not need much MathML DOM, of course, just a simple subclass of nsXMLElement or nsGenericElement, with some basic overrides.

Comment 21

[rbs]

That is what the nsMathMLElement bit does (or should do), which is why I am suggesting to take that.

The rest of the patch (and the MathML DOM) is overkill (at present) and it is strategically harming the entire patch. We might have a directory structure that delineates the two, perhaps mathml/content, mathml/dom, and use an #ifdef MOZ_MATHML_DOM for the extra.

Comment 22

[Daniel Kraft]

With some help for the build system and separation part, I'd really like to look over this patch again and clean it up, so we can check in at least the basic part!

Comment 23

[rbs]

All you need for now is nsMathMLElement -- to be built by default without magic build config incanations. The rest is not urgent, and you can learn about it at your pace.

Comment 24

[Daniel Kraft]

OK, I will try to file an appropriate patch for this as soon as I'll find some time!

Comment 25

[Daniel Kraft]

Attachment: New, simple patch; nearly no DOM in now

It finally happend, I did rework this one.  Here's a new patch, implementing only nsMathMLElement, together with style/class-support.  Two points why I'm not yet requesting review:

1) I'm not sure whether I should completely take the DOM out of this patch and do just a style/class fix here?  So no nsIDOMMathMLElement or modification of nsDOMClassInfo?

2) I found myself duplicating some code from nsGenericHTMLElement/nsSVGElement -- is there really no better way, i.e., a generic, common base other than nsGenericElement/nsXMLElement like nsStyleableElement or so?

Expecting your comments on those two points and hope we can finally get somethin like this in.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> like nsStyleableElement or so?

One could be added, sure.... 

Comment 27

[Daniel Kraft]

Attachment: Testcase for style/id/class formatting of MathML

And added my test-case which should display all MathML-variables (a, b and c) green.  Without my patch, this works only for id (b), but applying the patch fixes it for all three.

Comment 28

[Daniel Kraft]

> One could be added, sure.... 
 
However, my point was if there's *already* such class I simply missed...  While I generally believe in code-sharing by inheritance, it would be over my responsibility here to add a generic super-class for nsGenericHTMLElement and nsSVGElement, wouldn't it?

Comment 29

[Boris Zbarsky [:bzbarsky]]

well... if you're willing to do it, and if jst/sicking/peterv like the idea, go for it.  If you'd rather not deal with it right now, that's fine too.

You're right that currently there is no such class.

Comment 30

[rbs]

Re: Comment #25

> It finally happend, I did rework this one.

OSS rocks.

> 1)

I am fine with what you have, since it is so little a sub, yet an useful entry point if needed later.

> 2)

I like the idea of an alternative patch with a mainline nsStylableElement as you mentioned -- the more shared code in mainline, the better/smaller the #ifdef MathML extras.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It sounds like it might work fine to break out some of the related code into a superclass. I take it it's code for IDs, classes and styles? The code would have to assume that the attributes are called 'id', 'class' and 'style' respectively, which in theory might not be the case for all styleable elements. However in reality it probably will be, so it sounds like a good idea to me.

Comment 32

[Daniel Kraft]

Yes, I'm thinking of nearly my nsMathMLElement-class here (the three DOM attributes taken out), as it currently only does work for class/style to work (id also works with plain nsXMLElement) -- the main points are Get/SetInlineStyleRule and a base-override of ParseAttribute.

I'd like to do this, and if it's done, we should do before nsMathMLElement, I think -- but this needs a new bug, doesn't it?

BTW, to get to the DOM:  What I did here is a half-way implementation of MathMLElement according to the spec, but it's still missing getOwnerMathElement() -- I believe we should implement this class fully or none, either adding this very method, too, or break all DOM out of it, maybe for a conditional build enabling it.

Comment 33

[Daniel Kraft]

Please give your comments on the common base in bug #379178, I'd like to get this implemented there.

Comment 34

[Vlad Sukhoy]

See bug 377499 for a related work on MathML DOM.

Comment 35

[Vlad Sukhoy]

Taking over the bug to avoid any confusion and work duplication, I'm working on this as a part of Summer 2007 MathML DOM project. WIP patches will still be posted onto bug 377499.

Comment 36

[Vlad Sukhoy]

Attachment: nsMathMLElement via nsStyledElement (no MathML DOM at all)

I am wondering who is the right reviewer for this?

Comment 37

[Vlad Sukhoy]

Comment on attachment 275478 [details] [diff] [review]
nsMathMLElement via nsStyledElement (no MathML DOM at all)

Ok, I think I figured.

Comment 38

[rbs]

Comment on attachment 275478 [details] [diff] [review]
nsMathMLElement via nsStyledElement (no MathML DOM at all)

r=rbs

Comment 39

[Vlad Sukhoy]

Comment on attachment 275478 [details] [diff] [review]
nsMathMLElement via nsStyledElement (no MathML DOM at all)

requesting 1.9 approval, see bug 355548 for details

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch has some bugs, like nsMathMLElement needs to QI to nsIDOMNode or things break. Please don't check it in. Bug 404406 has a new patch based on this one, let's use that instead.

Comment 41

[Vlad Sukhoy]

ok, waiting for improved patch from bug 404406 then.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 275478 [details] [diff] [review]
nsMathMLElement via nsStyledElement (no MathML DOM at all)

clearing approval, this patch is obsolete

Comment 43

[Vlad Sukhoy]

fixed by bug 404406 which was fixed by monster patch from bug 355548 done by roc which contained portions of attachment 275478 [details] [diff] [review].

Comment 44

[Vlad Sukhoy]

Comment on attachment 275478 [details] [diff] [review]
nsMathMLElement via nsStyledElement (no MathML DOM at all)

obsoleting the patch on this bug