Created attachment 165273 [details] Patch

This is one of the top 5 (IMHO) unimplemented features in WebKit MathML. :)

Comment on attachment 165273 [details] Patch Attachment 165273 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13993130

Comment on attachment 165273 [details] Patch Attachment 165273 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13994130

Comment on attachment 165273 [details] Patch Attachment 165273 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13984263

Comment on attachment 165273 [details] Patch Attachment 165273 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13990145

Created attachment 165275 [details] Patch

Sorry, I forgot to #if ENABLE(MATHML) in the first patch.

Comment on attachment 165273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165273&action=review I think this is interesting. It's unclear to me if we shouldn't just subclass HTMLTableElement instead? Or break out a TableElement subclass for this? How should the DOM elements look like for <mtd>. Does MathML have a specified JS DOM? > Source/WebCore/mathml/MathMLElement.cpp:68 > +void MathMLElement::parseAttribute(const Attribute& attribute) Should this be its own DOM element? MathMLMathTableCellElement maybe? > Source/WebCore/rendering/RenderTableCell.cpp:73 > + return isMathMLElement(node()) ? toMathMLElement(toElement(node()))->colSpan() : 1; Is the toElement needed here, doesn't toMathMlElement() handle Node*?

Comment on attachment 165275 [details] Patch Attachment 165275 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13993144

Comment on attachment 165273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165273&action=review It is interesting. MathML 2 had one short chapter and two large appendicies on a DOM, http://www.w3.org/TR/MathML2/appendixd.html and http://www.w3.org/TR/MathML2/appendixe.html, containing e.g. http://www.w3.org/TR/MathML2/appendixd.html#dom.TableCellElement for MathMLTableCellElement. These sections were dropped from MathML3 "with the intention of updating them" later (http://www.w3.org/TR/MathML3/appendixf.html) as XHTML faded into the night. :) Seriously, I don't know if they were ever implemented in Firefox for instance, or what's the best thing to do with them now, but I do know that the main editor of MathML 2 and 3, plus the main Firefox MathML developer, are both cc'd on this bug and will doubtless opine (I hope). For now, I guess we could try to match the beginning of their DOM, but it's a big project to start so I figured we could just implement it later, and let folks use getAttribute()/setAttribute() for now. (This made sense to me, but I am not a DOM expert.) I don't think we want mtable/mtr/mtd elements to be in subclasses of table/tr/td elements, since the MathML ones are xml not html elements. The renderers can be the same though, it seems. >> Source/WebCore/rendering/RenderTableCell.cpp:73 >> + return isMathMLElement(node()) ? toMathMLElement(toElement(node()))->colSpan() : 1; > > Is the toElement needed here, doesn't toMathMlElement() handle Node*? MathMLElement.h:55: inline MathMLElement* toMathMLElement(Element* element) could be changed to take a Node*. I'll do this.

There is some question in my mind if mtd should just be a direct alias for td. i.e. be exposed as an HTMLTableDataElement to the JS DOM. Unless we plan to expose everything as a MathMLElement (which is fine), then we will have to make some choices as to our DOM classes, and those have historically informed our C++ class hierarchy. Are there any ways in which td differs from mtd besides tagname? I guess what contexts its allowed by the parser/dom?

HTMLTableCellElement, rather.

In any case, the patch seems OK. Just trying to understand the larger design choices for mathML here. :)

> Does MathML have a specified JS DOM? As Dave indicated MathML2 did have a DOM specification as part of the main spec, however in the run up to MathML3 we sounded out a few implementors and basically MathML implementations were not DOM based (eg they were rendering via TeX or Mathematica or Maple or..) or they were DOM based but happy just to expose an XML DOM not a specific MathML DOM. So maintaining the MathML DOM in sync as the spec was updated was a lot of work for no visible users so it was basically dropped with some wording inserted that it could be resurrected as a separate spec if someone needed it. The incorporation of MathML into HTML5 was done basically outside the Math Working group, and I think really the way it is phrased there strongly indicates that all MathML elements should end up being MathMLElement (or subclasses of that) rather than HTMLElement, and be in the mathml namespace. There isn't a lot of difference other than default alignments and spacing and some surface syntax for adding rules and things between rows/columns between a MathML mtable and a HTML/css inline table. So hopefully you can share some code but never having looked at the webkit code I don't want to comment on that:-) With webkit's implementation becoming more complete, it means now practically speaking we have two DOM based implemntations, you and mozilla. If it would be helpful to resurrect a MathML DOM specification, we could certainly look to hosting that at W3C but it would need someone who knew what they were talking about to say what it should say: basically if you and Frédéric Wang think it would be worth standardising any aspects of the DOM interface to MathML we could standardise those aspects, but we wouldn't want to standardise things that were not common between the two implementations and thus declare one of them non-conforming.

