MathML elements as nsMathMLElement

RESOLVED FIXED

Status

()

Core
MathML
--
minor
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: Daniel Kraft, Assigned: Vlad Sukhoy)

Tracking

(Blocks: 1 bug)

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

13 years ago
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]".
(Reporter)

Comment 1

13 years ago
Created attachment 169743 [details]
Testcase

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

Updated

13 years ago
Blocks: 276028

Updated

13 years ago
Assignee: firefox → rbs
Component: General → MathML
Product: Firefox → Core
QA Contact: firefox.general → ian
Version: unspecified → Other Branch

Comment 2

13 years ago
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.
(Reporter)

Comment 3

13 years ago
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

13 years ago
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.
(Reporter)

Comment 5

13 years ago
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.
(Reporter)

Comment 6

13 years ago
Created attachment 170042 [details]
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

13 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 8

13 years ago
Created attachment 170073 [details]
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.
Attachment #170042 - Attachment is obsolete: true
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.
(Reporter)

Comment 10

13 years ago
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!
(Reporter)

Comment 11

13 years ago
Created attachment 170741 [details]
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).
Attachment #170073 - Attachment is obsolete: true
Attachment #170741 - Flags: review?(rbs)
(Reporter)

Comment 12

13 years ago
Created attachment 171915 [details] [diff] [review]
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...
Attachment #170741 - Attachment is obsolete: true
Attachment #171915 - Flags: review?(rbs)
(Reporter)

Updated

13 years ago
Attachment #170741 - Flags: review?(rbs)
(Reporter)

Updated

13 years ago
Attachment #171915 - Attachment is patch: true
Attachment #171915 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 13

13 years ago
Created attachment 173152 [details] [diff] [review]
the same as real patch
Attachment #171915 - Attachment is obsolete: true
Attachment #173152 - Flags: review?(rbs)
(Reporter)

Updated

13 years ago
Attachment #171915 - Flags: review?(rbs)
(Reporter)

Comment 14

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

Sorry rbs, I forgot some files in the diff
Attachment #173152 - Flags: review?(rbs)
(Reporter)

Comment 15

13 years ago
Created attachment 173154 [details] [diff] [review]
this one should be more complete
(Reporter)

Updated

13 years ago
Attachment #173152 - Attachment is obsolete: true
Attachment #173154 - Flags: review?(rbs)
(Reporter)

Comment 16

13 years ago
Created attachment 173520 [details] [diff] [review]
style fixes

Added missing changes in layout/mathml to the patch, and maily a lot of coding
style fixes. No code changed directly.
Attachment #173154 - Attachment is obsolete: true
Attachment #173520 - Flags: review?(rbs)
(Reporter)

Updated

13 years ago
Attachment #173154 - Flags: review?(rbs)
(Reporter)

Comment 17

13 years ago
rbs, what's about my patch?

Comment 18

11 years ago
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.
(Reporter)

Comment 19

11 years ago
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.
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

11 years ago
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.
(Reporter)

Comment 22

11 years ago
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

11 years ago
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.
(Reporter)

Comment 24

11 years ago
OK, I will try to file an appropriate patch for this as soon as I'll find some time!
(Reporter)

Comment 25

11 years ago
Created attachment 262925 [details] [diff] [review]
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.
Attachment #173520 - Attachment is obsolete: true
Attachment #173520 - Flags: review?(rbs)
> like nsStyleableElement or so?

One could be added, sure.... 
(Reporter)

Comment 27

11 years ago
Created attachment 262926 [details]
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.
(Reporter)

Comment 28

11 years ago
> 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?
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

11 years ago
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.
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.
(Reporter)

Comment 32

11 years ago
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.
(Reporter)

Comment 33

11 years ago
Please give your comments on the common base in bug #379178, I'd like to get this implemented there.
Depends on: 379178
(Assignee)

Comment 34

11 years ago
See bug 377499 for a related work on MathML DOM.
(Assignee)

Comment 35

11 years ago
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.
Assignee: rbs → vladimir.sukhoy
Blocks: 377499
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 36

10 years ago
Created attachment 275478 [details] [diff] [review]
nsMathMLElement via nsStyledElement (no MathML DOM at all)

I am wondering who is the right reviewer for this?
(Assignee)

Comment 37

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

Ok, I think I figured.
Attachment #275478 - Flags: review?(rbs)
Attachment #275478 - Flags: superreview?(jonas)

Comment 38

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

r=rbs
Attachment #275478 - Flags: review?(rbs) → review+
Attachment #275478 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 39

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

requesting 1.9 approval, see bug 355548 for details
Attachment #275478 - Flags: approval1.9?

Updated

10 years ago
Attachment #275478 - Flags: approval1.9? → approval1.9+
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.
(Assignee)

Comment 41

10 years ago
ok, waiting for improved patch from bug 404406 then.
Comment on attachment 275478 [details] [diff] [review]
nsMathMLElement via nsStyledElement (no MathML DOM at all)

clearing approval, this patch is obsolete
Attachment #275478 - Flags: approval1.9+
Depends on: 404406
(Assignee)

Comment 43

10 years ago
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].
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 44

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

obsoleting the patch on this bug
Attachment #275478 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.