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

Add MathMLElement interface and mixins ElementCSSInlineStyle, HTMLOrForeignElement #4143

Merged
merged 7 commits into from
Oct 15, 2024

Conversation

Philipp-M
Copy link
Contributor

This aims to add the MathMLElement interface, from https://searchfox.org/mozilla-central/source/dom/webidl/MathMLElement.webidl

I noticed it used mixins that were not there yet, which are also used for the interfaces SVGElement and HTMLElement, I've added them as well and removed the redundant attributes from interfaces that inherit from these. I can keep it more focused on only the MathMLElement addition if that is desired. Was the omittance of ElementCSSInlineStyle intentional?

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

I noticed it used mixins that were not there yet, which are also used for the interfaces SVGElement and HTMLElement, I've added them as well and removed the redundant attributes from interfaces that inherit from these.

This is unfortunately a breaking change, so we can't do this yet.


We prefer getting the bindings from the specification instead of from browser implementations. See https://www.w3.org/TR/MathML2/appendixe.html#dom-bindings.IDLBinding.

A changelog entry is missing as well.

@daxpedda daxpedda added the waiting for author Waiting for author to respond label Oct 7, 2024
@Philipp-M
Copy link
Contributor Author

This is unfortunately a breaking change, so we can't do this yet.

What's the policy here? Accumulate more breaking changes at once?

Though I think it's a "minor breaking change" if at all, everything should be working as expected, as the methods just shift towards the parent interfaces, and coercion should do the rest, I'm not sure if that's even considered a breaking change?

We could also leave the methods on the child interfaces there, and accept the duplicated methods, so that it's not a breaking change.

We prefer getting the bindings from the specification instead of from browser implementations. See https://www.w3.org/TR/MathML2/appendixe.html#dom-bindings.IDLBinding.

Thanks for the link, I missed that (although I studied the specs as well). AFAIK this browser implementation is a subset of the specs.

The motivation for this change, is to have a mathml_element.style(), which interestingly is not in that specs. It's here though: https://www.w3.org/TR/cssom-1/#the-elementcssinlinestyle-mixin
I guess I'll just combine these then?

I think I'll just add the sub-interfaces as well, so that we have a complete and more accurate representation of MathML then?

@Philipp-M
Copy link
Contributor Author

I'm not sure if that's even considered a breaking change

Well I guess it is, when it's used explicitly: HtmlInputElement::set_autofocus(el, true)

So that leaves this suggestion:

We could also leave the methods on the child interfaces there, and accept the duplicated methods, so that it's not a breaking change.

@daxpedda
Copy link
Collaborator

daxpedda commented Oct 8, 2024

This is unfortunately a breaking change, so we can't do this yet.

What's the policy here? Accumulate more breaking changes at once?

I think at this point we have more then enough changes lined up for a breaking change.
There just isn't enough maintainer time available right now to actually go through with it.

See #4090.

I'm not sure if that's even considered a breaking change

Well I guess it is, when it's used explicitly: HtmlInputElement::set_autofocus(el, true)

So that leaves this suggestion:

We could also leave the methods on the child interfaces there, and accept the duplicated methods, so that it's not a breaking change.

Indeed, this sounds reasonable.
The old interfaces can be marked with [RustDeprecated].

@Philipp-M
Copy link
Contributor Author

I've updated this to not be a breaking change anymore.

Well yet another time it turns out how messy the specification and its implementation in browsers is, e.g. https://www.w3.org/TR/MathML3/chapter2.html#fund.globatt specifies style directly, while e.g. mathElementStyle doesn't exist in the browsers I have checked (Firefox, Chrome). All the other methods seemingly not as well.
So I guess what is already in this PR, is at least compliant to the specs, and should cover all major currently used browsers...

Copy link
Collaborator

@daxpedda daxpedda 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 for cleaning up the mixin interfaces!

crates/web-sys/webidls/enabled/Element.webidl Outdated Show resolved Hide resolved
crates/web-sys/webidls/enabled/Element.webidl Outdated Show resolved Hide resolved
crates/web-sys/webidls/enabled/Element.webidl Outdated Show resolved Hide resolved
crates/web-sys/webidls/enabled/MathMLElement.webidl Outdated Show resolved Hide resolved
@Philipp-M
Copy link
Contributor Author

(force push, because CI didn't run successfully, but I don't think that failed test is related to the PR)

Copy link
Collaborator

@daxpedda daxpedda 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 for the patience!

@daxpedda daxpedda merged commit e37a1dd into rustwasm:main Oct 15, 2024
23 checks passed
@Philipp-M Philipp-M deleted the add-mathml-element branch October 15, 2024 23:01
github-merge-queue bot pushed a commit to linebender/xilem that referenced this pull request Dec 1, 2024
…t`s (#765)

As noted in
#621 (comment),
rustwasm/wasm-bindgen#4143 is now added in
wasm_bindgen 0.2.96.

This PR updates all the dependencies of xilem_web and finally correctly
supports `style` for `MathMlElement`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants