E550 Add `MathMLElement` interface and mixins `ElementCSSInlineStyle`, `HTMLOrForeignElement` by Philipp-M · Pull Request #4143 · wasm-bindgen/wasm-bindgen · GitHub
[go: up one dir, main page]
Skip to content

Add MathMLElement interface and mixins ElementCSSInlineStyle, HTMLOrForeignElement #4143

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 15, 2024
Merged

Add MathMLElement interface and mixins ElementCSSInlineStyle, HTMLOrForeignElement #4143

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?

daxpedda
daxpedda requested changes Oct 7, 2024
View reviewed changes
Copy link
Member
@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
Member
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...

@Philipp-M Philipp-M mentioned this pull request Oct 10, 2024
Merged
daxpedda
daxpedda requested changes Oct 10, 2024
View reviewed changes
Copy link
Member
@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!

daxpedda
daxpedda reviewed Oct 15, 2024
View reviewed changes
daxpedda
daxpedda reviewed Oct 15, 2024
View reviewed changes
daxpedda
daxpedda reviewed Oct 15, 2024
View reviewed changes
@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)

daxpedda
daxpedda approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member
@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 wasm-bindgen:main Oct 15, 2024
23 checks passed
@Philipp-M Philipp-M deleted the add-mathml-element branch October 15, 2024 23:01
@Philipp-M Philipp-M mentioned this pull request Nov 29, 2024
Merged
github-merge-queue bot pushed a commit to linebender/xilem that referenced this pull request Dec 1, 2024
@Philipp-M
xilem_web: Update dependencies, and support style for `MathMlElemen…
1eaff8f 
…t`s (#765)

As noted in
#621 (comment),
wasm-bindgen/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
Reviewers

@daxpedda daxpedda daxpedda approved these changes

Assignees
No one assigned
Labels
waiting for author Waiting for author to respond
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Philipp-M @daxpedda
0