-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 tests for 'class' and 'for' aliases for 'className' and 'htmlFor' #48220
base: master
Are you sure you want to change the base?
Add tests for 'class' and 'for' aliases for 'className' and 'htmlFor' #48220
Conversation
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.
Looks good to me but I think a second opinion is needed just because I'm not experienced to know that we merge tests before the work has been done to verify that the change is not going to break the web.
For subsequent reviewers, the changes are good apart from the one issue I flagged. So deep second review is not necessary (I feel).
I think they'd need to be marked as tentative for that, which is impossible for the modified tests so I also think this should only get merged after the spec changes, which should only get merged after we know if this is actually web compatible. |
Delaying landing this seems good. In that case I'll approve the PR. |
|
||
<script> | ||
test(function() { | ||
assert_true(testDiv.class == "", "class must reflect \"class\""); |
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 use assert_equals
@@ -356,7 +356,8 @@ interface Element : Node { | |||
readonly attribute DOMString tagName; | |||
|
|||
[CEReactions] attribute DOMString id; | |||
[CEReactions] attribute DOMString className; | |||
[CEReactions] attribute DOMString class; | |||
[CEReactions] attribute DOMString className; // legacy alias of .class |
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.
Note that a bot will make those changes as soon as the spec change lands
This adds new tests that test for the behavior proposed in issue #9379 on the HTML spec and issue #1310 on the DOM spec.
Some other tests have also been updated and expanded to include the new behavior.
(Notably, acid3 was testing for the exact opposite, this has been inverted)