Closed Bug 667856 Opened 9 years ago Closed 8 years ago

Stringify attributes passed to nsIDOMElement.setAttribute()

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mounir, Assigned: mounir)

References

Details

(Whiteboard: [needs review])

Attachments

(1 file, 1 obsolete file)

Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #542406 - Flags: review?(bzbarsky)
So, does this test pass without adding the annotation to input.alt?
Comment on attachment 542406 [details] [diff] [review]
Patch v1

Boris, could you just review the IDL change. Ms2ger, could you review the tests?
Attachment #542406 - Flags: review?(Ms2ger)
Blocks: 667864
(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...
(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().
Blocks: 586786
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?
Attachment #542406 - Flags: review?(Ms2ger) → review+
> 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.
No longer blocks: 586786, 667864
Attached patch PatchSplinter Review
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?
Attachment #542406 - Attachment is obsolete: true
Attachment #542781 - Flags: review?(bzbarsky)
Attachment #542406 - Flags: review?(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....
Attachment #542781 - Flags: review?(bzbarsky) → review+
Paris bindings for element effectively did this.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Depends on: 814195
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.