Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tidy Mathml element #869

Merged
merged 6 commits into from
Nov 30, 2024
Merged

Tidy Mathml element #869

merged 6 commits into from
Nov 30, 2024

Conversation

Quafadas
Copy link
Contributor

@Quafadas Quafadas commented Nov 9, 2024

This is a follow up to #858 which added the base class for MathML elements.

The three elements added so far intend to make it "easy" to to check that I'm following the right sort of pattern as I haven't done this before - and then I'd add the remainder.

@Quafadas Quafadas changed the title WIP: Conrete Mathml elements WIP: Concrete Mathml elements Nov 10, 2024
@Quafadas
Copy link
Contributor Author

@zetashift Sorry to bother you again, this would be the follow up PR to the base element. I've had a double check, and I can't see that I would do anything differently? Do you agree with adding the remaining elements?

@Quafadas
Copy link
Contributor Author

@raquo - Does this look more or less the way you would expect? If so I'll continue on filling in the blanks...

@raquo
Copy link
Contributor

raquo commented Nov 16, 2024

Hm, well, TBH this is indeed what I expected, but upon further research it appears that my expectations were incorrect.

For some reason, MathMLMathElement does not appear to be a real JS class that exists. It does not exist in my Firefox or Chrome. If I open the browser console and enter MathMLElement (the common class), it prints out a reference to the corresponding JS class. However, if I do the same with MathMLMathElement, I get MathMLMathElement is not defined. Same for all other subtypes of MathMLElement that I tried.

That means that MathMLMathElement can not be a @JSGlobal class in Scala.js – as the corresponding JS class does not exist. I see this MathMLMathElement type defined in the spec here – they call it an "Interface", similar to the generic MathMLElement type (here), but only the latter seems to exist as a class in the JS DOM.

I'm not sure why it's like this. In contrast, with HTML and SVG, the browsers expose the various elements' interfaces as subclasses. For example, a element is observably a SVGCircleElement.

Perhaps the browsers don't care enough about MathML to expose a typed interface to it. Whatever the reason, without JS types there's not much we can do about it in the scalajs-dom, as this repo is only supposed to reflect what exists in JS.

On the plus side, if all we want is the ability to build MathML elements using Laminar (or other such libraries), we shouldn't need the help of JS types. We just need to define all the MathML attributes in a new trait in Scala DOM Types – similarly to how SVG attributes are defined there. In practice, Laminar will let you apply any of those MathML attributes to any MathML element anyway, the only benefit of having precise types in scalajs-dom would be to read them via element.ref – but thankfully that is only a secondary use case for Laminar users, because if JS does not expose said precise types, we're out of luck on that.

The methods that Laminar does use internally are already defined on the common Element type, so I think now that we have the common MathMLElement type (that does exist in JS), we should be covered as far as scalajs-dom interfaces go.

@sjrd
Copy link
Member

sjrd commented Nov 16, 2024

Specified interfaces can still be defined in scalajs-dom as traits.

@raquo
Copy link
Contributor

raquo commented Nov 16, 2024

Ah yes, good point. But... doesn't this mean that the MathMLElement sypertype would need to be a trait as well, instead of an abstract class? But if it's also a trait, then we won't be able to type-test against it at runtime element.isinstanceOf(MathMLElement), and we should be able to do so. And the Element type up the hierarchy is also an abstract class, so we're not getting past that wall directly anyway.

Since we don't actually have user-accessible constructors for any of those types, I guess we could mix-in types like this: MathMLMathElement with MathMLElement, but that seems rather ugly.

But, that brings me to another point that I forgot to mention, that could make all this moot. The spec of MathMLMathElement says it should have two attributes – macros and display, both strings. However, testing in JS, those do not exist as properties on the element instance. They return undefined (not even null), and so, they must be only available as actual attributes that you need to read and write with get/setAttribute{NS?} methods.

That's not unprecedented – those are simply un-reflected attributes. But the browsers also don't seem to implement the methods that the spec promises, e.g. the spec says elements should have a getAnnotation method, but it's also undefined.

I'm now inclined to believe that the browsers implement only a subset of MathML spec, and the reason they haven't exposed other subtypes of MathMLElement, is that they did not implement any distinguishing features of those subtypes. I haven't exhaustively tested this, but a few attributes and methods that I tried are all absent from the real JS instances.


