-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Rename HTMLOrSVGElement to reflect its wider use in MathML as well #5248
base: main
Are you sure you want to change the base?
Conversation
Thanks! I think the main issue here is the tests.
It's unclear what these have to do with the PR. This tests dataset, tabIndex, focus, and blur. It does not test autofocus or nonce. These tests are also not exhaustive of the behaviors involved, but I don't think that's required, unless there exists exhaustive tests for SVG elements and those behaviors that we should try to be at least as good at. Still, at least a test for focus()'s options would be nice. Also, is there any need to modify https://html.spec.whatwg.org/#dom-tabindex for any MathML elements, or do they all always give |
@@ -126042,3 +126042,4 @@ INSERT INTERFACES HERE | |||
|
|||
</body> | |||
</html> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this addition
This reverts commit 543940f.
I guess these tests are for the more general case of adding a MathMLElement IDL (which includes the case of HTMLOrForeignElement).
Yes, this is what I asked in #4893 (comment) ; @bkardell since you wrote these tests, do you know why autofocus and nonce were not tested? Additionally, I mentioned these two tests for tabindex on the abandoned PR:
I agree it would be nice to have more tests however, I believe that would be asking way too much for just a renaming to HTMLOrForeignElement. Even the important case of tabindex is not tested with SVG last time I checked.
MathML3 allows an |
I disagree. The point of an integration PR is to be sure any integration into the HTML Standard is up to the requirements of the HTML Standard. If the goal is to be a first-class peer of other HTML Standard types, then there needs to be the corresponding amount of work done. It's true that before the modern WHATWG working mode, some PRs went un-tested. That's not a reason to abandon test discipline on future changes. |
@domenic I agree with what you are saying so I believed you overlooked the just a renaming to HTMLOrForeignElement part of my comment. Unless I'm missing something, the renaming alone does not cause any observable change in implementations so there is actually nothing testable and it does not make sense to require tests in the first place. And even forgetting about MathML, the renaming makes sense to do it by itself, since that matches the internal name used by browsers and makes the interface name less specific in case it has to be reused (e.g. Mozilla uses it for XULElement). Now if we send more PRs that actually have an observable change such as introducing |
I did not overlook this. The renaming integration PR is an opportunity to improve the test coverage and hold the line, if we're going to welcome MathMLElement into the same family as HTMLOrSVGElement. If we're not willing to do that, then we should just live with the not-so-suitable name, as a reminder that MathMLElement is not up to the same standards. |
Perhaps another way of looking at it is: it seems autofocus, nonce, and the options member of focus() were added to the MathMLElement spec without any tests. This may be acceptable working practice for the MathML working group, but it is not acceptable for the WHATWG. Thus, at the point where the specs intersect in a way that asks for changes to WHATWG standards, we need to ensure that those tests are present. Even if the way that the specs intersect is just in a request for renaming. |
This test seems to reveal that this PR is incomplete, and https://html.spec.whatwg.org/#dom-tabindex also needs modification to add something like "a MathML element with a href attribute", before we could merge this PR. (Similar to the above comments, I realize that this PR is currently just a renaming, and that the MathML working group has mixed in |
@domenic I'm uncomfortable with this approach as that makes the boundary of required tests quite arbitrary, as opposed to focusing on changes in what is actually specified in the WHATWG spec. Anyway, I'm not going to argue any further and with that explanation, I agree that this PR is not ready yet. Thanks for the useful feedback and please try to be a bit less negative in the way you communicate it. |
@@ -126041,4 +126041,4 @@ INSERT INTERFACES HERE | |||
<!-- Hopefully Kam and Tess won't notice they're covered by these acknowledgments three times! --> | |||
|
|||
</body> | |||
</html> | |||
</html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don’t remove trailing new‑lines.
</html> | |
</html> |
…nkable MathML elements... test updates to follow
@domenic After some decision making and conversations - proposed spec updates for this added, will be updating tests to reflect here too, assuming this much looks right to you? |
Awesome, yeah! Editorially the latest commit looks good to me; do the proposed semantics match existing implementations (or do we have multi-implementer interest in updating to match them)? |
@domenic They have differences - but I believe the answer is yes we have agreement to match: this decision was made in the CG in cooperation with @emilio and double checked back with @rniwa , we're working on chromium - I think that about covers us. |
Attempting to match defaults defined in whatwg/html#5248 definitely needs scrutiny..
* Update tabindex-001.html Attempting to match defaults defined in whatwg/html#5248 definitely needs scrutiny.. * Update tabindex-001.html The default `.tabIndex` of an mrow should be 0 regardless of when it has an href or not, as a linkable element and matching historical (oddity) of other linkable things in HTML/SVG
Generalizing HTMLOrSVGElement to MathML elements seem fine to me since it doesn't post any backwards compatibility concerns. |
… mrow test, a=testonly Automatic update from web-platform-tests correct default expectation for existing mrow test (#22226) * Update tabindex-001.html Attempting to match defaults defined in whatwg/html#5248 definitely needs scrutiny.. * Update tabindex-001.html The default `.tabIndex` of an mrow should be 0 regardless of when it has an href or not, as a linkable element and matching historical (oddity) of other linkable things in HTML/SVG -- wpt-commits: 6b2fee71a825487369f34f82e7daa732920d186e wpt-pr: 22226
All mathml elements return -1 for tabIndex unless it's a link (because it has href) in which case it would return tabIndex of 0 in WebKit so this PR is consistent with that behavior. In general, I'm not too concerned about the focusing behavior within MathML than the rendering behavior since I haven't encountered a content where portions of MathML were interactive, had a hyperlink, etc... |
… mrow test, a=testonly Automatic update from web-platform-tests correct default expectation for existing mrow test (#22226) * Update tabindex-001.html Attempting to match defaults defined in whatwg/html#5248 definitely needs scrutiny.. * Update tabindex-001.html The default `.tabIndex` of an mrow should be 0 regardless of when it has an href or not, as a linkable element and matching historical (oddity) of other linkable things in HTML/SVG -- wpt-commits: 6b2fee71a825487369f34f82e7daa732920d186e wpt-pr: 22226 UltraBlame original commit: 88b6c5710ce466879d3991f983b9d5397d60a222
… mrow test, a=testonly Automatic update from web-platform-tests correct default expectation for existing mrow test (#22226) * Update tabindex-001.html Attempting to match defaults defined in whatwg/html#5248 definitely needs scrutiny.. * Update tabindex-001.html The default `.tabIndex` of an mrow should be 0 regardless of when it has an href or not, as a linkable element and matching historical (oddity) of other linkable things in HTML/SVG -- wpt-commits: 6b2fee71a825487369f34f82e7daa732920d186e wpt-pr: 22226 UltraBlame original commit: 88b6c5710ce466879d3991f983b9d5397d60a222
… mrow test, a=testonly Automatic update from web-platform-tests correct default expectation for existing mrow test (#22226) * Update tabindex-001.html Attempting to match defaults defined in whatwg/html#5248 definitely needs scrutiny.. * Update tabindex-001.html The default `.tabIndex` of an mrow should be 0 regardless of when it has an href or not, as a linkable element and matching historical (oddity) of other linkable things in HTML/SVG -- wpt-commits: 6b2fee71a825487369f34f82e7daa732920d186e wpt-pr: 22226 UltraBlame original commit: 88b6c5710ce466879d3991f983b9d5397d60a222
This is how it is implemented in Gecko and Chromium too. However, beware that @bkardell's proposal is to make tabIndex == 0 for all potentially linkable elements, not only those that are actually links. Apparently he said that's the case for in SVG and HTML too, but the CG people didn't want to introduce a separate element for MathML.
People from the CG said they used links it widely, but not sure they actually had to rely on tabindex behavior, which was only implemented recently. |
Oh, that's not good. What's the set of those elements? |
From the small actual spec edit, based on @domenic's observations and questions above:
Apologies if this feels like I am saying something too basic or something, but I feel like it would be good to state what seem like the important bits here because there seems to have been some confusion in earlier discussions... This spec edit is about setting what the default .tabIndex reports as, which isn't much of a good indicator of pretty much anything as far as I can tell, really and feels a little obscure: The default We'd like, ultimately, for advances in the platform to lift all the boats, and this is substantially more difficult where we do something different. |
It's very misleading to change the semantics of I don't think we should make such focus behavior semantics change in this PR. Please split it into a separate PR. |
@bkardell See the relevant article on GitHub Help for details. |
I'm keeping an eye on this PR because it's one of the things that need fixing to make the Web IDL of HTML + MathML consistent for https://github.com/w3c/webref, which is used for web-platform-tests and MDN. @bkardell what still remains to resolve to get this PR merged? It looks like the discussion of focus behavior ended up with the conclusion that focus behavior changes should be done in another PR, so is there anything else still blocking? |
It's really linking and relationship to I would be happy to remove the |
(updated tests)
Renames relatively recent
HTMLOrSVGElement
IDL mixin toHTMLOrForeignElement
in order to reflect its wider use in MathML as well, which previously was justElement
. This is referenced in the new interface defined in MathML-Core and aimed at resolving this historical oddity.At least two implementers are interested (and none opposed):
Tests are written and can be reviewed and commented upon at:
Implementation bugs are filed:
(See WHATWG Working Mode: Changes for more details.)
💥 Error: Wattsi server error 💥
PR Preview failed to build. (Last tried on Jan 15, 2021, 8:01 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.
🔗 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.