Add more test cases for document.all #7254
Conversation
Build PASSEDStarted: 2017-09-07 22:54:21 View more information about this build on: |
| assert_equals(document.all(), null); | ||
| }, "legacy caller with no argument"); | ||
|
|
||
| test(function() { | ||
| if (typeof Proxy !== "undefined") { |
There was a problem hiding this comment.
This should be removed I think. Otherwise you can pass the test by not implementing proxies.
There was a problem hiding this comment.
There's still the more straightforward test below that's not guarded by this if.
There was a problem hiding this comment.
Sure, but why would we guard this one?
There was a problem hiding this comment.
Yeah, guards in tests skew results and create weird incentives. Let's not go there?
|
|
||
| test(function() { | ||
| assert_equals(Function.prototype.call.call(document.all, {}, "043"), spans[1]); | ||
| }, "legacy caller with arbitrary this value"); |
There was a problem hiding this comment.
Can you add one where the this value is a platform object? E.g., document.body?
|
Thanks, these look good to me. I haven't run them in browsers yet. Are there many failures? We should probably file bugs, if any. |
|
@annevk I only tested Chrome and Firefox. Firefox passed all the new tests (but fails some of the old ones). Chrome fails the constructor test but passes all the other new tests (but also fails some of the old tests). I don't have access to either Safari or Edge right now. |
|
w3c-test:mirror |
|
Safari TP fails these tests:
|
|
@annevk The first two are the results of WebKit implementing the spec that no one else follows. The third is probably failing even before this change. |
|
Edge (sorry for the screenshot, browserstack makes copy-and-pasting suck): It would probably be good to file bugs on all browsers then? |
Created whatwg/webidl#434 for issue tracking purposes. |
There was a problem hiding this comment.
Revised to match my changes to whatwg/html#2932 ; checking my work appreciated. I confirmed Safari TP does pass all of them now though, except the preexisting problem "collections are new live HTMLCollection instances".
|
@domenic I can't approve my own PR but your changes LGTM. |
| new document.all("picture"); | ||
| }, "New should not work on document.all()"); | ||
|
|
||
| // https://esdiscuss.org/topic/isconstructor#content-11 |
There was a problem hiding this comment.
Thanks for adding this test! I tried to pass the above test by adding custom bindings that would throw TypeError when document.all() was invoked as a constructor, but this one still failed. So I had to go fix it in V8 which seems like the right place:
https://chromium-review.googlesource.com/c/v8/v8/+/882642
whatwg/html#2932