In Gecko, the mtable implementation is based on the HTML table implementation, so the MathML mtd frame class inherits from the HTML td frame and similarly for other tabular elements. There are a few differences handled in the subclasses or not implemented yet in Gecko (like mlabeledtr). Regarding the content side, we only have one nsMathMLElement class for all MathML elements. IIUC, implementing the MathML DOM would mean adding a whole class hierarchy with one class for each MathML element. A student wanted to do this 6 years ago during a summer project but finally gave up. I'm not really sure that it's worth adding so much code to add this feature and moreover MathML can already be accessed and modified via the Core DOM.

OK. Thank you both for the background! CCing Ian so he can see this go by. I suspect his opinions are not especially strong in this area of MathML. I suspect that we'll end up like gecko, trying to re-use as much as possible from our HTML implementation. Whether we expose those details to the web or not is more a spec question. Since MathML is now part of HTML5 it would be very possible to spec interlacing of elements (like having a HTML table, or html table subclass inside a mathml dom), but I suspect we'll end up similar to what you've done -- sharing the rendering but not the DOM. We may still move to a model similar to HTMLImageElement/SVGImageElement, where we share the DOM and rendering implementation, but silently, not exposed to JS.

I think the basic question is whether MathML DOM elements should subclass HTMLElement <http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#htmlelement>. Can a web author use a title or dataset attribute on e.g. a <mtd> element? The MathML spec suggests that when MathML is in an HTML context, i.e. HTML5, then class and style (and presumably id?) attributes on MathML elements would typically use CSS, but "this is not specified by MathML [its spec]" <http://www.w3.org/TR/MathML3/chapter2.html#fund.globatt>. Also, <mtext> (and maybe other MathML "token elements") are encouraged to allow HTML content, but MathML doesn't specify details like whether it must be inline, etc. I can tell you that for instance in education, having input elements inside MathML, and styling MathML with CSS, are very big use cases. Similarly, it might be nice to allow title and dataset attributes. Perhaps this is ultimately a decision for the HTML5 committee, with input from the MathML committee and from browser implementors? Fred, WebKit also uses table/tr/td renderers (Gecko "frames"?) for mtable/mtr/mtd, and I'm guessing all HTML5 engines will want to. However, changing the DOM Element classes (not renderers) is pretty easy, at least in WebKit, and I'd guess in Gecko. If a committee decides to expose title/dataset/etc. on MathML elements, I think we just change MathMLElement to derive from HTMLElement instead of StyledElement. (StyledElement is what HTMLElement derives from, it's basically Element plus allows a style attribute.) Eric (& Fred), is there a significant performance cost to deriving from HTMLElement? Is there a reason not to let someone use title/dataset/etc. attributes if they want to? David C. (and Ian), is there some XML purity issue that I don't understand, or is the current state just because of historical evolution? I think after deciding the base class, we can come back to more specific classes like MathMLTableCellElement vs. HTMLTableCellElement. I'll also upload another patch. :)

Created attachment 165292 [details] Patch

Comment on attachment 165292 [details] Patch Attachment 165292 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13990402

MathML tables look like HTML tables, but in general does it really make sense to consider MathMLelement a subclass of HTMLelement? This is not the case with SVGElement, AFAIK. I think these languages serve different purposes and what you mention (title, dataset) are really just generic attributes like style or class, not something that would suggest that a MathML element is a kind of HTML element. In general, MathML and HTML elements in Gecko use ancestor C++ classes to share implementations, rather than making MathML elements subclasses of HTML elements. (Note: "title" works is Gecko but is nonstandard, the MathML spec suggests <maction actiontype="title"> instead)

