667856 - Stringify attributes passed to nsIDOMElement.setAttribute()

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

So this is working but I really wonder why we don't default to what WebIDL specs say. I've read the comment (in the bug and the code) but the reason isn't obvious to me: shouldn't most arguments stringify null and undefined by default? I mean, I don't understand how it's less risky to keep this wrong behavior by default and be wrong most of the time instead of having a correct default behavior and be wrong in a few situations.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So, does this test pass without adding the annotation to input.alt?

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 542406 [details] [diff] [review]
Patch v1

Boris, could you just review the IDL change. Ms2ger, could you review the tests?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #1)
> So, does this test pass without adding the annotation to input.alt?

Yes, there is a todo for aElement[aAttr] = null;
I did add a test for input.alt to test aElement.setAttribute(aAttr, null). I'm not going to annotate all DOMString attributes...

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to comment #1)
> So, does this test pass without adding the annotation to input.alt?

FWIW, Webkit (Chromium 13 at least) seems to behave like us when setting an IDL attribute with null and undefined. Opera follows the specs and I didn't check for IE9. Though, all of them follows the specs when setting null or undefined trough .setAttribute().

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 542406 [details] [diff] [review]
Patch v1

--- a/content/html/content/test/reflect.js
+++ b/content/html/content/test/reflect.js
@@ -1,8 +1,80 @@
+/**
+ * Checks that a given attribute is correctly reflected as a string.
+ *
+ * @param aElement  Element   node to test
+ * @param aAttr     String    name of the attribute
+ */
+function reflectString(aElement, aAttr)
+{
+  // Tests when nothing is set.

"when the attribute isn't set"?

+  // Tests setting null to the content attribute.

setting the content attribute to null

+  aElement.setAttribute(aAttr, null);
+  is(aElement.getAttribute(aAttr), "null",
+     "null should have been stringify to 'null'");

stringified (my spelling checker still doesn't admit it's a word, but at least the grammar works out)

+  is(aElement[aAttr], "null",
+     "null should have been stringify to 'null'");
+  aElement.removeAttribute(aAttr);
+
+  // Tests setting null to the IDL attribute.

setting the IDL attribute to null

+  aElement[aAttr] = null;
+  todo_is(aElement.getAttribute(aAttr), "null",
+     "null should have been stringify to 'null'");
+  todo_is(aElement[aAttr], "null",
+     "null should have been stringify to 'null'");
+  aElement.removeAttribute(aAttr);

(Oh. todo. I swear I'm not blind!)

+  // Tests various strings.
+  var stringsToTest = [ "", 42, "null", "undefined", "foo", aAttr ];

I had some fun writing a few more tests. How about

  var stringsToTest = [
    // [test value, expected output]
    ["", ""],
    [42, "42"],
    ["null", "null"],
    ["undefined", "undefined"],
    ["foo", "foo"],
    [aAttr, aAttr],
    [true, "true"],
    [false, "false"],
    // ES5, verse 8.12.8.
    [{ toString: function() { return "foo" } },
     "foo"],
    [{ valueOf: function() { return "foo" } },
     "[object Object]"],
    [{ valueOf: function() { return "quux" },
       toString: undefined },
     "quux"]
    [{ valueOf: function() { return "foo" },
       toString: function() { return "bar" } },
     "bar"]
  ];

  stringsToTest.forEach(function([v,e]) {

And use e for the expected value below.

+
+  stringsToTest.forEach(function(v) {
+    aElement.setAttribute(aAttr, v);
+    is(aElement[aAttr], v,
+       "IDL attribute should return the value it has been sot to.");

Sot?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> but I really wonder why we don't default to what WebIDL specs say

Because we haven't historically, and changing means possibly breaking stuff?

> shouldn't most arguments stringify null and undefined by default?

They haven't for the last 10 years or so in Gecko.

> I don't understand how it's less risky to keep this wrong behavior by default

Because we've been shipping it for 10 years and sites aren't broken with it right now.  If we change it, maybe they'll break.  And maybe not.  We don't know.  That's called risk.  ;)

Furthermore, the XPConnect conversion in question is used for non-webfacing stuff too, so we would be possibly breaking extensions as well as Firefox UI code.

For quickstubbed stuff or the new DOM bindings that won't be an issue, of course.

Note that last I checked this part of the spec was in flux.  Has it settled down?  I'd really rather not make changes here if the spec is going to keep flipflopping.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch

This patch doesn't include the tests anymore (they have been landed with a todo). I'm still asking for a review but AFAIUI, the conclusion of the IRC conversation was that we could try to stringify "null" in quickstub'd method, right?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 542781 [details] [diff] [review]
Patch

r=me

We could try to stringify null in quickstubs by default if we audit the quickstubbed methods and add annotations for the ones that should not stringify....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Paris bindings for element effectively did this.