Fix Node.append throwing Illegal invocation

[danieltroger]

Purpose

Fix our tests failing while the javascript code works outside of hammerhead

Example stack trace:

TypeError: Illegal invocation
    at Function.t._hasShadowUIParentOrContainsShadowUIClassPostfix (http://192.168.1.111:64494/hammerhead.js:10:25144)
    at t._onElementAdded (http://192.168.1.111:64494/hammerhead.js:10:25735)
    at t._addNodeCore (http://192.168.1.111:64494/hammerhead.js:10:18887)
    at HTMLSpanElement.append (http://192.168.1.111:64494/hammerhead.js:10:21546)

This happens when calling Element.append with a number or object as arguments. This gets serialized to text in JS but hammerhead assumes it's a node because a check wrongly assumes everything that's not a string is a node.

Approach

Change the check to assume everything that's not instanceof Node is not a node.

References

https://developer.mozilla.org/en-US/docs/Web/API/Element/append

Valid code:
div.append(123), div.append({hi: "test"}); <- this fails the check

Pre-Merge TODO

• Write tests for your proposed changes
• Make sure that existing tests do not fail

[miherlosev]

Hi @danielroe,

Thank you for your contribution to the testcafe-hammerhead module.

Could you please add tests for the fixed case here

testcafe-hammerhead/test/client/fixtures/sandbox/node/dom-processor-test.js

Line 109 in ec51bd8

test('Element.prototype.append', function () {

.

Also, check the node.append(null) scenario.

[danielroe]

I would love to but suspect you mean @danieltroger 😉

[danieltroger]

Also, check the node.append(null) scenario.null

It should be fine, no? In that case null becomes coerced to a string by the browser which is reflected by the new behavior: it goes into the first case where the next function gets "null" which is a normal string and shouldn't confuse it.

Could you please add tests for the fixed case here

If the tests are easy to run I maybe can do it. I wanted to open DevExpress/testcafe#6719 but didn't because I couldn't figure out how to run the right tests and not 300 irrelevant ones. How can I run that specific test?

[miherlosev]

It should be fine, no? In that case, null becomes coerced to a string by the browser which is reflected by the new behavior: it goes into the first case where the next function gets "null" which is a normal string and shouldn't confuse it.

This will raise an unhandled exception due to the toString method call on the null object.
https://github.com/DevExpress/testcafe-hammerhead/pull/2730/files#diff-7c84f76327fa18006608834e14dfdd38de9c53fc8aa9c86e1c105eb955ec4bdcR497

If the tests are easy to run I maybe can do it.

You don't need to write tests in the testcafe repository. You need to add tests in

testcafe-hammerhead/test/client/fixtures/sandbox/node/dom-processor-test.js

Line 109 in ec51bd8

test('Element.prototype.append', function () {

To run tests you need to execute 2 gulp tasks: gulp build and gulp gulp test-client. After that, in your terminal, you will see the URL that you must open in a browser. Then you need to run client QUnit tests.

[danieltroger]

This will raise an unhandled exception due to the toString method call on the null object.

Ah, fair point. Doing node + "" instead should work then though, right?

To run tests you need to execute 2 gulp tasks: gulp build and gulp gulp test-client. After that, in your terminal, you will see the URL that you must open in a browser. Then you need to run client QUnit tests.

Ok thanks I'll try it out

[danieltroger]

Added tests.

Here they are failing on current master:

Here they are when ran on my patch-1 branch:

[miherlosev]

Doing node + "" instead should work then though, right?

Usually, we use the String(node) expression for this.

Here they are failing on current master:

You changes breaks the existing use case. Please fix this.

[danieltroger]

You changes breaks the existing use case. Please fix this.

It seems like the issue is that the function which I changed is used in both Element.append and Element.appendChild but my changes only should apply to Element.append.

Changing this would require quite some digging and learning the undocumented inner workings of testcafe. I'd appreciate if you could take care of it.

I've already debugged, identified and solved the issue for my case and just thought I'd share so that you don't have to do the work again when the next person opens an issue about it.