1141498 - Make testcommon.js's addDiv() more portable, and more flexible

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Jonathan Watt [:jwatt]]

From bug 1073379 comment 22:

According to MDN...
 https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/remove#Browser_compatibility
...remove() isn't supported everywhere -- it's supported not in IE, at least. Given that this is a testharness.js test & hence can in theory be run w/ other browsers, maybe better to avoid using remove(). (and use div.parentNode.removeChild(div) instead)

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Stop using Node.remove() in the DOM animation test's testcommon.js

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Enable calls to DOM animation test's testcommon.js's addDiv() to set attributes

Per the "Obviously, feel free to wrap or extend it to add the extra class / ID setting behaviour" comment in bug 1072037 comment 11.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8575256 [details] [diff] [review]
part 2 - Enable calls to DOM animation test's testcommon.js's addDiv() to set attributes

>+++ b/dom/animation/test/testcommon.js
>-function addDiv(t) {
>+function addDiv(t, attrs) {
>   var div = document.createElement('div');
>+  if (attrs) {
>+    for (var attr in attrs) {
>+      div.setAttribute(attr, attrs[attr]);
>+    }
>+  }

nit: maybe rename "attr" to "attrName"? For me at least, that'd make the setAttribute call easier to skim & make it easier to keep track of what the various values involved are.

r=me regardless

Comment 5

[Jonathan Watt [:jwatt]]

Sure, sounds good.

Comment 6

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d366dc0fb693
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab01fe57d48

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Both backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f8076d7dcfcb for mochitest-1 bustage, along with bug 1141710:

https://treeherder.mozilla.org/logviewer.html#?job_id=7437755&repo=mozilla-inbound

Comment 8

[Jonathan Watt [:jwatt]]

Part 1 caused the test test_element-get-animation-players.html to fail because calling element.remove() is a no-op when the element has no parent node, but of course if element.parentNode is null then evaluating element.parentNode.removeChild(element) causes a JavaScript error. So despite how the patch looked, the old and new code are not strictly equivalent! :/

Comment 9

[Jonathan Watt [:jwatt]]

Repushed with fix after full local run of all test files using this utilities file.

https://hg.mozilla.org/integration/mozilla-inbound/rev/475fe01b5ddc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f168c9676e5c

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/475fe01b5ddc
https://hg.mozilla.org/mozilla-central/rev/f168c9676e5c