(In reply to comment #18) > I think the basic question is whether MathML DOM elements should subclass HTMLElement <http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#htmlelement>. Can a web author use a title or dataset attribute on e.g. a <mtd> element? The MathML spec suggests that when MathML is in an HTML context, i.e. HTML5, then class and style (and presumably id?) attributes on MathML elements would typically use CSS, but "this is not specified by MathML [its spec]" <http://www.w3.org/TR/MathML3/chapter2.html#fund.globatt>. Also, <mtext> (and maybe other MathML "token elements") are encouraged to allow HTML content, but MathML doesn't specify details like whether it must be inline, etc. I can tell you that for instance in education, having input elements inside MathML, and styling MathML with CSS, are very big use cases. Similarly, it might be nice to allow title and dataset attributes. Perhaps this is ultimately a decision for the HTML5 committee, with input from the MathML committee and from browser implementors? > Speaking personally (not as Math WG chair) it's always been my view that as far as possible if mathml is hosted in another language (so (x)html here) any global attributes should be made to apply to mathml and similarly the content model of mtext etc should allow inline markup from the host. Basically if authors want to produce svg+mathml+html documents we should as far as possible make it look like a coherent language. But I'm mainly concerned with the markup level: how that relates to the DOM hierarchy others are much better to judge. the current state with respect to mathml in HTML5 is as far as I can tell is that mi, mo, mn, ms, mtext, allow html content but flow content (ie non-inline stuff) is not conforming, this is specified a bit backwards in the html5 spec (you need to look where for example div is allowed, which is "Where flow content is expected" and in particular this isn't mtext. However that is (as far as I can see, a conformance issue, from the implementation standpoint it's still supposed to do something sensible. On the other hand some html elements, notably <p>, abort the math expression at the parse stage so they won't be in mtext (er unless you construct the dom directly in script I suppose) The schema underlying http://validator.w3.org/nu/ currently doesn't allow global html attributes such as title and data-foo on mathml elements but as noted above personally I think it should/could. Things are a bit less specified than perhaps they ought to be as the mathml spec (as it needs to allow mathml to be embedded in docbook, ODF, TEI, ... as well as HTML basically says it is up to the host language to define the extension points but html5 apart from the parsing level which is specified in some detail, more or less punts all details to the mathml spec. > Eric (& Fred), is there a significant performance cost to deriving from HTMLElement? Is there a reason not to let someone use title/dataset/etc. attributes if they want to? David C. (and Ian), is there some XML purity issue that I don't understand, or is the current state just because of historical evolution? Mostly historical evolution I suspect. There is a MathML purity issue in that if mathml-in-html aquires so many html features that you can not easily extract the mathml into a pure mathml system then something has been lost, but I think you have to give up some things in order to make the resulting language coherent.

My understanding is the specs are unambiguous that MathML elements inherit from Element and not HTMLElement, but that's a matter for the MathML spec, not the HTML spec. The many global attributes in HTML only apply to HTML elements. Some attributes are supported more globally, e.g. id="", but those are defined in DOM Core, not HTML, and even then it's up to the various languages to define whether those attributes are _conforming_ or not, regardless of what they do. Similarly, the content models of MathML elements are to be defined by HTML. The "Contexts in which this element can be used" lines are explicitly non-normative. Nothing in HTML defines what is allowed in <mi>, that's entirely up to the MathML spec's definition of the content model of <mi>. The reverse (where <math> is allowed in HTML) is defined by the HTML specification by it putting the <math> element into various categories used in HTML's content model definitions. If MathML wants to allow HTML in MathML, it needs to do the equivalent.

(In reply to comment #23) > My understanding is the specs are unambiguous that MathML elements inherit from Element and not HTMLElement, but that's a matter for the MathML spec, not the HTML spec. > Yes that's my understanding too. [I'm not sure webkit's bug tracker is the best place to discuss spec coordination but since we're both here...] > The many global attributes in HTML only apply to HTML elements. Some attributes are supported more globally, e.g. id="", but those are defined in DOM Core, not HTML, and even then it's up to the various languages to define whether those attributes are _conforming_ or not, regardless of what they do. > > Similarly, the content models of MathML elements are to be defined by HTML. The "Contexts in which this element can be used" lines are explicitly non-normative. Nothing in HTML defines what is allowed in <mi>, that's entirely up to the MathML spec's definition of the content model of <mi>. The reverse (where <math> is allowed in HTML) is defined by the HTML specification by it putting the <math> element into various categories used in HTML's content model definitions. If MathML wants to allow HTML in MathML, it needs to do the equivalent. Taken in isolation I think that's a reasonable approach to take, but in the case of MathML, if the reader follows the advice in the html spec of checking the the MathML spec for the content models of mathml elements, what they find is that it says that they just take text _but_ specifications defining compound formats that include mathml (such as docbook+mathml or html+mathml) may extend the content models of mathml elements to allow elements from the host language. In other words we tell them to read the html spec for details of html+mathml. The html spec (or failing that an html+mathml specific document) is in fact the more natural place to define this as the content models allowed for token elements are directly related to the parsing of those elements and that, for text/html is defined by html. It wouldn't make a lot of sense for mathml to say p is allowed in mtext given that the html spec says that p closes the surrounding math element. In practice what you want to happen (and what does happen in the validator.nu based validators) is that (apart from parsing weirdness for <p> and friends) any phrasing/inline html elements are conforming inside mathml token elements and any other html elements works as well as implementations can make them work, but are non conforming. Michael Smith, Henri Sivonen and I hashed out the details of the mathml schema integration for validator.nu. We should probably expose that somewhere as a specification rather than just as a relaxng schema embedded in the validator sources.

Comment on attachment 165292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165292&action=review > Source/WebCore/mathml/MathMLElement.cpp:58 > +} It's a pity that we re-implement the logic from HTMLTableCell here. That means that any work done on the HTML side will need to be redone on the MathML side. Also there will be differences or inconsistencies between the 2: e.g. HTML cells (for better or worse) cap their colspan / rowspan but MathML cells won't.

Comment on attachment 165292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165292&action=review >> Source/WebCore/mathml/MathMLElement.cpp:58 >> +} > > It's a pity that we re-implement the logic from HTMLTableCell here. That means that any work done on the HTML side will need to be redone on the MathML side. Also there will be differences or inconsistencies between the 2: e.g. HTML cells (for better or worse) cap their colspan / rowspan but MathML cells won't. Agreed. I suspect eventually we'll want a TableElement baseclass to share logic between HTML and MathML, but I also think it might be OK to duplicate logic here for now. Part of how we started the larger discussion on this bug was trying to understand how different (or not) HTML vs. MathML tables are, and if we should be trying to share code or not.

