-
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
[cssom] Add tests for CSSGroupingRule#insertRule
#17276
[cssom] Add tests for CSSGroupingRule#insertRule
#17276
Conversation
(Needs a rebase to pass CI, due to #17286. Sorry!) |
aaf396a
to
4ce782c
Compare
No problem. Rebased! |
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.
LGTM % 1 bug :)
var first = groupingRule.cssRules[0].cssText; | ||
|
||
assert_throws('HierarchyRequestError', function() { | ||
groupingRule.insertRule('@import url ("foo.css");', 0); |
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.
There shouldn't be a space between url
and (
Thanks for your eagle eye, @zcorpan |
Spec PR w3c/csswg-drafts#4057 |
@zcorpan noted that the two definitions differ in their use of |
groupingRule.deleteRule('.baz {}', 0); | ||
} catch (err) {} | ||
|
||
assert_equals(groupingRule.cssRules.wptMarker, 'wpt'); |
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.
This test could pass if the returned object is a new structured clone each time. Add a straight assert_equals
on the object directly, too.
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.
I based this off of existing CSSOM tests. Asserting object equality will require creating additional references, so I assumed this wasn't done as a way to promote garbage collection.
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.
Ah, yeah, storing a reference would defeat the purpose of testing the effect of GC. But as-is it seems extremely unlikely for a GC to happen in the middle of running this test...
That said, I think this is good enough to merge.
These tests are somewhat contentious due an inconsistency in the CSS specification: w3c/csswg-drafts#3528
e31996f
to
98dcdfd
Compare
Thanks for the review, @zcorpan. The spec change was accepted today, so I'll merge this myself. |
These tests are somewhat contentious due an inconsistency in the CSS
specification:
w3c/csswg-drafts#3528