Bug 200470

Summary: Implement MathML DOM
Product: WebKit Reporter: Brian Kardell <bkardell>
Component: DOMAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fred.wang, graouts, koivisto, rbuis, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugzilla.mozilla.org/show_bug.cgi?id=1571487
Bug Depends on: 201219    
Bug Blocks: 195797    
Attachments:
Description Flags
Patch (WIP)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Brian Kardell 2019-08-06 10:12:28 PDT
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.
Comment 1 Frédéric Wang (:fredw) 2019-08-07 10:32:56 PDT
Created attachment 375710 [details]
Patch (WIP)
Comment 2 Rob Buis 2019-08-27 05:48:28 PDT
Created attachment 377337 [details]
Patch
Comment 3 Rob Buis 2019-08-28 00:11:49 PDT
Created attachment 377434 [details]
Patch
Comment 4 Rob Buis 2019-08-28 02:08:51 PDT
Created attachment 377435 [details]
Patch
Comment 5 Rob Buis 2019-08-28 02:38:15 PDT
Created attachment 377438 [details]
Patch
Comment 6 Rob Buis 2019-08-28 05:18:49 PDT
Created attachment 377439 [details]
Patch
Comment 7 Rob Buis 2019-08-28 06:39:40 PDT
Created attachment 377440 [details]
Patch
Comment 8 Ryosuke Niwa 2019-08-28 07:50:10 PDT
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.
Comment 9 Brian Kardell 2019-08-28 08:35:48 PDT
(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?
Comment 10 Ryosuke Niwa 2019-08-28 08:49:37 PDT
(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.
Comment 11 Rob Buis 2019-08-28 10:39:07 PDT
(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.
Comment 12 Rob Buis 2019-09-03 02:59:23 PDT
Created attachment 377879 [details]
Patch
Comment 13 Rob Buis 2019-09-03 07:10:24 PDT
Created attachment 377894 [details]
Patch
Comment 14 Rob Buis 2019-09-03 11:13:07 PDT
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.
Comment 15 Ryosuke Niwa 2019-09-03 13:57:04 PDT
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.
Comment 16 Frédéric Wang (:fredw) 2019-09-03 22:49:50 PDT
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.
Comment 17 Ryosuke Niwa 2019-09-03 23:01:32 PDT
(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...
Comment 18 Rob Buis 2019-09-04 07:10:20 PDT
Created attachment 377979 [details]
Patch
Comment 19 Rob Buis 2019-09-04 11:38:10 PDT
Created attachment 377991 [details]
Patch
Comment 20 Rob Buis 2019-09-04 13:25:05 PDT
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.
Comment 21 Ryosuke Niwa 2019-09-04 13:41:10 PDT
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.
Comment 22 Rob Buis 2019-09-04 23:35:09 PDT
Created attachment 378049 [details]
Patch
Comment 23 Rob Buis 2019-09-05 05:53:09 PDT
Created attachment 378076 [details]
Patch
Comment 24 Ryosuke Niwa 2019-09-05 10:14:34 PDT
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.
Comment 25 Rob Buis 2019-09-05 10:58:47 PDT
Created attachment 378098 [details]
Patch
Comment 26 Frédéric Wang (:fredw) 2019-09-05 11:25:41 PDT
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"?
Comment 27 Rob Buis 2019-09-05 13:12:50 PDT
(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.
Comment 28 Rob Buis 2019-09-06 03:07:34 PDT
Created attachment 378179 [details]
Patch
Comment 29 Frédéric Wang (:fredw) 2019-09-06 03:10:42 PDT
(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.
Comment 30 WebKit Commit Bot 2019-09-06 05:28:00 PDT
Comment on attachment 378179 [details]
Patch

Clearing flags on attachment: 378179

Committed r249572: <https://trac.webkit.org/changeset/249572>
Comment 31 WebKit Commit Bot 2019-09-06 05:28:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Radar WebKit Bug Importer 2019-09-06 05:29:21 PDT
<rdar://problem/55109156>