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

Proposal for MathML base class #858

Merged
merged 8 commits into from
Oct 28, 2024
Merged

Proposal for MathML base class #858

merged 8 commits into from
Oct 28, 2024

Conversation

Quafadas
Copy link
Contributor

  • run sbt prePR [x]
  • commit changes to api-reports [x]

This proposes a base class for MathML elements.

If it gets merged / agreed, I'd propose to follow it up with concrete elements following the style of the SVG elements, and the documentation here;

https://developer.mozilla.org/en-US/docs/Web/MathML

Related;

cc @raquo

@Quafadas Quafadas changed the title Proposal for mathml base class Proposal for MathML base class Sep 22, 2024
Copy link
Contributor

@raquo raquo left a comment

Choose a reason for hiding this comment

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

Thanks!

A couple questions, perhaps more to the scalajs-dom maintainers:

MDN says the style property is defined for both HTML, SVG, and MathML elements, but in scalajs-dom interfaces, it's only defined for HTML elements. Unless there's a known reason for that, I think it should be defined for SVG (and now MathML) elements as well.

I'm also wondering why event props like onclick / onmouseover / etc. are defined in HTML and SVG element classes in scalajs-dom – MDN does not mention anything HTML or SVG specific about most of those events (example). I guess it's ok for MathML to follow this pattern for now, but shouldn't most of those event properties (that apply to all elements) be moved into the parent Element class?

dom/src/main/scala/org/scalajs/dom/MathMLElement.scala Outdated Show resolved Hide resolved
dom/src/main/scala/org/scalajs/dom/MathMLElement.scala Outdated Show resolved Hide resolved
@raquo raquo mentioned this pull request Sep 24, 2024
10 tasks
@Quafadas
Copy link
Contributor Author

I've updated the two direct pieces of feedback - thankyou 🙏

@Quafadas
Copy link
Contributor Author

Quafadas commented Oct 1, 2024

Just checking in - anything else I could do to nudge this forward?

@zetashift
Copy link
Contributor

zetashift commented Oct 1, 2024

but in scalajs-dom interfaces, it's only defined for HTML elements. Unless there's a known reason for that, I think it should be defined for SVG (and now MathML) elements as well.

I do not know why it has been done like that, I'm not against this change however.

As for this PR, thank you! TIL about the MathML spec.
I am wondering how faithful this implementation is to the MathML spec: https://w3c.github.io/mathml-core/#the-top-level-math-element, shouldn't this at least have the global attributes in the class: https://w3c.github.io/mathml-core/#global-attributes ?

@Quafadas
Copy link
Contributor Author

Quafadas commented Oct 1, 2024

I am wondering how faithful this implementation is to the MathML spec: https://w3c.github.io/mathml-core/#the-top-level-math-element, shouldn't this at least have the global attributes in the class: https://w3c.github.io/mathml-core/#global-attributes ?

I think you are absolutely right. I'll try to add it. I think I got the on* properties through sheer luck parroting the SVG element. Thanks for the pointer, BRB...

@Quafadas
Copy link
Contributor Author

Quafadas commented Oct 1, 2024

@zetashift done I believe. Thanks for the pointer 🙏

@Quafadas
Copy link
Contributor Author

Just checking in? Anything else I can do?

@zetashift
Copy link
Contributor

Sorry! This looks good to me!

@Quafadas
Copy link
Contributor Author

Quafadas commented Oct 26, 2024

@raquo I assume, that I also need to add the concrete mathML instances, before we can take advantage of it downstream? I'm planning to do that in a separate PR to keep this one small...

@raquo
Copy link
Contributor

raquo commented Oct 26, 2024

@Quafadas Well, there are levels to this.

If we just have a single MathMLElement class that is shared by all MathML elements in the DOM, then in Laminar we can still type all MathML elements as instances of that one class, ignoring the subclasses. It will work, but you wouldn't be able to read any subclass-specific properties on such instances using el.ref.<propName>. You would still be able to set those properties via Laminar's usual syntax though, if you define those properties in Scala DOM Types.

FTR, I don't actually know how many of such subclass-specific properties MathML elements have.

@Quafadas
Copy link
Contributor Author

Quafadas commented Oct 26, 2024

@raquo Thanks for the note.

I think what I'm proposing is that once this gets merged, I'll follow it straight up with another PR that puts up a complete list of the concrete elements.

Based on what you've said II think my proposal would be to be quite diligent on the top level math element, but I would prefer to avoid a big bike shedding exercise on the correctness of the sub-elements (that are rather niche). If they turned out to have meaningful properties, it should be easier to add them at a later date...

Does that sounds sensible?

@raquo
Copy link
Contributor

raquo commented Oct 27, 2024

@Quafadas Thanks, sounds good to me, just keep in mind that you want to look at the MathMLElement superclass that is common to all MathML elements. The outer <math> element will likely have its own subclass of MathMLElement (not sure the exact name), so it may have properties on it that are not available on other types of MathML elements.

From what I see on MDN, it seems that the root <math> element only has display as the additional attribute. I assume it also reflects as a property, so should be present in the subclass definition, but I'm not sure.

@Quafadas
Copy link
Contributor Author

@zetashift Is there a remaining barrier to merging?

@zetashift zetashift merged commit 69fdc08 into scala-js:main Oct 28, 2024
5 checks passed
@zetashift
Copy link
Contributor

Thank you!

@Quafadas
Copy link
Contributor Author

@zetashift Mostly thank you in fact :-)!

@Quafadas Quafadas mentioned this pull request Nov 10, 2024
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.

3 participants