Thanks to everyone for all the great information. I think I finally understand the mtext/etc. "MathML token element" content rules now. Eric & Julien, <mtable> is used for matrices & vectors, and also rectangular diagrams of basically mappings between sets ("commutative diagrams" are common in pure mathematics). There are no sections or headings. There are extra alignment attributes and an equation numbering tag. Actually the alignment stuff makes this one of the most complicated areas of mathematical layout, in MathML or also TeX. If we're going to unify HTML and MathML elements, unfortunately table elements are probably not the best ones to start with. (http://www.w3.org/TR/MathML3/chapter3.html#presm.tabmat) Fred, as I understand it SVG chose a box model incompatible with CSS, so any mixing with the CSS box model is explicitly disallowed. MathML is very different, and allows CSS, including padding, borders, etc. I believe this is excellent, and will be much appreciated by web developers. I agree completely with David Carlisle that web authors will want a single seamless language. They will want access to global html attributes on MathML elements, such as "title" instead of <maction actiontype="title">. I realize that the specs now say MathML elements are XML elements and hence not HTML elements, for instance they have a different namespace. But is this best for the future? Don't the global html attributes apply to MathML elements as well as they do to most HTML elements?

Comment on attachment 165292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165292&action=review Well, I've certainly learned quite a bit in this thread. I think we're eventually going to want to share more code with HTMLTableElement, but I think the code duplication is OK for this first-pass implementation. (I don't think I necessarily understand all the contraints of MathMLTable yet well enough to decide what should or shouldn't be shared.) > Source/WebCore/rendering/RenderTableCell.cpp:77 > + if (UNLIKELY(!m_hasAssociatedTableCellElement)) { I worry some callers might use this to imply that the table cell was anonymous. We have a "table cell" element associated, it's just not an HTML table cell. colSpan() is actually surprisingly hot code. :)

m_hasAssociatedTableCellElement is private at least, and only used a few times. How about I rename it to m_hasHTMLTableCellElement? And do you want another review then, or should I just land it?

(In reply to comment #29) > m_hasAssociatedTableCellElement is private at least, and only used a few times. How about I rename it to m_hasHTMLTableCellElement? And do you want another review then, or should I just land it? That would be fine. I don't need to see the rename. Renaming it would make it easier to understand how it's now being used. :)

(In reply to comment #30) > (In reply to comment #29) > > m_hasAssociatedTableCellElement is private at least, and only used a few times. How about I rename it to m_hasHTMLTableCellElement? And do you want another review then, or should I just land it? > > That would be fine. I don't need to see the rename. Renaming it would make it easier to understand how it's now being used. :) I agree too and like the new name. m_hasAssociatedTableCellElement is used to share the colspan / rowspan information with the HTMLTableCellElement, thus saving several bytes per RenderTableCell.

FYI: colspan()/rowspan() show up hot in some of the profiles seen in dependent bugs of bug 92258. I suspect that this boolean flag and UNLIKELY branch are previous attempts to keep colspan/rowspan off the profile. Not entirely successful, unfortunately. :)

(In reply to comment #32) > FYI: colspan()/rowspan() show up hot in some of the profiles seen in dependent bugs of bug 92258. I suspect that this boolean flag and UNLIKELY branch are previous attempts to keep colspan/rowspan off the profile. Not entirely successful, unfortunately. :) I have seen the same functions show up too, but re-adding the caching didn't yield to enough improvements that I consider the caching needed. For now, it's better to keep the memory win (if needed, there is a way to store both colspan / rowspan without using more than 32 bits). This is such improvements that MathML will not get with the current code unfortunately. Dave, that shouldn't block you though.

Committed r129695: <http://trac.webkit.org/changeset/129695>

Looks like this change broke the Windows Debug build. Here's the relevant output: bug\build\source\webcore\rendering\RenderTableCell.cpp(107) : error C2121: '#' : invalid character : possibly the result of a macro expansion 8>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderTableCell.cpp(107) : error C2143: syntax error : missing ')' before 'if' 8>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderTableCell.cpp(107) : error C2121: '#' : invalid character : possibly the result of a macro expansion 8>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderTableCell.cpp(107) : error C2059: syntax error : ')' 8>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderTableCell.cpp(113) : error C2121: '#' : invalid character : possibly the result of a macro expansion 8>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderTableCell.cpp(113) : error C2143: syntax error : missing ')' before 'if' 8>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderTableCell.cpp(113) : error C2121: '#' : invalid character : possibly the result of a macro expansion 8>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\rendering\RenderTableCell.cpp(113) : error C2059: syntax error : ')' 8>Build log was saved at "file://C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\obj\WebCore\BuildLog.htm"

Nuts, sorry. I think I can fix it quickly. I'll try now. (I thought the win EWS was failing over the weekend for everything. Sorry, I'm new at this.)

(In reply to comment #36) > Nuts, sorry. I think I can fix it quickly. I'll try now. > > (I thought the win EWS was failing over the weekend for everything. Sorry, I'm new at this.) No worries! Thanks for working on it.

I have hopefully fixed it now in http://trac.webkit.org/changeset/129708 but I managed to not create a ChangeLog entry. Should I just add one now? Sorry, I thought I was doing it right, but I guess I rushed myself.

(In reply to comment #38) > I have hopefully fixed it now in http://trac.webkit.org/changeset/129708 but I managed to not create a ChangeLog entry. Should I just add one now? > > Sorry, I thought I was doing it right, but I guess I rushed myself. That fixed it! Thanks again. I guess don't worry about the Changelog at this point?

(In reply to comment #39) > I guess don't worry about the Changelog at this point? Right, makes sense. Thanks for all your help.