Implement MathML DOM

• Component: DOM
• Status: RESOLVED FIXED

[Brian Kardell]

This is a request to normalize the DOM APIs exposed to MathML as described in 
https://mathml-refresh.github.io/mathml-core/#dom-mathmlelement and  https://github.com/mathml-refresh/mathml/issues/83

Like other things in the platform it seeks to expose as `MathMLElement` instead of `Element` and have the same basic 'shape' as other elements.

There is a lot of related IDL work/sorting out of mixins happening including the addition of `HTMLOrSVGElement` to the HTML specification last year (https://github.com/whatwg/html/pull/3543).  Conceptually, we use the same mixin and we'd like to rename it `HTMLOrForeignElement` instead before it gets implemented everywhere.


The following web platform tests were added tentatively for testing the desired normalization of MathML elements

* https://wpt.fyi/interop/mathml/relations/html5-tree/clipboard-event-handlers.tentative.html?label=master&label=experimental

* https://wpt.fyi/results/mathml/relations/html5-tree/css-inline-style-interface.tentative.html?label=master&label=experimental

* https://wpt.fyi/results/mathml/relations/html5-tree/html-or-foreign-element-interfaces.tentative.html?label=master&label=experimental

* https://wpt.fyi/results/mathml/relations/html5-tree/math-global-event-handlers.tentative.html?label=master&label=experimental


Similar bugs are open for firefox and chromium.  From the linked issue it seems we have agreement.

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

Created attachment 375710 [details]
Patch (WIP)

[Rob Buis]

Created attachment 377337 [details]
Patch

[Rob Buis]

Created attachment 377434 [details]
Patch

[Rob Buis]

Created attachment 377435 [details]
Patch

[Rob Buis]

Created attachment 377438 [details]
Patch

[Rob Buis]

Created attachment 377439 [details]
Patch

[Rob Buis]

Created attachment 377440 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 377440 [details]
Patch

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

> Source/WebCore/html/HTMLOrForeignElement.idl:30
> +] interface HTMLOrForeignElement {

Let's not add this and MathMLElement in a single patch.
There is a lot of nuance with regards to supporting tabIndex in MathML.

> Source/WebCore/html/HTMLOrForeignElement.idl:31
> +  readonly attribute DOMStringMap dataset; // FIXME: Should be [SameObject].

Nit: Wrong indentation. Always use four spaces.

[Brian Kardell]

(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 377440 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377440&action=review
> 
> > Source/WebCore/html/HTMLOrForeignElement.idl:30
> > +] interface HTMLOrForeignElement {
> 
> Let's not add this and MathMLElement in a single patch.
> There is a lot of nuance with regards to supporting tabIndex in MathML.
> 
> > Source/WebCore/html/HTMLOrForeignElement.idl:31
> > +  readonly attribute DOMStringMap dataset; // FIXME: Should be [SameObject].
> 
> Nit: Wrong indentation. Always use four spaces.

@rniwa is there something you can point to re: nuance for supporting tabIndex here?  I don't entirely understand what it is? Is it captured in any of the focus metabug whatwg issues or something and I missed it?  It seems good if we could link to something because I don't think it's just me, it seems it's not common knowledge?

[Ryosuke Niwa]

(In reply to Brian Kardell from comment #9)
>
> @rniwa is there something you can point to re: nuance for supporting
> tabIndex here?  I don't entirely understand what it is? Is it captured in
> any of the focus metabug whatwg issues or something and I missed it?  It
> seems good if we could link to something because I don't think it's just me,
> it seems it's not common knowledge?

https://github.com/whatwg/html/issues/4607

Defining what's focusable, how it can be focused, and how it interacts with sequential navigation is hard. Implementing tabIndex in MathML would involve testing all kinds of focusing scenarios including but not limited to focus via Element.focus(), focus via clicking (user initiated click), and focus via keyboard. We also need to test how it interacts with non-MathML focusable elements as well. Writing all those tests alone would be quite a bit of work.

[Rob Buis]

(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 377440 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377440&action=review
> 
> > Source/WebCore/html/HTMLOrForeignElement.idl:30
> > +] interface HTMLOrForeignElement {
> 
> Let's not add this and MathMLElement in a single patch.
> There is a lot of nuance with regards to supporting tabIndex in MathML.

Good idea, I split out the patch for HTMLOrForeignElement, removing the MathML parts, here:
https://bugs.webkit.org/show_bug.cgi?id=201219

> > Source/WebCore/html/HTMLOrForeignElement.idl:31
> > +  readonly attribute DOMStringMap dataset; // FIXME: Should be [SameObject].
> 
> Nit: Wrong indentation. Always use four spaces.

Fixed.

[Rob Buis]

Created attachment 377879 [details]
Patch

[Rob Buis]

Created attachment 377894 [details]
Patch

[Rob Buis]

Note that I am happy to split the tabIndex (functionality + tests) part from the MathML DOM part into a second patch, if that makes reviewing easier.

[Ryosuke Niwa]

Comment on attachment 377894 [details]
Patch

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

> Source/WebCore/html/HTMLOrForeignElement.idl:31
> +  readonly attribute DOMStringMap dataset; // FIXME: Should be [SameObject].

Nit: Wrong indentation.
Use exactly 4 spaces.

> Source/WebCore/mathml/MathMLElement.cpp:89
> -    } else if (name == rowspanAttr) {
> +        return;
> +    }
> +
> +    if (name == rowspanAttr) {

Why are we making this change? Can we avoid making unnecessary code changes?

> Source/WebCore/mathml/MathMLElement.idl:30
> +MathMLElement implements ElementCSSInlineStyle;

We need a test to ensure when inline style is updated, the computed style & its rendering also gets updated.
Looks like imported/w3c/web-platform-tests/mathml/relations/html5-tree/css-inline-style-interface.tentative.html
tests compute style but there doesn't seem to be any test for the actual rendering.

> Source/WebCore/mathml/MathMLElement.idl:33
> +MathMLElement implements HTMLOrForeignElement;

We need a test for mouse focusing using eventSender.

> LayoutTests/imported/w3c/ChangeLog:10
> +        * web-platform-tests/mathml/relations/html5-tree/clipboard-event-handlers.tentative-expected.txt: Added.

Are these imported tests? If so, please state at which git hash you're importing them for the record.

> LayoutTests/mathml/tabindex-order-expected.txt:10
> +id: mqrt tabindex: 1 msqrt is focused.

Please make it so that this is "id: d" so that all ID's increase lexicologically by one letter from "a" to "y".

> LayoutTests/mathml/tabindex-order-expected.txt:53
> +id: mqrt tabindex: 1 msqrt is focused.

Ditto.

> LayoutTests/mathml/tabindex-order.html:1
> +<html>

DOCTYPE?

> LayoutTests/mathml/tabindex-order.html:26
> +            for (var i = 0; i < rects.length; ++i) {

Nit: Not curly braces around a single like statement.
Also, "rect" isn't the right term to use here. elements?
Finally, why not just `for (const element of elements)`

> LayoutTests/mathml/tabindex-order.html:33
> +            if (window.testRunner) {

Ditto about curly braces.

> LayoutTests/mathml/tabindex-order.html:39
> +            var rects = document.getElementsByClassName('tab');

ditto about the variable name.

> LayoutTests/mathml/tabindex-order.html:48
> +            for (var i = 0; i < rects.length; ++i) {

Why not `for (const element of elements)`?

> LayoutTests/mathml/tabindex-order.html:57
> +            for (var i = 0; i < rects.length; ++i) {

Ditto.

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

Comment on attachment 377894 [details]
Patch

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

>> Source/WebCore/mathml/MathMLElement.cpp:89
>> +    if (name == rowspanAttr) {
> 
> Why are we making this change? Can we avoid making unnecessary code changes?

Sorry, I think this one is from my original patch. IIRC, I tried to be consistent with HTML/SVG where early returns are used instead of a sequence of else if. This is important since we now do a StyledElement::parseAttribute(name, value) call at the end of the function, so we don't want to do that if the attribute is already handled here. However, I think this refactoring can be done in a separate/preliminary patch.

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #16)
> Comment on attachment 377894 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377894&action=review
> 
> >> Source/WebCore/mathml/MathMLElement.cpp:89
> >> +    if (name == rowspanAttr) {
> > 
> > Why are we making this change? Can we avoid making unnecessary code changes?
> 
> Sorry, I think this one is from my original patch. IIRC, I tried to be
> consistent with HTML/SVG where early returns are used instead of a sequence
> of else if. This is important since we now do a
> StyledElement::parseAttribute(name, value) call at the end of the function,
> so we don't want to do that if the attribute is already handled here.
> However, I think this refactoring can be done in a separate/preliminary
> patch.

Yeah, let's make that change in a separate patch given this is already 100KB...

[Rob Buis]

Created attachment 377979 [details]
Patch

[Rob Buis]

Created attachment 377991 [details]
Patch

[Rob Buis]

Comment on attachment 377894 [details]
Patch

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

>> Source/WebCore/html/HTMLOrForeignElement.idl:31
>> +  readonly attribute DOMStringMap dataset; // FIXME: Should be [SameObject].
> 
> Nit: Wrong indentation.
> Use exactly 4 spaces.

Fixed.

>>>> Source/WebCore/mathml/MathMLElement.cpp:89
>>>> +    if (name == rowspanAttr) {
>>> 
>>> Why are we making this change? Can we avoid making unnecessary code changes?
>> 
>> Sorry, I think this one is from my original patch. IIRC, I tried to be consistent with HTML/SVG where early returns are used instead of a sequence of else if. This is important since we now do a StyledElement::parseAttribute(name, value) call at the end of the function, so we don't want to do that if the attribute is already handled here. However, I think this refactoring can be done in a separate/preliminary patch.
> 
> Yeah, let's make that change in a separate patch given this is already 100KB...

Removed those changes.

>> Source/WebCore/mathml/MathMLElement.idl:30
>> +MathMLElement implements ElementCSSInlineStyle;
> 
> We need a test to ensure when inline style is updated, the computed style & its rendering also gets updated.
> Looks like imported/w3c/web-platform-tests/mathml/relations/html5-tree/css-inline-style-interface.tentative.html
> tests compute style but there doesn't seem to be any test for the actual rendering.

I added css-inline-style-dynamic.tentative.html (through WPT import).

>> Source/WebCore/mathml/MathMLElement.idl:33
>> +MathMLElement implements HTMLOrForeignElement;
> 
> We need a test for mouse focusing using eventSender.

I added focus-event-handling.html.

>> LayoutTests/imported/w3c/ChangeLog:10
>> +        * web-platform-tests/mathml/relations/html5-tree/clipboard-event-handlers.tentative-expected.txt: Added.
> 
> Are these imported tests? If so, please state at which git hash you're importing them for the record.

Yes, I now did a sync of mathml/relations/html5-tree/, and included the git hash for that.

>> LayoutTests/mathml/tabindex-order-expected.txt:10
>> +id: mqrt tabindex: 1 msqrt is focused.
> 
> Please make it so that this is "id: d" so that all ID's increase lexicologically by one letter from "a" to "y".

Done.

>> LayoutTests/mathml/tabindex-order-expected.txt:53
>> +id: mqrt tabindex: 1 msqrt is focused.
> 
> Ditto.

Done.

>> LayoutTests/mathml/tabindex-order.html:1
>> +<html>
> 
> DOCTYPE?

Added.

>> LayoutTests/mathml/tabindex-order.html:26
>> +            for (var i = 0; i < rects.length; ++i) {
> 
> Nit: Not curly braces around a single like statement.
> Also, "rect" isn't the right term to use here. elements?
> Finally, why not just `for (const element of elements)`

Should be all done.

>> LayoutTests/mathml/tabindex-order.html:33
>> +            if (window.testRunner) {
> 
> Ditto about curly braces.

Fixed.

>> LayoutTests/mathml/tabindex-order.html:48
>> +            for (var i = 0; i < rects.length; ++i) {
> 
> Why not `for (const element of elements)`?

Done.

>> LayoutTests/mathml/tabindex-order.html:57
>> +            for (var i = 0; i < rects.length; ++i) {
> 
> Ditto.

Done.

[Ryosuke Niwa]

Comment on attachment 377991 [details]
Patch

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

> LayoutTests/mathml/focus-event-handling.html:22
> +<script src="focus-event-handling.js"></script>

Please inline the script here. Having a separate JS file is an old style we don't want to use in new tests.

> LayoutTests/mathml/focus-event-handling.js:46
> +    eventSender.mouseMoveTo(x, y);
> +    eventSender.mouseDown();
> +    eventSender.mouseUp();

Please use ui-helper.js and UIHelper.activateAt or UIHelper.activateElement
so that this test would run in iOS WebKit2.

[Rob Buis]

Created attachment 378049 [details]
Patch

[Rob Buis]

Created attachment 378076 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 378076 [details]
Patch

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

> LayoutTests/mathml/focus-event-handling.html:56
> +        spaceElement.addEventListener("focusin", focusinHandler, false);
> +        spaceElement.addEventListener("focusout", focusoutHandler, false);

Can we listen to focus & blur instead?
They're more commonly used than focusin & focusout.

It's interesting that focousout doesn't seem to fire on iOS though.
It's probably a bug somewhere.

[Rob Buis]

Created attachment 378098 [details]
Patch

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

Comment on attachment 378098 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:16532
> +				AACC83D92316578600EB6BF5 /* MATHML */,

nit: Why not "MathML"?

[Rob Buis]

(In reply to Ryosuke Niwa from comment #24)
> Comment on attachment 378076 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378076&action=review
> 
> > LayoutTests/mathml/focus-event-handling.html:56
> > +        spaceElement.addEventListener("focusin", focusinHandler, false);
> > +        spaceElement.addEventListener("focusout", focusoutHandler, false);
> 
> Can we listen to focus & blur instead?
> They're more commonly used than focusin & focusout.

Done.

> It's interesting that focousout doesn't seem to fire on iOS though.
> It's probably a bug somewhere.

It seems the same problem is still there. Seems some other element gets the focus before the MathML elements? Unfortunately my iOS simulator build is broken so far on Catalina beta, so hopefully Fred can have a look tomorrow.

[Rob Buis]

Created attachment 378179 [details]
Patch

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

(In reply to Rob Buis from comment #27)
> > It's interesting that focousout doesn't seem to fire on iOS though.
> > It's probably a bug somewhere.
> 
> It seems the same problem is still there. Seems some other element gets the
> focus before the MathML elements? Unfortunately my iOS simulator build is
> broken so far on Catalina beta, so hopefully Fred can have a look tomorrow.

OK, I checked that this morning and indeed it does not seem an issue with the test but with the implementation. Since SVG also has a similar test failure with blur on iOS, I guess we can skip that for MathML too and handle this issue in a follow-up bug.

[WebKit Commit Bot]

Comment on attachment 378179 [details]
Patch

Clearing flags on attachment: 378179

Committed r249572: <https://trac.webkit.org/changeset/249572>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55109156>