Actually, this brings me to another problem that I've just realized. The MathMLElement definition that's already merged is not correct.

  • Some of the properties don't actually exist on real instances of MathMLElement in the browser, and should be removed: xmlbase, dir, displaystyle, mathbackground, mathcolor, mathsize, scriptlevel, intent, arg
  • Some of the properties are named incorrectly: class should be className, tabindex should be tabIndex
  • style property should have type CSSDeclaration (a JS class that we already have in scalajs-dom), not String.

I think the main reason for those mistakes is the confusion between attributes (what you see in the text-based html/markml markup) and properties of element instances available via JS. Those are not the same, and even if some of them are mirrored (reflected), their names don't always match.

@Quafadas Can you please adjust the MathMLElement class as per above? ^^^

The way I tested changes in the browser console is just by trying to read properties from an instance of a element. You can select the element from the rendered document in the browser's element tree, and in the browser console, type for example $0.tabindex to see that it returns undefined – i.e. there is no such property ($0 is a magic variable in browser dev tools that refers to the element selected in the element tree). $0.autofocus, for example, is defined, and has the right type, so that one can stay.

@sjrd
Copy link
Member

sjrd commented Nov 16, 2024

Ah yes, good point. But... doesn't this mean that the MathMLElement sypertype would need to be a trait as well, instead of an abstract class?

No, it doesn't. In Scala, a trait can extend a class. 🙂

@raquo
Copy link
Contributor

raquo commented Nov 16, 2024 via email

@Quafadas
Copy link
Contributor Author

TL:DR I'll set out to modify the base MathML class as suggested.

I'm confused though, on whether the remaining elements should be added at all? Are they traits? Should we simply have a MathMl base class and nothing else?


I also fired up the browser tools, with this very simple example to try and improve my own understanding, and left more confused. Yey.

<!DOCTYPE html>
<html lang="en-US">
  <head>
    <title>My first math page</title>
  </head>
  <body>
    <p>
      The fraction
      <math id="hello" display="block">
        <mfrac>
          <mn>1</mn>
          <mn>3</mn>
        </mfrac>
        <mroot>
          <mn>2</mn>
          <mn>3</mn>
        </mroot>
        <mtext> is a fraction.</mtext>
      </math>
      is not a decimal number.
    </p>
  </body>
</html>
</script>

I've followed the breadcrumbs on the display attribute and proven it does something, because it changes the way these elements are rendered if set / removed. However, I also see this in the browser console.

image

I guess, that I've been spoiled by scalaJS that I didn't realise, that could be undefined.

image

and then that this works as expected.
image

I'm feeling miffed, because before setting out , I did check;
https://caniuse.com/mathml

Where I understood it was being advertised as widely supported! MathML they said, it'll be fun they said.

Just to that I understand, in essence scala-js-dom is the facade to browser API's. So in the end what matters, is what the browsers do, and I agree, that the browsers do not seem to expose these API's on the elements. Am I correct in thinking, that if I wanted to leave the "display" attribute in , it could be implemented in terms of the getAttribute('display') and setAttribute("display", "block") methods? I proved to my satisfaction that did indeed alter the behaviour of the element.

Would that be unidiomatic / stupid for scala-js-dom? Even if sensible, I'm not sure if I'd do it as I guess it's a lot of work for little gain?

@raquo
Copy link
Contributor

raquo commented Nov 17, 2024

Yes, you're on the right track. We should just have MathMLElement base class, and forget about the traits, because there's nothing we could put in those traits anyway (since the attributes are not mirrored in the JS API, whether by spec, or by lack of implementation in the browsers). We can implement setting of mathml attributes using setAttribute with Scala DOM Types / Laminar (that's how we set SVG attributes in Laminar, for example).

I think this arrangement would be perfectly appropriate for both scalajs-dom and Laminar – in line with how we normally do this, and accounting for the limitations of the underlying JS API.

@Quafadas Quafadas changed the title WIP: Concrete Mathml elements WIP: Tidy Mathml element Nov 18, 2024
@Quafadas
Copy link
Contributor Author

I think this is it then. A greatly simplified MathMLElement... and that's it.

@Quafadas Quafadas changed the title WIP: Tidy Mathml element Tidy Mathml element Nov 18, 2024
@raquo
Copy link
Contributor

raquo commented Nov 22, 2024

Final version looks good to me 👍

@Quafadas
Copy link
Contributor Author

@raquo thanks for the help and guidance.

@Quafadas
Copy link
Contributor Author

@zetashift Sorry to bother... would there be any chance of merging?

Copy link
Contributor

@zetashift zetashift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@zetashift zetashift merged commit f86bae5 into scala-js:main Nov 30, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants