669578 - Add a reflectInt method to reflect.js

Status: RESOLVED FIXED

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

Comment 1

[Ainsley Chong]

Attachment: Added reflectInt() to reflect.js and added test_bug669578.html

Please let me know if you have any problems with the patch. Thanks!

Comment 2

[Mounir Lamouri (:mounir)]

Ainsley, the patch seems to be empty. Did you upload the correct file?

Comment 3

[Ainsley Chong]

Attachment: Patch v2

Comment 4

[Ainsley Chong]

Ah sorry, I built the patch incorrectly. Hopefully this one works.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 548031 [details] [diff] [review]
Patch v2

Review of attachment 548031 [details] [diff] [review]:
-----------------------------------------------------------------

You shouldn't create test_bug669578.html but instead, modify content/html/content/tests/forms/test_input_attributes_reflection.html, creates content/html/content/tests/forms/test_select_attributes_reflection.html and content/html/content/tests/test_li_attributes_reflection.html.

It seems like you are highly re-using reflectUnsignedInt() for reflectInt(). Unfortunately, reflectUnsignedInt() is one of the first method in this file and should be refreshed. I think you should try to have a look at what reflectString() and reflectBool() are doing. Those are more likely to be interesting.
I don't know if you had a look at this:
http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#reflecting-content-attributes-in-idl-attributes
Look at the paragraphs about "signed integer type (long)" and "signed integer type (long) that is limited to only non-negative numbers". You will see that three concepts are not defined in these paragraphs:
1. rules for parsing signed integers (but you have a link);
2. rules for parsing non-negative integers (you have a link too);
3. converted to the shortest possible string representing the number. For this, you should have a look at ECMAScript specifications [1], section 9.3.

Based on 1, 2, 3 and the other cases mentioned in the specs, you will have to chose some values to test.

If you have any question, feel free to ping me on IRC (volkmar) or send me an email (mounir _ AT _ mozilla.com).

[1] http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-262.pdf

::: content/html/content/test/reflect.js
@@ +451,5 @@
> + *
> + * @param aParameters   Object    object containing the parameters, which are:
> + *  - element           Element   node to test on
> + *  - attribute         String    name of the attribute
> + *  - nonZero           Boolean   whether the attribute should be non-null

It should be nonNegative instead of nonZero.

Comment 6

[Mounir Lamouri (:mounir)]

Ainsley, I marked this bug blocking bug 673820 because while looking at the specifications I saw that we are not following them in a specific situation. Our test harness lets you set todo's so you can add this test case and mark it as a todo then fix the other bug if you feel like it ;)

Comment 7

[Ainsley Chong]

Ah. Thanks for the feedback! I'll do some more reading and make another attempt.

Comment 8

[Ainsley Chong]

Attachment: Patch v3

Found some strange behavior while testing reflectInt().

general:
- It seems a bit strange that element[attr] = -3000000000 is allowed, and returns element[attr] == 1294967296. It makes sense bitwise but is pretty confusing.

select.size:
- setAttribute(size, "1234hello") --> getAttribute(size) == "1234". This is inconsistent with select.setAttribute(tabIndex, "1234hello") --> getAttribute(tabIndex) == "1234hello"

select.tabIndex:
- select[tabIndex] has a max value of 32767. Ex. select[tabIndex] = 55555 --> select[tabIndex] == 32767. Strangely though after that assignment select.getAttribute(tabindex) == 55555, I would think since the IDL attribute is limiting the value to 32767 it would also limit the value to the content attribute.

li.value:
- li[value] = -1 does not throw and INDEX_SIZE_EXCEPTION like other non-negative integer attributes will.
- .setAttribute(value, "   111111  ") does not trim out the whitespaces, which is done by other integer attributes.

input.maxLength:
- When created defaultValue == -1, but when an invalid value is passed such as input[maxlength] = "foo" it reflects 0 as the default value instead of -1. You also cannot input[maxLength] = -1 even though that is the initial value.

Comment 9

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

Some comments:

--- a/content/html/content/test/forms/test_input_attributes_reflection.html
+++ b/content/html/content/test/forms/test_input_attributes_reflection.html
-// TODO: height (non-negative integer)
+// TODO: height (non-negative integer) This does not seem to exist
-// TODO: width (non-negative integer)
+// TODO: width (non-negative integer) This does not seem to exist

Indeed. I filed a bug: http://www.w3.org/Bugs/Public/show_bug.cgi?id=13385

-// TODO: maxLength (long)
+//Bug: .setAttribute(maxLength, -0) --> .getAttribute(maxLength) == 0 instead of -0

This is correct: http://es5.github.com/#x9.8.1

+//Bug: .setAttribute does not trim input strings

What do you mean? setAttribute should always store the exact string you pass.

+//Bug: .setAttribute(maxLength, "1.2345") --> 

?

+// .maxLength (long)

Drop the "(long)"

--- /dev/null
+++ b/content/html/content/test/forms/test_li_attributes_reflection.html

This doesn't want to line in forms/, please move it to content/html/content/test/test_li_attributes_reflection.html.

+//Bug: li[value] = negative does not throw an error like it should

Should it really? AFAICT, it isn't "limited to only non-negative numbers"

--- /dev/null
+++ b/content/html/content/test/forms/test_select_attributes_reflection.html

(This is in the right place, though.)

--- a/content/html/content/test/reflect.js
+++ b/content/html/content/test/reflect.js
+
+/**
+ * Checks that a given attribute name for a given element is correctly reflected
+ * as an signed int.

int -> integer

+function reflectInt(aParameters)
+{
+  var element = aParameters.element;
+  var attr = aParameters.attribute;
+  var nonNegative = aParameters.nonNegative;
+  var defaultValue = aParameters.defaultValue;
+  var minValue = aParameters.minValue;
+  var maxValue = aParameters.maxValue;
+
+  ok(attr in element, attr + " should be an IDL attribute of this element");
+  is(typeof element[attr], "number", attr + " IDL attribute should be a number");
+
+  if (defaultValue === undefined) {defaultValue = 0;}
+  if (minValue === undefined && nonNegative) {minValue = 0;}
+  if (minValue === undefined && !nonNegative) {minValue = -2147483648;}
+  if (maxValue === undefined) {maxValue = 2147483647;}

Please move these up to the declarations or min/maxValue, like in reflectString.

I haven't looked at the rest in detail, but please get rid of the tabs that crept in and

+function isLimitedSignedInt(value) {
+
+  var intValue = parseInt(value);
+
+  if(!isNaN(intValue) && intValue <= 2147483647 && intValue >= 0) {
+    return true;
+  } else {
+    return false;
+  }   
+}

These functions should just 'return !isNaN...'

Comment 10

[Ainsley Chong]

+//Bug: .setAttribute(maxLength, -0) --> .getAttribute(maxLength) == 0 instead of -0
This is correct: http://es5.github.com/#x9.8.1

Gotcha. Then in that case I think select.setAttribute(tabIndex, -0) is not working correctly, select.getAttribute(tabIndex) == "-0"

+//Bug: .setAttribute does not trim input strings
What do you mean? setAttribute should always store the exact string you pass.

That's what I thought, but there are several cases where I did notice string trimming, such as: select.setAttribute(size, "   111111") --> getAttribute(size) == "111111", select.setAttribute(size, "1.2345"

Comment 11

[Ainsley Chong]

<sorry accidentally saves changes before finished>

...

+//Bug: .setAttribute does not trim input strings
What do you mean? setAttribute should always store the exact string you pass.

Ah ok. But there are several cases where I did notice string trimming, such as: select.setAttribute(size, "   111111") --> getAttribute(size) == "111111", select.setAttribute(size, "1.2345") --> getAttribute(size) == "1"

+//Bug: li[value] = negative does not throw an error like it should
Should it really? AFAICT, it isn't "limited to only non-negative numbers"

Yeah the li.value behaves strangely. If you attempt to set li[value] = <negative value> --> li[value] == 0, as if it does not allow negative numbers. However other attributes that don't allow negative numbers throw an error instead of returning the default value, not sure why.

I'll start working on your suggested changes. Thanks!

Comment 12

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

(In reply to comment #10)
> +//Bug: .setAttribute(maxLength, -0) --> .getAttribute(maxLength) == 0
> instead of -0
> This is correct: http://es5.github.com/#x9.8.1
> 
> Gotcha. Then in that case I think select.setAttribute(tabIndex, -0) is not
> working correctly, select.getAttribute(tabIndex) == "-0"
> 
> +//Bug: .setAttribute does not trim input strings
> What do you mean? setAttribute should always store the exact string you pass.
> 
> That's what I thought, but there are several cases where I did notice string
> trimming, such as: select.setAttribute(size, "   111111") -->
> getAttribute(size) == "111111", select.setAttribute(size, "1.2345"

(In reply to comment #11)
> <sorry accidentally saves changes before finished>
> 
> ...
> 
> +//Bug: .setAttribute does not trim input strings
> What do you mean? setAttribute should always store the exact string you pass.
> 
> Ah ok. But there are several cases where I did notice string trimming, such
> as: select.setAttribute(size, "   111111") --> getAttribute(size) ==
> "111111", select.setAttribute(size, "1.2345") --> getAttribute(size) == "1"
> 
> +//Bug: li[value] = negative does not throw an error like it should
> Should it really? AFAICT, it isn't "limited to only non-negative numbers"
> 
> Yeah the li.value behaves strangely. If you attempt to set li[value] =
> <negative value> --> li[value] == 0, as if it does not allow negative
> numbers. However other attributes that don't allow negative numbers throw an
> error instead of returning the default value, not sure why.

(Note to self: nsBlockFrame::RenumberLists gets the parsed value at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#6702, should probably support negative numbers.)

> I'll start working on your suggested changes. Thanks!

Thanks for taking this on! Please don't let all the comments discourage you, we just want the code to be as good as possible.

Comment 13

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

Comment on attachment 548681 [details] [diff] [review]
Patch v3

And I managed to do the same. The rest of my reply:

(In reply to comment #10)
> +//Bug: .setAttribute(maxLength, -0) --> .getAttribute(maxLength) == 0
> instead of -0
> This is correct: http://es5.github.com/#x9.8.1
> 
> Gotcha. Then in that case I think select.setAttribute(tabIndex, -0) is not
> working correctly, select.getAttribute(tabIndex) == "-0"

Really? That would be a bug.

(In reply to comment #11)
> +//Bug: .setAttribute does not trim input strings
> What do you mean? setAttribute should always store the exact string you pass.
> 
> Ah ok. But there are several cases where I did notice string trimming, such
> as: select.setAttribute(size, "   111111") --> getAttribute(size) ==
> "111111", select.setAttribute(size, "1.2345") --> getAttribute(size) == "1"

I was afraid that was the case. We'll need to change that at one point, but that's something for another bug.

(Flagging myself for review so I don't forget)

Comment 14

[Mounir Lamouri (:mounir)]

Comment on attachment 548681 [details] [diff] [review]
Patch v3

I would prefer to review an updated patch (I mean a patch including Ms2ger's comments).

The work you did for this patch is quite impressive and you shouldn't be discouraged by our comments: the idea is to be able to get the best patch possible and you are obviously in your way to reach that goal :)

Comment 15

[Ainsley Chong]

>  if (defaultValue === undefined) {defaultValue = 0;}
>  if (minValue === undefined && nonNegative) {minValue = 0;}
>  if (minValue === undefined && !nonNegative) {minValue = -2147483648;}
>  if (maxValue === undefined) {maxValue = 2147483647;}

> Please move these up to the declarations or min/maxValue, like in reflectString.

Not quite sure I understand. I added "minValue" and "maxValue" as input parameters because some IDL attributes seem to have limits and some don't. For example select[tabIndex] is limited to [-32767, 32767] while select[size] is not, and the function needs some way to know that.

> function isLimitedSignedInt(value) {

> These functions should just 'return !isNaN...'

I think there needs to be functions to check when to throw INDEX_SIZE_EXCEPTION. According to the link it should be thrown if a non-negative int is set to a negative value, but I also noticed that it throws that error if it is set to a value outside the legal 32 bit range (not sure if this is the expected behavior). 

That means the inputs must be between [0, 2147483647] || [-4294967296, -2147483649] or an INDEX_SIZE_EXCEPTION should be thrown. At least, this is how I understand it from:

http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#reflecting-content-attributes-in-idl-attributes

Comment 16

[Ainsley Chong]

Attachment: Patch v4

I've added todo_is() statements in reflect.js to catch the failing tests which look like they may be bugs. Here is a summary of the issues:

1) Not throwing NS_ERROR_DOM_INDEX_SIZE_ERROR (li[value]): It's still not clear to me if this attribute is a regular signed integer or a non-negative signed integer. However when you set li[value] = <negative integer> -> li[value] == 0, current behavior does not accept negative integers as legal input. However if it is a non-negative integer it is supposed to throw NS_ERROR_DOM_INDEX_SIZE_ERROR.

2) element.setAttribute("-0") -> element.getAttribute == "-0", should be "0". (li[value], select[size], select[tabIndex])

3) After element.setAttribute(), element.getAttribute() shows that the input string has been trimmed. (select[size], select[tabIndex], input[maxLength]) This varies from trimming whitespace ("    111" -> "111") to trimming letters ("1234hello" -> "1234") to trimming decimal values ("1.2345" -> "1"). Particularly strange is the case where "why 567 what" is trimmed to "567". According to the parsing rules I believe it should just be thrown out as NaN.

4) For a attribute that allows negative values, element[attr] = -2147483648 -> element[attr] == 0. I think this assignment is supposed to be allowed. (select[tabIndex])

5) On creation element is set with a default value of -1, but on illegal assignment ex. element[attr] = "foo" -> element[attr] == 0 instead of -1. (input[maxLength])

Comment 17

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

Comment on attachment 549663 [details] [diff] [review]
Patch v4

--- a/content/html/content/test/reflect.js
+++ b/content/html/content/test/reflect.js
+/**
+ *  - nonNegative       Boolean   whether the attribute is allowed to be negative

"false if the attribute is 'limited to only non-negative numbers', and true otherwise"

Looking at the code, it looks like it's the other way around, though.

+ *  - defaultValue      Integer   [optional] default value, if different from the default one

The default default? How about "default value, if one exists"?

+ */
+function reflectInt(aParameters)
+{
+  var defaultValue = aParameters.defaultValue;
+  var minValue = aParameters.minValue;
+  var maxValue = aParameters.maxValue;
+
+  if (defaultValue === undefined) {defaultValue = 0;}
+  if (minValue === undefined && nonNegative) {minValue = 0;}
+  if (minValue === undefined && !nonNegative) {minValue = -2147483648;}
+  if (maxValue === undefined) {maxValue = 2147483647;}

What I'd prefer to see here is

var defaultValue = aParameters.defaultValue !== undefined
                     ? aParameters.defaultValue : 0;
var minValue = aParameters.minValue !== undefined
                 ? aParameters.minValue : (nonNegative ? 0 : -2147483648);
var maxValue = aParameters.maxValue !== undefined
                 ? aParameters.maxValue : 2147483647;

Though actually, those constants should be -0x80000000 and 0x7FFFFFFF. Also, the correct code for defaultValue is

var defaultValue = aParameters.defaultValue !== undefined
                     ? aParameters.defaultValue : (nonNegative ? 0 : -1);

+  /**
+   * the stringified value and a 'result' property containing the expected
+   * result when the value is set to the IDL attribute.
+   */

I don't understand your explanation of 'result'. I'm also not happy with result: "invalidResult"; should that be NaN?

+  var valuesToTest = [
+    { value: "-0", stringified: "0", result: 0 },

This is wrong, actually. |"-0"| is a string and shouldn't change. Its |-0| (integer literal) that stringifies to |"0"|. Could you add that case?

+    // Test special characters

characters?

+    { value: undefined, stringified: "undefined", result: "invalidResult" },
+    { value: null, stringified: "", result: "invalidResult" },
+    { value: NaN, stringified: "NaN", result: "invalidResult" },
+    { value: Infinity,  stringified: "Infinity", result: "invalidResult" },
+    { value: -Infinity, stringified: "-Infinity", result: "invalidResult" },
+  ];
+
+  valuesToTest.forEach(function(v) {
+   

Remove this empty line.

+    //Apply minValue and maxValue limits to expected results

Add a space before "Apply" and terminate the sentence with a period.

+    var limitedResult = 0;
+    if(v.result > 0) {
+      limitedResult = Math.min(v.result, maxValue);
+    } else if(v.result < 0) {
+      limitedResult = Math.max(v.result, minValue);
+    }

var limitedResult = Math.min(Math.max(v.result, minValue), maxValue);

But maybe call this "expected" or "expectedResult"?

+    element.setAttribute(attr, v.value);
+    // Check if value is within allowed limits for the signed integers
+    if((nonNegative && isLimitedSignedInt(v.value)) || (!nonNegative && isSignedInt(v.value))) {

if ((nonNegative ? isLimitedSignedInt : isSignedInt)(v.value)) {

is clearer, I think.

+      //TBD: Element that allows negative values not interpreting -2147483648 as a legitiate input

legitimate

+      if (!nonNegative && v.result == -2147483648 && element[attr] != limitedResult) { 

Drop the element[attr] != limitedResult check.

+        todo_is(element[attr], limitedResult, element.nodeName + ".setAttribute(" + attr + ", " + v.value + "), " + 
+          element.nodeName + "[" + attr + "] ");
+      } else {
+      is(element[attr], limitedResult, element.nodeName + ".setAttribute(" + attr + ", " + v.value + "), " + 
+        element.nodeName + "[" + attr + "] ");

Indentation is off.

+      }
+    } else {
+      //TBD: Element attributes are not returning the correct default value for invalid inputs
+      if(isNaN(parseInt(v.value)) && element[attr] != defaultValue) {

This is for the space trimming? If so, could you add an extra (optional?) property to the test objects to make that clear?

+    //TBD: Element does not correctly .setAttributes(-0) -> .getAttributes() == 0
+    if(element.getAttribute(attr) === "-0") {

This should go, see above.

+      todo_is(element.getAttribute(attr), v.stringified, element.nodeName + ".setAttribute(" + attr + ", " + v.value + "), " + 
+        element.nodeName + ".getAttribute(" + attr + ") ");

+    // Check if input is a number outside the limits of signed integers
+    } else if((!nonNegative && !isDOMSignedInt(v.value)) || (nonNegative && !isDOMLimitedSignedInt(v.value))) {

This can use ?: like above.

+      try {
+        element[attr] = v.value; 
+        //TBD: Element does not throw INDEX_SIZE_ERROR correctly when given numeric input that is out of bounds
+        todo(false, element.nodeName + "[" + attr + "] = " + v.value + " should throw NS_ERROR_DOM_INDEX_SIZE_ERR");
+      } catch(e) {  
+        caught = true;

Drop this line.

+        is(e.code, DOMException.INDEX_SIZE_ERR, element.nodeName + "[" + attr + "] = " + v.value + " should throw NS_ERROR_DOM_INDEX_SIZE_ERR");
+      }

+function isSignedInt(value) {
+
+  var intValue = parseInt(value);
+
+  if(!isNaN(intValue) && intValue <= 2147483647 && intValue >= -2147483648) {
+    return true;
+  } else {
+    return false;
+  }   
+}

I'd prefer

function isSignedInt(value) {
  var intValue = parseInt(value);
  return !isNaN(intValue) && intValue <= 2147483647 && intValue >= -2147483648;
}

And similar code for the other functions.

+
+function isDOMSignedInt(value) {
+function isDOMLimitedSignedInt(value) {

I don't understand what these are trying to do.

Please remove the trailing whitespace you add throughout this file and use element.localName instead of element.nodeName.

--- a/content/html/content/test/forms/test_input_attributes_reflection.html
+++ b/content/html/content/test/forms/test_input_attributes_reflection.html
+// .maxLength
+reflectInt({
+  element: document.createElement("input"),
+  attribute: "maxLength",
+  nonNegative: true,
+  defaultValue: -1,
+  minValue: -1,
+});

Drop the defaultValue. I also need to figure out what the minValue is for.

--- /dev/null
+++ b/content/html/content/test/forms/test_select_attributes_reflection.html
+// .size
+reflectInt({
+  element: document.createElement("select"),
+  attribute: "size",
+  nonNegative: true,
+  defaultValue: 0,
+});

This is correct.

+// .tabIndex
+reflectInt({
+  element: document.createElement("select"),
+  attribute: "tabIndex",
+  nonNegative: false,
+  defaultValues: 0,

Typo

+  maxValue: 32767,
+  minValue: -32768,
+});

Ainsley, this is looking pretty good already, but the problem is that the code it's testing is an awful mess. I'm marking it r- because I want to see the updated patch at least once more, and I still need to wrap around some parts of the code. I hope we're not scaring you off, you're doing fantastic work!

Mounir, could you do the next pass over the patch?

Comment 18

[Mounir Lamouri (:mounir)]

Comment on attachment 549663 [details] [diff] [review]
Patch v4

Review of attachment 549663 [details] [diff] [review]:
-----------------------------------------------------------------

I realize that some comments I wrote might have been answered in previous comments in the bug. Feel free to ignore them.

In a general note, as Ms2ger said, the patch is pretty good but it seems to be testing something that is pretty buggy. We should try to file bugs on those individual issues and fix them ASAP to clean this test.
Again, thank you for working on that!

::: content/html/content/test/forms/test_input_attributes_reflection.html
@@ +99,5 @@
>    attribute: "formTarget",
>    otherValues: [ "_blank", "_self", "_parent", "_top" ],
>  });
>  
> +// TODO: height (non-negative integer) This does not seem to exist

Might be a good idea to point the spec bug Ms2ger opened.

@@ +113,5 @@
> +reflectInt({
> +  element: document.createElement("input"),
> +  attribute: "maxLength",
> +  nonNegative: true,
> +  defaultValue: -1,

This is the expected default value for nonNegative, we shouldn't have to set it.

@@ +114,5 @@
> +  element: document.createElement("input"),
> +  attribute: "maxLength",
> +  nonNegative: true,
> +  defaultValue: -1,
> +  minValue: -1,

This is the expected value for nonNegative, we shouldn't have to set it.

@@ +115,5 @@
> +  attribute: "maxLength",
> +  nonNegative: true,
> +  defaultValue: -1,
> +  minValue: -1,
> +});

The caller should be able to do:
// .maxLength
reflectInt({
  element: document.createElement("input"),
  attribute: "maxLength",
  nonNegative: true,
}
and have the same result as above.

@@ +206,5 @@
>  // .selectedOption
>  todo("selectedOption" in document.createElement("input"),
>       "selectedOption isn't implemented yet");
>  
> +// TODO: width (non-negative integer) This does not seem to exist

Might be a good idea to point the spec bug Ms2ger opened.

::: content/html/content/test/forms/test_select_attributes_reflection.html
@@ +33,5 @@
> +  nonNegative: false,
> +  defaultValues: 0,
> +  maxValue: 32767,
> +  minValue: -32768,
> +});

AFAIK, tabindex isn't a full and simple content attribute reflection. For example, the default value depends of the element being focused or not. I wouldn't try to test tabIndex that way. Though, that's interesting to see that there is a min and max value here...

::: content/html/content/test/reflect.js
@@ +471,5 @@
> +
> +  if (defaultValue === undefined) {defaultValue = 0;}
> +  if (minValue === undefined && nonNegative) {minValue = 0;}
> +  if (minValue === undefined && !nonNegative) {minValue = -2147483648;}
> +  if (maxValue === undefined) {maxValue = 2147483647;}

Would be better to do that before the first checks and with the assignments.
Like:
var nonNegative = aParameters.nonNegative ? aParameter.nonNegative : false;
var defaultValue = aParameters.defaultValue ? aParameters.defaultValue : nonNegative ? 0 : -1;

Same goes for minValue and maxValue.

@@ +540,5 @@
> +    { value: undefined, stringified: "undefined", result: "invalidResult" },
> +    { value: null, stringified: "", result: "invalidResult" },
> +    { value: NaN, stringified: "NaN", result: "invalidResult" },
> +    { value: Infinity,  stringified: "Infinity", result: "invalidResult" },
> +    { value: -Infinity, stringified: "-Infinity", result: "invalidResult" },

This list is pretty huge which is great! Though, I would add those values:
""
" "
"+"
"-"
"+foo"
"-foo"
"+    foo"
"42.0"
"+42"
"1e5"
"42+-$"
value higher than PR_MAX
value lower than PR_MIN

@@ +544,5 @@
> +    { value: -Infinity, stringified: "-Infinity", result: "invalidResult" },
> +  ];
> +
> +  valuesToTest.forEach(function(v) {
> +   

Remove this empty line :)

@@ +556,5 @@
> +
> +    element.setAttribute(attr, v.value);
> +    // Check if value is within allowed limits for the signed integers
> +    if((nonNegative && isLimitedSignedInt(v.value)) || (!nonNegative && isSignedInt(v.value))) {
> +      //TBD: Element that allows negative values not interpreting -2147483648 as a legitiate input

Does that apply to something else than tabindex?

@@ +565,5 @@
> +      is(element[attr], limitedResult, element.nodeName + ".setAttribute(" + attr + ", " + v.value + "), " + 
> +        element.nodeName + "[" + attr + "] ");
> +      }
> +    } else {
> +      //TBD: Element attributes are not returning the correct default value for invalid inputs

That should be fixed. Could you open a bug and CC me?

@@ +574,5 @@
> +        is(element[attr], defaultValue, element.nodeName + ".setAttribute(" + attr + ", " + v.value + "), " + 
> +          element.nodeName + "[" + attr + "] ");
> +      }
> +    }
> +    //TBD: Element does not correctly .setAttributes(-0) -> .getAttributes() == 0

This is interesting and should be fixed. Could you open a bug for that and CC me?

@@ +578,5 @@
> +    //TBD: Element does not correctly .setAttributes(-0) -> .getAttributes() == 0
> +    if(element.getAttribute(attr) === "-0") {
> +      todo_is(element.getAttribute(attr), v.stringified, element.nodeName + ".setAttribute(" + attr + ", " + v.value + "), " + 
> +        element.nodeName + ".getAttribute(" + attr + ") ");
> +    //TBD: Element .setAttributes() is trimming data from the input string as if using parseInt()

You mean .setAttribute("  foo  ") and .getAttribute() returns "foo"?

@@ +582,5 @@
> +    //TBD: Element .setAttributes() is trimming data from the input string as if using parseInt()
> +    } else if (v.stringified != element.getAttribute(attr) && element.getAttribute(attr) == parseInt(v.value)) {
> +      todo_is(element.getAttribute(attr), v.stringified, element.nodeName + ".setAttribute(" + attr + ", " + v.value + "), " + 
> +        element.nodeName + ".getAttribute(" + attr + ") "); 
> +    //TBD: Element .setAttributes() is somehow able to parse "why 567 what" into "567"

Same as above, I'm not sure I understand the issue here.

@@ +595,5 @@
> +    
> +    // Check if input is NaN or if parseInt returns NaN
> +    if(isNaN(v.value) || isNaN(parseInt(v.value))) {
> +        element[attr] = v.value;
> +        //TBD: Element attributes are not returning the correct default value for invalid inputs

That applies to which attribute?

@@ +607,5 @@
> +    // Check if input is a number outside the limits of signed integers
> +    } else if((!nonNegative && !isDOMSignedInt(v.value)) || (nonNegative && !isDOMLimitedSignedInt(v.value))) {
> +      try {
> +        element[attr] = v.value; 
> +        //TBD: Element does not throw INDEX_SIZE_ERROR correctly when given numeric input that is out of bounds

That applies to which attribute?

@@ +616,5 @@
> +      }
> +    // Otherwise test value normally
> +    } else {
> +      element[attr] = v.value;
> +      //TBD: Element that allows negative integers not recognizing -2147483648 as a valid value

I guess that is more or less the same issue than one of the first TBD. It applies to which attributes?

@@ +638,5 @@
> +     "When not set, the IDL attribute should return default value.");
> +}
> +
> +function isSignedInt(value) {
> +

You should probably remove this empty line (same goes for the next functions).

@@ +645,5 @@
> +  if(!isNaN(intValue) && intValue <= 2147483647 && intValue >= -2147483648) {
> +    return true;
> +  } else {
> +    return false;
> +  }   

You can probably do |return condition;| instead (same goes for the next functions).

@@ +646,5 @@
> +    return true;
> +  } else {
> +    return false;
> +  }   
> +}

I would prefer to have these four methods defined before their actual use points. In addition. you could make them local to |reflectInt|, no one will use them except this method, I believe.

Comment 19

[Ainsley Chong]

Thanks! I'll work on these comments this weekend.

Comment 20

[Ainsley Chong]

Questions/Comments:

Ms2ger:

> function isDOMSignedInt(value) {
> function isDOMLimitedSignedInt(value) {
> I don't understand what these are trying to do.

After doing setAttribute(value), we need to check if (-2147483648 <= value <= 2147483647) to know if element[attr] should return value or defaultValue. That's what isSignedInt() is for

After doing element[attr] = value, we need to check if (-4294967296 <= value <= 4293967295) to know if we should expect a INDEX_SIZE_ERR. That's what isDOMLimitedSignedInt() is for. I do find it strange that element[attr] = value accepts numbers outside the 32 bit range, you would know better if this is a bug or not.

>reflectInt({
>  element: document.createElement("input"),
>  attribute: "maxLength",
>  nonNegative: true,
>  defaultValue: -1,
>  minValue: -1,
>});
> Drop the defaultValue. I also need to figure out what the minValue is for.

You are right about the minValue, I have dropped it.

As for the defaultValue, it's needed because input[maxLength] behaves like a nonNegative value (throws INDEX_SIZE_ERR when input[maxLength] = -x) but has a default value of -1 instead of 0. This may be a bug.

Mounir:

> +reflectInt({
> +  element: document.createElement("input"),
> +  attribute: "maxLength",
> +  nonNegative: true,
> +  defaultValue: -1,
> This is the expected default value for nonNegative, we shouldn't have to set it.

See comment on same section for Ms2ger

> AFAIK, tabindex isn't a full and simple content attribute reflection. 

Ah, if you want we can omit it. Actually it has the most issues and was the only reason I added a minValue and maxValue to reflectInt(), if we don't need to test that we can remove those input parameters.

> +      //TBD: Element that allows negative values not interpreting -2147483648 as a legitiate input
> Does that apply to something else than tabindex?

Nope, only found this issue with input.tabIndex.

> +    //TBD: Element .setAttributes() is trimming data from the input string as if using parseInt()
> You mean .setAttribute("  foo  ") and .getAttribute() returns "foo"?

Yup. Some examples of element.setAttribute() -> element.getAttribute() that I saw were:

"    111" -> "111"
"1234hello" -> "1234"
"1.2345" -> "1")
"why 567 what" -> "567"

> +        //TBD: Element attributes are not returning the correct default value for invalid inputs
> That applies to which attribute?

Only applies to input.tabIndex. It may be that this attribute is just starting with an incorrect defaultValue of -1 instead of 0.

> +        //TBD: Element does not throw INDEX_SIZE_ERROR correctly when given numeric input that is out of bounds
> That applies to which attribute?

Only applies to li.value.

Comment 21

[Ainsley Chong]

Couple more questions for Mounir:

> "1e5"

Interesting, element[attr] = "1e5" -> element[attr] == 100000. From reading the link below I did not expect that result. That that would happen. Is this the expected behavior?

On the other hand, setAttribute("1e5") -> element[attr] != 100000, seems inconsistent with the last behavior.

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#rules-for-parsing-integers

> value higher than PR_MAX
> value lower than PR_MIN

I did some searching and could not find references to PR_MAX and PR_MIN, what are these values?

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to Ainsley Chong from comment #21)
> Couple more questions for Mounir:
> 
> > "1e5"
> 
> Interesting, element[attr] = "1e5" -> element[attr] == 100000. From reading
> the link below I did not expect that result. That that would happen. Is this
> the expected behavior?

So, when you do element.attr = "1e5", it's using ECMAScript string conversion to number and I believe that "1e5" is equals to 100000 in that case.

> On the other hand, setAttribute("1e5") -> element[attr] != 100000, seems
> inconsistent with the last behavior.

So, here, we have to follow the "getting" rule from the specs and indeed "1e5" should be saved in the content attribute and "1e5" will be parsed as "1" according to rules for parsing an integer in the HTML specifications.

> > value higher than PR_MAX
> > value lower than PR_MIN
> 
> I did some searching and could not find references to PR_MAX and PR_MIN,
> what are these values?

Sorry, I meant PR_INT32_MAX, PR_INT32_MIN and PR_UINT32_MAX, see:
https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtypes.h

Comment 23

[Ainsley Chong]

Attachment: Patch_v5

If you two agree, I think the following should be filed as bugs:

1) li[value] = <invalid value> (ex. -222, 2147483649) is not throwing INDEX_SIZE_ERROR

2) {select.setAttribute(size), input.setAttribute(maxLength)} is trimming input strings, which can be seen be calling .getAttribute(). Examples:

"    111" -> "111"
"1234hello" -> "1234"
"1.2345" -> "1")
"why 567 what" -> "567"

3) {select[size], input[maxLength], li[value]} is not correctly reflecting exponential values set by .setAttribute(). Examples:

select.setAttribute(size, "1e5") -> select[size] == 0 instead of 1
select.setAttribute(size, "300e2") -> select[size] == 0 instead of 300

4) input[maxLength] starts with a default value of -1 instead of 0, which it should since input[maxLength] behaves like a non-negative integer.

Comment 24

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

(In reply to Ainsley Chong from comment #23)
> Created attachment 551986 [details] [diff] [review]
> Patch_v5
> 
> If you two agree, I think the following should be filed as bugs:
> 
> 1) li[value] = <invalid value> (ex. -222, 2147483649) is not throwing
> INDEX_SIZE_ERROR

I'm not sure where you got that. The requirement to throw an INDEX_SIZE_ERR exception only applies to attributes that are "limited to only non-negative numbers" (<http://www.whatwg.org/html/#reflect>). That isn't the case for li.value (<http://www.whatwg.org/html/#dom-li-value>). This patch seems to be missing the li test file, though?

> 2) {select.setAttribute(size), input.setAttribute(maxLength)} is trimming
> input strings, which can be seen be calling .getAttribute(). Examples:
> 
> "    111" -> "111"
> "1234hello" -> "1234"
> "1.2345" -> "1")
> "why 567 what" -> "567"

These are definitely bugs, but might be covered by bug 673820?

I need to think about 3 and 4.

volkmar: select.size seems to be unsigned per spec, do we have a bug for that?

Comment 25

[Mounir Lamouri (:mounir)]

(In reply to Ainsley Chong from comment #23)
> Created attachment 551986 [details] [diff] [review]
> Patch_v5
> 
> If you two agree, I think the following should be filed as bugs:
> 
> 1) li[value] = <invalid value> (ex. -222, 2147483649) is not throwing
> INDEX_SIZE_ERROR

I tend to say like Ms2ger here, we shouldn't throw INDEX_SIZE_ERROR for li.value.

> 2) {select.setAttribute(size), input.setAttribute(maxLength)} is trimming
> input strings, which can be seen be calling .getAttribute(). Examples:
> 
> "    111" -> "111"
> "1234hello" -> "1234"
> "1.2345" -> "1")
> "why 567 what" -> "567"

We should open a bug to fix that, indeed.

> 3) {select[size], input[maxLength], li[value]} is not correctly reflecting
> exponential values set by .setAttribute(). Examples:
> 
> select.setAttribute(size, "1e5") -> select[size] == 0 instead of 1
> select.setAttribute(size, "300e2") -> select[size] == 0 instead of 300

I believe bug 673820 might fix that.

> 4) input[maxLength] starts with a default value of -1 instead of 0, which it
> should since input[maxLength] behaves like a non-negative integer.

I'm not sure I understand, input.maxLength default value should be '-1' according to the specs and it seems to be what we do.

Comment 26

[Mounir Lamouri (:mounir)]

Likely, we will have to put some todo for bug 586761.

Comment 27

[Ainsley Chong]

> I'm not sure where you got that. The requirement to throw an INDEX_SIZE_ERR 
> exception only applies to attributes that are "limited to only non-negative 
> numbers" (<http://www.whatwg.org/html/#reflect>). That isn't the case for 
> li.value (<http://www.whatwg.org/html/#dom-li-value>).

Yeah that's true. However li[value] also does not actually accept the "invalid" values, Ex:

li[value] = -222 -> li[value] == 0
li[value] = 2147483649 -> li[value] == 0

> This patch seems to be missing the li test file, though?

When I call cat on the .patch it seems to be in there?

> 4) input[maxLength] starts with a default value of -1 instead of 0, which it
> should since input[maxLength] behaves like a non-negative integer.

> I'm not sure I understand, input.maxLength default value should be '-1' 
> according to the specs and it seems to be what we do.

Ah alright. But in that case input[maxLength] is returning the wrong default value when you set it to an invalid value. Ex:

input[maxLength] = "foo" -> input[maxLength] == 0 instead of -1

Also, it throws an error when you try to set it to a negative number:

input[maxLength] = -1 -> INDEX_SIZE_ERROR

Comment 28

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

(In reply to Ainsley Chong from comment #27)
> > I'm not sure where you got that. The requirement to throw an INDEX_SIZE_ERR 
> > exception only applies to attributes that are "limited to only non-negative 
> > numbers" (<http://www.whatwg.org/html/#reflect>). That isn't the case for 
> > li.value (<http://www.whatwg.org/html/#dom-li-value>).
> 
> Yeah that's true. However li[value] also does not actually accept the
> "invalid" values, Ex:
> 
> li[value] = -222 -> li[value] == 0
> li[value] = 2147483649 -> li[value] == 0

That's a bug, then.

> > This patch seems to be missing the li test file, though?
> 
> When I call cat on the .patch it seems to be in there?

It's mentioned in the Makefile, but I can't see the actual file.

> > 4) input[maxLength] starts with a default value of -1 instead of 0, which it
> > should since input[maxLength] behaves like a non-negative integer.
> 
> > I'm not sure I understand, input.maxLength default value should be '-1' 
> > according to the specs and it seems to be what we do.
> 
> Ah alright. But in that case input[maxLength] is returning the wrong default
> value when you set it to an invalid value. Ex:
> 
> input[maxLength] = "foo" -> input[maxLength] == 0 instead of -1
> 
> Also, it throws an error when you try to set it to a negative number:
> 
> input[maxLength] = -1 -> INDEX_SIZE_ERROR

Spec: <http://www.whatwg.org/html/#dom-input-maxlength>

> The maxLength IDL attribute must reflect the maxlength content
> attribute, limited to only non-negative numbers.

so that's correct.

Note that select.size changed in <http://html5.org/r/6455>.

Comment 29

[Mounir Lamouri (:mounir)]

(In reply to Ainsley Chong from comment #27)
> Yeah that's true. However li[value] also does not actually accept the
> "invalid" values, Ex:
> 
> li[value] = -222 -> li[value] == 0
> li[value] = 2147483649 -> li[value] == 0

It seems like a bug indeed. I've open bug 679653.

> > This patch seems to be missing the li test file, though?
> 
> When I call cat on the .patch it seems to be in there?

Did you call |hg add| ?

> Ah alright. But in that case input[maxLength] is returning the wrong default
> value when you set it to an invalid value. Ex:
> 
> input[maxLength] = "foo" -> input[maxLength] == 0 instead of -1

I think that might be a bug. I would except it to return -1. Ms2ger, do you confirm?

> Also, it throws an error when you try to set it to a negative number:
> 
> input[maxLength] = -1 -> INDEX_SIZE_ERROR

That's definitely what the specs require.

Ainsley, are you on IRC? I think that would be very helpful if we could talk about this bug on IRC.
More information about how to use IRC: https://wiki.mozilla.org/IRC
Come on #developers or #content and look for me ('volkmar'). Note that I might not reply given that I'm connected 24/7 but not always watching.

Comment 30

[Mounir Lamouri (:mounir)]

Comment on attachment 551986 [details] [diff] [review]
Patch_v5

Review of attachment 551986 [details] [diff] [review]:
-----------------------------------------------------------------

You might want to test:
- ol.start;
- textarea.maxlength.

You shouldn't test anymore:
- select.size (the specs have changed, it's now an 'unsigned long').

Ainsley, would you be interested in fixing the bugs you discovered?

I did a few comments that require an answer. Except that this test might be good to go (if it passes). Though, I'm not sure if we shouldn't try to fix the bugs before.

Anyway, it is going to require another pass so I'm removing the review request. Re-ask a review with the updated patch.

BTW, the coding style wants this:
if (condition) {
  // do
} else if (condition2) {
  // do
} else {
  // do
}
You do not follow that rule as far as I've seen.

Again, thanks for this work. You found a few interesting bugs and that's really awesome! I would recommend you to try to fix them to change your mind. It would also make writing this test less frustrating ;)

::: content/html/content/test/forms/test_input_attributes_reflection.html
@@ +114,5 @@
> +reflectInt({
> +  element: document.createElement("input"),
> +  attribute: "maxLength",
> +  nonNegative: true,
> +  defaultValue: -1,

No need of this, it should be default defaultValue when nonNegative is true.

::: content/html/content/test/reflect.js
@@ +532,5 @@
> +    // Test numeric inputs that are outside legal signed values but are accepted as the positive bit equivalent
> +    { value: -2147483649, stringified: "-2147483649", result: 2147483647 },
> +    { value: -3000000000, stringified: "-3000000000", result: 1294967296 },
> +    { value: -4294967296,  stringified: "-4294967296", result: 0 },
> +    // Test numeric inputs that are outside legal 32 bit signed values and throw an error when assigned

Should they? They might because setting INT32_MAX + n is equivalent to setting INT32_MIN + n - 1. If n is high enough, it will likely not throw.

@@ +579,5 @@
> +    { value: -Infinity, stringified: "-Infinity", result: NaN },
> +  ];
> +
> +  valuesToTest.forEach(function(v) {
> +    // Apply minValue and maxValue limits to expected results.

Maybe you could remove minValue and maxValue and just use nonNegative being true or false. As far as I know, that's the only way to have minValue changing. maxValue should be the same for all attributes I believe.

@@ +592,5 @@
> +    // Check if value is within allowed limits for the signed integers
> +    if((nonNegative ? isLimitedSignedInt : isSignedInt)(v.value)) {
> +      // Check if the value is an exponential value
> +      if (v.exp !== undefined) {
> +        //TBD: .setAttribute(exponential) -> incorrect reflection for element[attr]

I think we actually follow the specifications. I mean, we shouldn't return "10000" for "1e5" but "1". Currently, we are returning the default value because we think the value is invalid. Though, that's another bug.
I believe the algorithm to parse integers should probably take into account exponents. At least, I don't see why not given that float do allow 'e'.

@@ +600,5 @@
> +        is(element[attr], expectedResult, element.localName + ".setAttribute(" + attr + ", " + v.value + "), " +
> +          element.localName + "[" + attr + "] ");
> +      }
> +    } else {
> +      //TBD: Element attributes are not returning the correct default value for invalid inputs

Is that the case you mentioned about maxLength?

@@ +609,5 @@
> +        is(element[attr], defaultValue, element.localName + ".setAttribute(" + attr + ", " + v.value + "), " +
> +          element.localName + "[" + attr + "] ");
> +      }
> +    }
> +    //TBD: Element .setAttributes() is trimming data from the input string as if using parseInt()

I've open a bug for that.

@@ +613,5 @@
> +    //TBD: Element .setAttributes() is trimming data from the input string as if using parseInt()
> +    if (v.stringified != element.getAttribute(attr) && element.getAttribute(attr) == parseInt(v.value)) {
> +      todo_is(element.getAttribute(attr), v.stringified, element.localName + ".setAttribute(" + attr + ", " + v.value + "), " +
> +        element.localName + ".getAttribute(" + attr + ") ");
> +    //TBD: Element .setAttributes() is somehow able to parse "why 567 what" into "567"

Indeed, it should be the default value. I opened a bug for that.

@@ +626,5 @@
> +
> +    // Check if input is NaN or if parseInt returns NaN
> +    if(isNaN(v.value) || isNaN(parseInt(v.value))) {
> +        element[attr] = v.value;
> +        //TBD: Element attributes are not returning the correct default value for invalid inputs

What do you mean?

@@ +635,5 @@
> +          is(element[attr], defaultValue, element.localName + "[" + attr + "] = " + v.value + ", " +
> +            element.localName + "[" + attr + "] ");
> +        }
> +    // Check if input is a number outside the limits of signed integers
> +    } else if(!(nonNegative ? isDOMLimitedSignedInt : isDOMSignedInt)(v.value)) {

You probably meant isDOMLimitedSignedInt(v.value) instead of isDOMLimitedSignedInt.
And I would prefer:
} else if (nonNegative ? !isDOMLIimitedSignedInt(v.value) : !isDOMSignedInt(v.value)) {

It's a nit though.

@@ +638,5 @@
> +    // Check if input is a number outside the limits of signed integers
> +    } else if(!(nonNegative ? isDOMLimitedSignedInt : isDOMSignedInt)(v.value)) {
> +      try {
> +        element[attr] = v.value;
> +        //TBD: Element does not throw INDEX_SIZE_ERROR correctly when given numeric input that is out of bounds

When does that happen?

Comment 31

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

(In reply to Mounir Lamouri (:volkmar) from comment #29)
> (In reply to Ainsley Chong from comment #27)
> > Ah alright. But in that case input[maxLength] is returning the wrong default
> > value when you set it to an invalid value. Ex:
> > 
> > input[maxLength] = "foo" -> input[maxLength] == 0 instead of -1
> 
> I think that might be a bug. I would except it to return -1. Ms2ger, do you
> confirm?

No, ToNumber("foo") is NaN, and ToInt32 then returns +0.

Comment 32

[Ainsley Chong]

Sure, I'd be interested in fixing the bugs. I'll have a look after revising this patch.

Also, did input.maxLength also become a long since the last time I looked? Moving target, haha.

Comment 33

[Ainsley Chong]

Actually, it is just me or did every single int become a long instead?

http://www.w3.org/TR/DOM-Level-2-HTML/idl-definitions.html

Comment 34

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

There are no "int"s in IDL, just shorts and longs (not sure why). You really don't want to be looking at DOM2HTML though; please use <www.whatwg.org/html>.

Comment 35

[Ainsley Chong]

Attachment: Patch v6

Here's the next version of the patch.

Mounir: Thanks for putting up all the bugs! I labeled the todo checks in reflect.js file with the bug #'s to clear up any confusion about what the checks were looking for, I'm hoping that will answer the questions you had in your previous post about which is which.

I think all the strange behavior is accounted for in Bugzilla now, except for one that I'd like to confirm with you both.

It looks like when you set an IDL attribute to an invalid value (ex. element[attr] = "foo") the corresponding content attribute is always set to zero. When you try to recover either value it returns zero (element.getAttribute(attr) == 0, element[attr] == 0). 

According to the spec: "On setting, the given value must be converted to the shortest possible string representing the number as a valid integer and then that string must be used as the new content attribute value."

http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#reflecting-content-attributes-in-idl-attributes

Does this look right to you?

Comment 36

[Mounir Lamouri (:mounir)]

(In reply to Ainsley Chong from comment #35)
> It looks like when you set an IDL attribute to an invalid value (ex.
> element[attr] = "foo") the corresponding content attribute is always set to
> zero. When you try to recover either value it returns zero
> (element.getAttribute(attr) == 0, element[attr] == 0). 

This an expected behavior. Ms2ger explained that in comment 31.

> According to the spec: "On setting, the given value must be converted to the
> shortest possible string representing the number as a valid integer and then
> that string must be used as the new content attribute value."

Actually, the IDL attribute is expecting an integer so the string has to be converted to an integer before the part of the spec you quoted is actually run. The string conversion is done in JS the way Ms2ger explained.

Comment 37

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

Comment on attachment 555656 [details] [diff] [review]
Patch v6

>--- a/content/html/content/test/reflect.js
>+++ b/content/html/content/test/reflect.js

>+function reflectInt(aParameters)

>+  var defaultValue = aParameters.defaultValue !== undefined
>+                      ? aParameters.defaultValue : -1;

  var defaultValue = aParameters.defaultValue !== undefined
                      ? aParameters.defaultValue
                      : nonNegative ? -1 : 0;

>--- /dev/null
>+++ b/content/html/content/test/test_li_attributes_reflection.html
>@@ -0,0 +1,32 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <title>Test for HTMLLIElement attributes reflection</title>
>+  <script type="application/javascript" src="/MochiKit/packed.js"></script>

We don't use packed.js anymore, please remove this line.

>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="application/javascript" src="reflect.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+</head>
>+<body>
>+<p id="display"></p>
>+<div id="content" style="display: none">
>+</div>
>+<pre id="test">
>+<script type="application/javascript">
>+
>+/** Test for HTMLLIElement attributes reflection **/
>+
>+// TODO: add non-reflectInt() attributes.
>+
>+// .value
>+reflectInt({
>+  element: document.createElement("li"),
>+  attribute: "value",
>+  nonNegative: false,
>+  defaultValue: 0,

Remove this line; it won't be necessary with the change to reflect.js I suggested above.

>--- /dev/null
>+++ b/content/html/content/test/test_ol_attributes_reflection.html
>@@ -0,0 +1,32 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <title>Test for HTMLLIElement attributes reflection</title>

HTMLOLElement

>+  <script type="application/javascript" src="/MochiKit/packed.js"></script>

And here.

>+// .start
>+reflectInt({
>+  element: document.createElement("ol"),
>+  attribute: "start",
>+  nonNegative: false,
>+  defaultValue: 1,
>+});

Please add "// TODO: Bug 601912 - Test the reversed attribute." or something like that.

Haven't reviewed the rest of reflect.js yet.

Comment 38

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

--- a/content/html/content/test/reflect.js
+++ b/content/html/content/test/reflect.js
+function reflectInt(aParameters)
+{
+  function isSignedInt(value) {
+    var intValue = parseInt(value);
+    return (!isNaN(intValue) && intValue <= 2147483647 && intValue >= -2147483648);

Please make this

  var result = /^[ \t\n\f\r]*([\+\-]?[0-9]+)[\s\S]*$/.exec(value);
  if (!result) {
    // Failed to parse. Return an error.
    return false;
  }
  var intValue = parseInt(result[1]);
  return -0x80000000 <= intValue && intValue <= 0x7FFFFFFF;

and add a test whose value starts with a vertical tab (\v).

+  }
+
+  function isLimitedSignedInt(value) {
+    var intValue = parseInt(value);
+    return (!isNaN(intValue) && intValue <= 2147483647 && intValue >= 0);

This goes wrong for -0.

And here:

  var result = /^[ \t\n\f\r]*\+?([0-9]+)[\s\S]*$/.exec(value);
  if (!result) {
    // Failed to parse. Return an error.
    return false;
  }
  var intValue = parseInt(result[1]);
  return 0 <= intValue && intValue <= 0x7FFFFFFF;

(Mounir, could you review these two snippets?)

+
+  /**
+   * Test various values.
+   * value: The test value that will be set using both setAttribute(value) and
+   *        element[attr] = value
+   * stringified: The expected result for getAttribute() after setting

This is the result of calling ES ToString() on value.

+   * result: The expected value for element[attr] after setting

Document exp?

+   */
+  var valuesToTest = [
+  ];

Haven't reviewed these closely yet.

+  valuesToTest.forEach(function(v) {
+
+	element.setAttribute(attr, v.value);

A tab crept in here, should be four spaces.

// Test what the IDL attribute returns.

+    // Check if value is within allowed limits for the signed integers
+    if ((nonNegative ? isLimitedSignedInt : isSignedInt)(v.value)) {
+      // Check if the value is an exponential value
+      if (v.exp !== undefined) {
+        //TBD: Bug 673820: .setAttribute(exponential) -> incorrect reflection for element[attr]
+        todo_is(element[attr], v.exp, "Bug 673820: " + element.localName + ".setAttribute(" + attr + ", " + v.value + "), " +
+          element.localName + "[" + attr + "] ");
+      } else if (v.result < 0 && attr == "value") {
+        //TBD: Bug 679653: li.setAttribute(value, <negative number>) --> li[value] == 0 instead of <negative value>
+        todo_is(element[attr], v.result, "Bug 679653: li.setAttribute(value, " + v.result + ") --> li[value] == 0 instead of " + v.result);
+      } else if ((v.value == -2147483648 || v.value == "-2147483648") && element[attr] == defaultValue) {
+        //TBD: Bug 586761: .setAttribute(attr, -2147483648) --> element[attr] == defaultValue instead of -2147483648
+        todo_is(element[attr], v.result, "Bug 586761: " + element.localName + ".setAttribute(value, " +
+          v.value + "), " + element.localName + "[" + attr + "] ");
+      } else {
+        is(element[attr], v.result, element.localName + ".setAttribute(" + attr + ", " + v.value + "), " +
+          element.localName + "[" + attr + "] ");
+      }
+    } else {
+      if (v.value == "why 567 what") {
+        //TBD: Bug 679672: .setAttributes() is somehow able to parse "why 567 what" into "567"

s/setAttributes/setAttribute/. Also elsewhere.

+        todo_is(element[attr], defaultValue, "Bug 679672: " + element.localName + ".setAttribute(" + attr + ", " +
+          v.stringified + "), " + element.localName + "[" + attr + "] ");

s/v.stringified/v.value/ here, please.

+      } else {
+        is(element[attr], defaultValue, element.localName + ".setAttribute(" + attr + ", " + v.value + "), " +
+          element.localName + "[" + attr + "] ");
+      }
+    }

// Test what the content attribute returns.

+    if (v.stringified != element.getAttribute(attr) && v.value == "why 567 what") {

You need to remove 'v.stringified != element.getAttribute(attr)'; otherwise we won't realize when this starts passing.

+      //TBD: Bug 679672: .setAttributes() is somehow able to parse "why 567 what" into "567"
+      todo_is(element.getAttribute(attr), v.stringified, "Bug 679672: " + element.localName + ".setAttribute(" + attr + ", " +
+        v.value + "), " + element.localName + ".getAttribute(" + attr + ") ");
+    } else if (v.stringified != element.getAttribute(attr) && element.getAttribute(attr) == parseInt(v.value)) {

And here. I'm not sure about the parseInt here, though. Would === v.value.trim() work?

+      //TBD: Bug 679671: Element .setAttributes() is trimming data from the input string
+      todo_is(element.getAttribute(attr), v.stringified, "Bug 679671: " + element.localName + ".setAttribute(" + attr + ", " + v.value + "), " +
+        element.localName + ".getAttribute(" + attr + ") ");
+    } else {
+      is(element.getAttribute(attr), v.stringified, element.localName + ".setAttribute(" + attr + ", " + v.value + "), " +
+        element.localName + ".getAttribute(" + attr + ") ");
+    }
+    element.removeAttribute(attr);

I like what I've seen so far. I'll try to look at the other half later today or tomorrow.

Comment 39

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

Comment on attachment 555656 [details] [diff] [review]
Patch v6

+    // Check if input is NaN or if parseInt returns NaN
+    if (isNaN(v.value) || isNaN(parseInt(v.value))) {

I don't understand. Why isn't v.result 0 for these cases? Could you elaborate?

+        element[attr] = v.value;
+        is(element[attr], 0, element.localName + "[" + attr + "] = " + v.value + ", " +
+          element.localName + "[" + attr + "] ");
+        is(element.getAttribute(attr), 0, element.localName + "[" + attr + "] = " + v.value + ", " +
+          element.localName + ".getAttribute(" + attr + ") ");

Indentation is off.

+    // Check if input is a number outside the limits of signed integers
+    } else if (nonNegative ? !isDOMLimitedSignedInt(v.value) : !isDOMSignedInt(v.value)) {
+      try {
+        element[attr] = v.value;
+      } catch(e) {
+        is(e.code, DOMException.INDEX_SIZE_ERR, element.localName + "[" + attr + "] = " + v.value + " should throw NS_ERROR_DOM_INDEX_SIZE_ERR");
+      }

This isn't right. The only case where we should throw is nonNegative && ToInt32(v.value) < 0.

+    } else if (v.result < 0 && attr == "value") {
+        //TBD: Bug 679653: li.setAttribute(value, <negative number>) --> li[value] == 0 instead of <negative value>
+        todo_is(element[attr], v.result, "Bug 679653: li[value] = " + v.result + ") --> li[value] == 0 instead of " + v.result);

Indentation.

+    } else if ((v.value == -2147483648 || v.value == "-2147483648") && element[attr] == defaultValue) {

You can test v.result === -2147483648 here, right? And drop the element[attr] == defaultValue check.

+        //TBD: Bug 586761: .setAttribute(attr, -2147483648) --> element[attr] == defaultValue instead of -2147483648
+        todo_is(element[attr], v.result, "Bug 586761: " + element.localName + "[" + attr + "] = " + v.value + ", " +
+          element.localName + "[" + attr + "] ");

Indentation.

+    // Otherwise test value normally
+    } else {
+      element[attr] = v.value;
+      is(element[attr], v.result, element.localName + "[" + attr + "] = " + v.value + ", " +
+        element.localName + "[" + attr + "] ");
+      is(element.getAttribute(attr), v.result+'', element.localName + "[" + attr + "] = " + v.value + ", " +

s/v.result+''/String(v.result)/

+        element.localName + ".getAttribute(" + attr + ") ");
+    }
+    element.removeAttribute(attr);
+  });


+  /**
+   * Test various values.
+   * value: The test value that will be set using both setAttribute(value) and
+   *        element[attr] = value
+   * stringified: The expected result for getAttribute() after setting
+   * result: The expected value for element[attr] after setting

This doesn't work. The expected value is different for the two parts of the test. You'll need to check two different result values, one for the setAttribute() case, and one for the element[attr] case. The former might not need an extra property in this list, but could be extracted by something similar to isSignedInt/isLimitedSignedInt. This will also make the 'exp' properties unnecessary. The latter (which is the result of calling ES ToInt32() on value) will need updating, though. I've noted some cases where this is necessary below.

r- for this issue. I want you to address the other comments too, but those are pretty minor.

+   */
+  var valuesToTest = [
+    { value: -4294967297, stringified: "-4294967297", result: NaN },

result: -1

+    // Test non-numeric string inputs
+    { value: "", stringified: "", result: NaN },
+    { value: " ", stringified: " ", result: NaN },
+    { value: "+", stringified: "+", result: NaN },
+    { value: "-", stringified: "-", result: NaN },
+    { value: "foo", stringified: "foo", result: NaN },
+    { value: "+foo", stringified: "+foo", result: NaN },
+    { value: "-foo", stringified: "-foo", result: NaN },
+    { value: "+     foo", stringified: "+     foo", result: NaN },
+    { value: "hello1234", stringified: "hello1234", result: NaN },
+    { value: "1234hello", stringified: "1234hello", result: 1234 },
+    { value: "444 world 555",  stringified: "444 world 555", result: 444 },
+    { value: "why 567 what",  stringified: "why 567 what", result: NaN },
+    { value: "-3 nots",  stringified: "-3 nots", result: -3 },

result should be 0 for all these.

+    { value: "2e5",  stringified: "2e5", result: 200000, exp: 2 },
+    { value: "300e2",  stringified: "300e2", result: 30000, exp: 300 },

but is correct here

+    { value: "42+-$",  stringified: "42+-$", result: 42 },

and will need to be 0 here too.

+    // Test special values
+    { value: undefined, stringified: "undefined", result: NaN },
+    { value: null, stringified: "", result: NaN },
+    { value: NaN, stringified: "NaN", result: NaN },
+    { value: Infinity,  stringified: "Infinity", result: NaN },
+    { value: -Infinity, stringified: "-Infinity", result: NaN },

result should be 0 here.

I also want tests for "0x(hex digits)".

Comment 40

[Ainsley Chong]

Attachment: Patch v7

Here's the next version of the patch.

Taking the general suggestion from Ms2ger that all the expected values can be derived, I restructured the tests to depend on three function calls defined at the top that generate the expected output for the various test cases. This seems cleaner to me, especially once the bugs are fixed and when we are able to remove all the "TBD".

Also adding the new test cases requested my Ms2ger I think I found a couple additional bugs:

1) setAttribute(attr, "+42foo") -> element[attr] == defaultValue, should be 42.

2) setAttribute(attr, "0xFFFFF") -> element[attr] == defaultValue, should be 0.

Comment 41

[Mounir Lamouri (:mounir)]

Comment on attachment 557373 [details] [diff] [review]
Patch v7

Review of attachment 557373 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks pretty good, there are a lot of nits to fix but nothing huge.
It's good enough for r+ but I want to see the updated patch. Ms2ger could even review it to be sure we both agree ;)

f=mounir

::: content/html/content/test/forms/test_input_attributes_reflection.html
@@ +99,5 @@
>    otherValues: [ "_blank", "_self", "_parent", "_top" ],
>  });
>  
> +// .height (non-negative integer) This does not seem to exist
> +// Filed as bug at http://www.w3.org/Bugs/Public/show_bug.cgi?id=13385

We are going to implement it, changes that to:
// TODO: .height (non-negative integer), bug 683855
todo("height" in document.createElement('input'), "");

@@ +205,5 @@
>  todo("selectedOption" in document.createElement("input"),
>       "selectedOption isn't implemented yet");
>  
> +// .width (non-negative integer) This does not seem to exist
> +// Filed as bug at http://www.w3.org/Bugs/Public/show_bug.cgi?id=13385

We are going to implement it, changes that to:
// TODO: .width (non-negative integer), bug 683855
todo("width" in document.createElement('input'), "");

::: content/html/content/test/reflect.js
@@ +463,5 @@
> +function reflectInt(aParameters)
> +{
> +  function testExponential(value) {
> +    var result = /^[ \t\n\f\r]*([\+\-]?[0-9]+)e[0-9]+/.exec(value);
> +    return (!result ? false : true);

nit: you don't need the parenthesis.

@@ +464,5 @@
> +{
> +  function testExponential(value) {
> +    var result = /^[ \t\n\f\r]*([\+\-]?[0-9]+)e[0-9]+/.exec(value);
> +    return (!result ? false : true);
> +  }

Sic... Don't count on me to review that :) I hope the bug will be fixed soon and I will trust you in the mean time ;)

@@ +468,5 @@
> +  }
> +
> +  function trim(value) {
> +    return value.replace(/^\s+|\s+$/g, "");
> +  }

You do not seem to be using this method.

@@ +471,5 @@
> +    return value.replace(/^\s+|\s+$/g, "");
> +  }
> +
> +  // Expected result from element.setAttribute(attr, value) -> element.getAttribute()
> +  function contentGetAttributeResult(value) {

Could you rename this: "expectedGetAttributeResult" or "expectedContentAttribute" ?

@@ +476,5 @@
> +    if (value === null) {
> +      return "";
> +    } else {
> +      return value + "";
> +    }

nit:
no else after a return

@@ +480,5 @@
> +    }
> +  }
> +
> +  // Expected result from element.setAttribute(attr, value) -> element[attr]
> +  function contentAttrReflectionResult(value, nonNegative, defaultValue) {

Maybe it would be better to rename this method StringToInteger() ?

And could you put some comments?

@@ +489,5 @@
> +      }
> +      var intValue = /([\+\-]?[0-9]+)/.exec(result[1]);
> +      if (-0x80000000 <= intValue[1] && intValue[1] <= 0x7FFFFFFF) {
> +        return intValue[1];
> +      } else {

nit: no else after a return

@@ +499,5 @@
> +      }
> +      var intValue = /([\+\-]?[0-9]+)/.exec(result[1]);
> +      if (0 <= intValue[1] && intValue[1] <= 0x7FFFFFFF) {
> +        return intValue[1];
> +      } else {

nit: no else after a return

@@ +506,5 @@
> +    }
> +  }
> +
> +  // Expected result from element[attr] = value -> element[attr] and element.getAttribute()
> +  function idlAttrReflectionResult(value) {

I would rename this method "expectedIdlAttributeResult" or something similar.

@@ +510,5 @@
> +  function idlAttrReflectionResult(value) {
> +    if (isNaN(value) || isNaN(parseInt(value))) {
> +      return 0;
> +    } else {
> +      return value << 0;

That's an interesting hack (really ;)) but I don't think we really care and it's not clear, better to do:
return parseInt(value);

and nit: no else after a return.

@@ +609,5 @@
> +    { value: undefined },
> +    { value: null },
> +    { value: NaN },
> +    { value: Infinity },
> +    { value: -Infinity },

You could probably make this a simple array instead of an array of objects containing only one field.

@@ +616,5 @@
> +  valuesToTest.forEach(function(v) {
> +
> +    mContentGetAttributeResult = contentGetAttributeResult(v.value);
> +    mContentAttrReflectionResult = contentAttrReflectionResult(v.value, nonNegative, defaultValue);
> +    mIdlAttrReflectionResult = idlAttrReflectionResult(v.value);

Better to just call these methods when you need them in my opinion.

@@ +620,5 @@
> +    mIdlAttrReflectionResult = idlAttrReflectionResult(v.value);
> +
> +    element.setAttribute(attr, v.value);
> +
> +    if (mContentGetAttributeResult != element.getAttribute(attr) && v.value == "why 567 what") {

I don't think you should check that the value you got is different than the one you are expecting given that we know "why 567 what" should produce a wrong result. That way, when the result will change, the todo_is() will produce an "TEST_UNEXPECTED_PASS".
In other words, the condition should be: if (v.value == "why 567 what") {"

This comment might apply to other condition below.

@@ +625,5 @@
> +      //TBD: Bug 679672: .setAttribute() is somehow able to parse "why 567 what" into "567"
> +      todo_is(element.getAttribute(attr), mContentGetAttributeResult, "Bug 679672: " + element.localName + ".setAttribute(" +
> +        attr + ", " + v.value + "), " + element.localName + ".getAttribute(" + attr + ") ");
> +    } else if (mContentGetAttributeResult != element.getAttribute(attr) && element.getAttribute(attr) == parseInt(v.value)) {
> +      //TBD: Bug 679671: Element .setAttribute() is trimming data from the input string

It has been fixed :)

@@ +634,5 @@
> +        v.value + "), " + element.localName + ".getAttribute(" + attr + ") ");
> +    }
> +
> +    if (mContentAttrReflectionResult < 0 && attr == "value") {
> +      //TBD: Bug 679653: li.setAttribute(value, <negative number>) --> li[value] == 0 instead of <negative value>

It has been fixed too :)

@@ +650,5 @@
> +      //TBD: Bug 679672: .setAttribute() is somehow able to parse "why 567 what" into "567"
> +      todo_is(element[attr], mContentAttrReflectionResult, "Bug 679672: " + element.localName + ".setAttribute(" + attr + ", " +
> +        v.value + "), " + element.localName + "[" + attr + "] ");
> +    } else if (v.value == "+42foo") {
> +      //TBD: Bug: Unable to correctly parse "+" character in front of string

+ is parsed correctly try with "+42" but "+42foo" is not. I believe it will be fixed by one of the bugs in the dependency list.

@@ +669,5 @@
> +      if (nonNegative && mIdlAttrReflectionResult < 0) {
> +        ok(false, element.localName + "[" + attr + "] = " + v.value + " should throw NS_ERROR_DOM_INDEX_SIZE_ERR");
> +      } else {
> +        if (mIdlAttrReflectionResult < 0 && attr == "value") {
> +          //TBD: Bug 679653: li.setAttribute(value, <negative number>) --> li[value] == 0 instead of <negative value>

It has been fixed :)

@@ +684,5 @@
> +            element.localName + ".getAttribute(" + attr + ") ");
> +        }
> +      }
> +    } catch(e) {
> +      is(e.code, DOMException.INDEX_SIZE_ERR, element.localName + "[" + attr + "] = " + v.value + " should throw NS_ERROR_DOM_INDEX_SIZE_ERR");

Add a ok(nonNegative && mIdlAttrReflectionResult < 0, "");
We do not want to come here in other situations.

::: content/html/content/test/test_li_attributes_reflection.html
@@ +14,5 @@
> +<script type="application/javascript">
> +
> +/** Test for HTMLLIElement attributes reflection **/
> +
> +// TODO: add non-reflectInt() attributes.

There is only "value", so no need for this comment.

::: content/html/content/test/test_ol_attributes_reflection.html
@@ +14,5 @@
> +<script type="application/javascript">
> +
> +/** Test for HTMLOLElement attributes reflection **/
> +
> +// TODO: add non-reflectInt() attributes.

nit: there is only one missing attribute so just put:
// TODO: .type (String)

@@ +16,5 @@
> +/** Test for HTMLOLElement attributes reflection **/
> +
> +// TODO: add non-reflectInt() attributes.
> +
> +// TODO: Bug 601912 - Test the reversed attribute.

I would prefer:
// TODO: .reversed (boolean), bug 601912
todo("reversed" in document.createElement("ol"), "reversed is not yet implemented");

And, it would be better to keep the three attributes in alphabetical order.

@@ +23,5 @@
> +  element: document.createElement("ol"),
> +  attribute: "start",
> +  nonNegative: false,
> +  defaultValue: 1,
> +});

I think we might want to add more tests here, like:
var ol = document.createElement("ol");
var li = document.createElement("li");
ol.appendChild(li);
li.value = 42;
is(ol.start, 42, "");
li.value = -42;
is(ol.start, -42, "");
li.removeAttribute("value");
ol.removeAttribute("start");
ol.appendChild(document.createElement("li"));
ol.reversed = true;
todo_is(ol.start, 2, "");
li.value = 42;
todo_is(ol.start, 2, "");
ol.start = 42;
is(ol.start, 42, "");

Comment 42

[Mounir Lamouri (:mounir)]

Comment on attachment 557373 [details] [diff] [review]
Patch v7

Review of attachment 557373 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/test/forms/test_input_attributes_reflection.html
@@ +99,5 @@
>    otherValues: [ "_blank", "_self", "_parent", "_top" ],
>  });
>  
> +// .height (non-negative integer) This does not seem to exist
> +// Filed as bug at http://www.w3.org/Bugs/Public/show_bug.cgi?id=13385

Actually, you can remove this change, height is a string for HTMLInputElement and I have a patch implementing it in bug 683855.

@@ +205,5 @@
>  todo("selectedOption" in document.createElement("input"),
>       "selectedOption isn't implemented yet");
>  
> +// .width (non-negative integer) This does not seem to exist
> +// Filed as bug at http://www.w3.org/Bugs/Public/show_bug.cgi?id=13385

Same thing here.

Comment 43

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

(In reply to Mounir Lamouri (:volkmar) from comment #41)
> Comment on attachment 557373 [details] [diff] [review]
> @@ +510,5 @@
> > +  function idlAttrReflectionResult(value) {
> > +    if (isNaN(value) || isNaN(parseInt(value))) {
> > +      return 0;
> > +    } else {
> > +      return value << 0;
> 
> That's an interesting hack (really ;)) but I don't think we really care and
> it's not clear, better to do:
> return parseInt(value);

Objection. parseInt is the wrong algorithm to use here.

Comment 44

[Ainsley Chong]

Attachment: Patch v8

Cleaned up the items in Mounir's last couple posts. 

One comment:

> +    if (mContentGetAttributeResult != element.getAttribute(attr) && v.value == "why 567 what") {

> In other words, the condition should be: if (v.value == "why 567 what") {"

I tried this, and it does not quite work. This is because this bug does not happen for all element.attributes, for example it does not occur in li.value and ol.start. The extra "!=" is needed to prevent entering the bug case for these elements.

I have removed the extra "!=" when the bug applied to all tested element.attributes.

Comment 45

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

(In reply to Ainsley Chong from comment #44)
> Created attachment 558335 [details] [diff] [review]
> Patch v8
> 
> Cleaned up the items in Mounir's last couple posts. 
> 
> One comment:
> 
> > +    if (mContentGetAttributeResult != element.getAttribute(attr) && v.value == "why 567 what") {
> 
> > In other words, the condition should be: if (v.value == "why 567 what") {"
> 
> I tried this, and it does not quite work. This is because this bug does not
> happen for all element.attributes, for example it does not occur in li.value
> and ol.start. The extra "!=" is needed to prevent entering the bug case for
> these elements.
> 
> I have removed the extra "!=" when the bug applied to all tested
> element.attributes.

The point here was that the test doesn't actually test anything... Does it fail for all nonZero:true cases?  If so, please check that.

Comment 46

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

Comment on attachment 558335 [details] [diff] [review]
Patch v8

>+  function testExponential(value) {
>+    var result = /^[ \t\n\f\r]*[\+\-]?[0-9]+e[0-9]+/.exec(value);
>+    return (!result ? false : true);

return !!/^[ \t\n\f\r]*[\+\-]?[0-9]+e[0-9]+/.exec(value);

>+  function expectedGetAttributeResult(value) {
>+    var result = "";
>+    if (value !== null) {
>+      result = value + "";

I prefer String(value)

>+  function stringToInteger(value, nonNegative, defaultValue) {
>+    var returnVal = 0;
>+    // Parse: Ignore leading whitespace, find [+/-][numbers]
>+    var result = /^[ \t\n\f\r]*([\+\-]?[0-9]+)/.exec(value);

I don't think the [\+\-]? is applicable in the nonNegative===true case; could you verify?

> (defaultValue !== null ? defaultValue : 0);

You've got this expression a couple of times, please check if you can do it only once. Maybe change the contract to disallow passing null as defaultValue.

>+      } else {
>+        // Otherwise, trim whitespace and inspect integer value
>+        var intValue = /([\+\-]?[0-9]+)/.exec(result[1]);

Why is this necessary? Will result[1] and intValue be different here?

>+  function expectedIdlAttributeResult(value) {
>+    var result = 0;
>+    if (!isNaN(value) && !isNaN(parseInt(value))) {

I don't think you need this check; "return value << 0;" should work.

>+    try {
>+      element[attr] = v;
>+      if (nonNegative && expectedIdlAttributeResult(v) < 0) {
>+        ok(false, element.localName + "[" + attr + "] = " + v + " should throw NS_ERROR_DOM_INDEX_SIZE_ERR");
>+      } else {
>+      }
>+    } catch(e) {
>+      if (nonNegative && expectedIdlAttributeResult(v) < 0) {
>+        is(e.code, DOMException.INDEX_SIZE_ERR, element.localName + "[" + attr + "] = " + v +
>+          " should throw NS_ERROR_DOM_INDEX_SIZE_ERR");
>+      } else {
>+        ok(false, element.localName + "[" + attr + "] = " + v + " should not throw NS_ERROR_DOM_INDEX_SIZE_ERR");
>+      }
>+    }

I think this would be clearer as

if (nonNegative && expectedIdlAttributeResult(v) < 0) {
  try {} catch {}
} else {
  // Normal stuff
}

The test harness will catch unexpected exception.

Also; please don't add parseInt calls unless you know you're passing a simple integer string. None of the code paths we're testing here matches its behaviour in edge cases.

I'd like to look over the patch before it lands, but I'm away until the 15th. Mounir, it would be nice if you could review too.

Comment 47

[Ainsley Chong]

Attachment: Patch v9

Patch v9 should resolve the issues from Ms2ger's last 2 posts.

One comment:

> I don't think the [\+\-]? is applicable in the nonNegative===true case; could you verify?

Ok, I removed the \- for the nonNegative == true case. However I had to add a special case for values that lead with "-0", which should return 0 instead of the defaultValue of -1.

Comment 48

[Mounir Lamouri (:mounir)]

Comment on attachment 559816 [details] [diff] [review]
Patch v9

Review of attachment 559816 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, still no r+ because of the additional ol+li tests that doesn't look right.

::: content/html/content/test/reflect.js
@@ +465,5 @@
> +  function testExponential(value) {
> +    return !!/^[ \t\n\f\r]*[\+\-]?[0-9]+e[0-9]+/.exec(value);
> +  }
> +
> +  // Expected result from element.setAttribute(attr, value) -> element.getAttribute()

I would change this comment to:
// Expected value returned by .getAttribute() when |value| has been previously passed to .setAttribute().

@@ +474,5 @@
> +    }
> +    return result;
> +  }
> +
> +  // Expected result from element.setAttribute(attr, value) -> element[attr]

I think you can remove this comment.

@@ +503,5 @@
> +    }
> +    return returnVal;
> +  }
> +
> +  // Expected result from element[attr] = value -> element[attr] and element.getAttribute()

I would change this comment to:
// Expected value returned by .getAttribute(attr) or .attr if |value| has been set via the IDL attribute.

@@ +505,5 @@
> +  }
> +
> +  // Expected result from element[attr] = value -> element[attr] and element.getAttribute()
> +  function expectedIdlAttributeResult(value) {
> +    return value << 0;

Could you put a comment explaining that value << 0 is going to convert the string to an integer?

@@ +542,5 @@
> +    -2147483649, -3000000000, -4294967296, 2147483649, 4000000000, -4294967297,
> +    // Test string inputs with extra padding
> +    "     1111111", "  23456   ",
> +    // Test non-numeric string inputs
> +    "", " ", "+", "-", "foo", "+foo", "-foo", "+     foo", "hello1234", "1234hello", "444 world 555",

Could you add:
"-    foo" ?

@@ +575,5 @@
> +    } else if (v == "+42foo") {
> +      //TBD: Bug: Unable to correctly parse "+" character in front of string
> +      todo_is(element[attr], stringToInteger(v, nonNegative, defaultValue), "Bug: " + element.localName +
> +        ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
> +    } else if ((v == "0x10FFFF" || v == "-0xABCDEF") && element[attr] != 0) {

Why && element[attr] != 0?

@@ +585,5 @@
> +        ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
> +    }
> +    element.removeAttribute(attr);
> +
> +    if (nonNegative && expectedIdlAttributeResult(v) < 0) {

I don't think you should have a specific code path here.

You should probably do something like:
try {
  element[attr] = v;
  if (!nonNegative || expectedIdlAttributeResult >= 0) {
    ok(false, "a comment");
  }

  if (expectedIdlAttributeResult(v) == -2147483648 && element[attr] == defaultValue) {
    //TBD: Bug 586761: .setAttribute(attr, -2147483648) --> element[attr] == defaultValue instead of -2147483648
    todo_is(element[attr], expectedIdlAttributeResult(v), "Bug 586761: " + element.localName + "[" +
            attr + "] = " + v + ", " + element.localName + "[" + attr + "] ");
  } else {
    is(element[attr], expectedIdlAttributeResult(v), element.localName + "[" + attr + "] = " + v +
       ", " + element.localName + "[" + attr + "] ");
    is(element.getAttribute(attr), expectedIdlAttributeResult(v), element.localName + "[" + attr +
       "] = " + v + ", " + element.localName + ".getAttribute(" + attr + ") ");
  }
} catch (e) {
  ok(nonNegative && expectedIdlAttributeResult(v) < 0, "a test description");
  is(e.code, [...]);
}

::: content/html/content/test/test_ol_attributes_reflection.html
@@ +27,5 @@
> +});
> +
> +// TODO: .type (String)
> +
> +// Additional ol+li tests

For those tests, no need to test li.value.
In addition, in the comment I did, I put "" instead of the test description but you should put a description :)

@@ +33,5 @@
> +var li = document.createElement("li");
> +li.value = 42;
> +is(li.value, 42, "");
> +ol.appendChild(li);
> +is(ol.start, 1, "");

I think ol.start should be equals to 42 according to the specs.

@@ +36,5 @@
> +ol.appendChild(li);
> +is(ol.start, 1, "");
> +is(li.value, 42, "");
> +li.value = -42;
> +is(ol.start, 1, "");

As far as I understand the specs, ol.start should probably be -42.

@@ +39,5 @@
> +li.value = -42;
> +is(ol.start, 1, "");
> +is(li.value, -42, "");
> +ol.start = 3;
> +is(ol.start, 3, "");

You don't need to test that, reflectInt() is already checking that.

@@ +46,5 @@
> +ol.appendChild(document.createElement("li"));
> +ol.reversed = true;
> +todo_is(ol.start, 2, "");
> +li.value = 42;
> +todo_is(ol.start, 2, "");

It would be interesting to add here:
ol.start = 42;
is(ol.start, 42, "");

Comment 49

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

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #48)
> Comment on attachment 559816 [details] [diff] [review]
> @@ +33,5 @@
> > +var li = document.createElement("li");
> > +li.value = 42;
> > +is(li.value, 42, "");
> > +ol.appendChild(li);
> > +is(ol.start, 1, "");
> 
> I think ol.start should be equals to 42 according to the specs.
> 
> @@ +36,5 @@
> > +ol.appendChild(li);
> > +is(ol.start, 1, "");
> > +is(li.value, 42, "");
> > +li.value = -42;
> > +is(ol.start, 1, "");
> 
> As far as I understand the specs, ol.start should probably be -42.

"The reversed, start, and type IDL attributes must reflect the respective content attributes of the same name. The start IDL attribute has the same default as its content attribute."

"If the start attribute is present, user agents must parse it as an integer, in order to determine the attribute's value. The default value, used if the attribute is missing or if the value cannot be converted to a number according to the referenced algorithm, is 1 if the element has no reversed attribute, and is the number of child li elements otherwise."

So this seems correct.

Comment 50

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

Comment on attachment 559816 [details] [diff] [review]
Patch v9

--- a/content/html/content/test/reflect.js
+++ b/content/html/content/test/reflect.js
+function reflectInt(aParameters)
+{
+  function testExponential(value) {
+    return !!/^[ \t\n\f\r]*[\+\-]?[0-9]+e[0-9]+/.exec(value);
+  }

Add a note with the bug number here as well, please.

+  // Expected result from element.setAttribute(attr, value) -> element.getAttribute()
+  function expectedGetAttributeResult(value) {
+    var result = "";
+    if (value !== null) {
+      result = String(value)
+    }
+    return result;
+  }

This works, but isn't the most intuitive, IMO. How about

  return (value !== null) ? String(value) : "";

+  // Expected result from element.setAttribute(attr, value) -> element[attr]
+  function stringToInteger(value, nonNegative, defaultValue) {

defaultValue can't be null here, so don't handle that. Also, get rid of the returnVal local, just return the value directly.

+    var returnVal = 0;
+	} else if (/^[ \t\n\f\r]*(\-0)/.exec(value)) {

Fix the hard tab here, and afaict, you can drop the parens in the regex.

+        // Special case: nonNegative && value leads with "-0", return 0
+        returnVal = 0;
+    } else {
+	  returnVal = (defaultValue !== null ? defaultValue : -1);

And this tab.

+  valuesToTest.forEach(function(v) {

You're calling stringToInteger(v, nonNegative, defaultValue) a lot here; maybe add a local variable with the result.

+    if (nonNegative && expectedIdlAttributeResult(v) < 0) {

I *do* prefer the specific code path here, so please ignore Mounir ;)

With these nits and Mounir's comments addressed, I think this is good to go.

Comment 51

[Mounir Lamouri (:mounir)]

(In reply to Ms2ger from comment #49)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #48)
> > Comment on attachment 559816 [details] [diff] [review]
> > @@ +33,5 @@
> > > +var li = document.createElement("li");
> > > +li.value = 42;
> > > +is(li.value, 42, "");
> > > +ol.appendChild(li);
> > > +is(ol.start, 1, "");
> > 
> > I think ol.start should be equals to 42 according to the specs.
> > 
> > @@ +36,5 @@
> > > +ol.appendChild(li);
> > > +is(ol.start, 1, "");
> > > +is(li.value, 42, "");
> > > +li.value = -42;
> > > +is(ol.start, 1, "");
> > 
> > As far as I understand the specs, ol.start should probably be -42.
> 
> [..]
> 
> So this seems correct.

Indeed. Though, the other comments for ol/li tests still applies.

(In reply to Ms2ger from comment #50)
> +    if (nonNegative && expectedIdlAttributeResult(v) < 0) {
> 
> I *do* prefer the specific code path here, so please ignore Mounir ;)
> 
> With these nits and Mounir's comments addressed, I think this is good to go.

Ok... I'm not going to fight for this ;)

Comment 52

[Mounir Lamouri (:mounir)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #48)
> @@ +542,5 @@
> > +    -2147483649, -3000000000, -4294967296, 2147483649, 4000000000, -4294967297,
> > +    // Test string inputs with extra padding
> > +    "     1111111", "  23456   ",
> > +    // Test non-numeric string inputs
> > +    "", " ", "+", "-", "foo", "+foo", "-foo", "+     foo", "hello1234", "1234hello", "444 world 555",
> 
> Could you add:
> "-    foo" ?

And:
+-1, -+1, ++1, --1

Comment 53

[Mounir Lamouri (:mounir)]

Comment on attachment 559816 [details] [diff] [review]
Patch v9

Review of attachment 559816 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/test/reflect.js
@@ +575,5 @@
> +    } else if (v == "+42foo") {
> +      //TBD: Bug: Unable to correctly parse "+" character in front of string
> +      todo_is(element[attr], stringToInteger(v, nonNegative, defaultValue), "Bug: " + element.localName +
> +        ".setAttribute(" + attr + ", " + v + "), " + element.localName + "[" + attr + "] ");
> +    } else if ((v == "0x10FFFF" || v == "-0xABCDEF") && element[attr] != 0) {

I've been trying this patch and I never went here (and the test was passing). Do we really need this special casing? What is failing here?

BTW, I've been testing the patch in bug 673820 with those tests and I found two bugs thanks to two tests failures. That's awesome! :)

Comment 54

[Mounir Lamouri (:mounir)]

Comment 53 is about the hexadecimal special casing, not "+42foo".

Comment 55

[Ainsley Chong]

Attachment: Patch v10

Updated for last few comments.

I think there is another bug regarding double signs such as "++2" or "--2". From reading the spec it looks like we should only be able to parse a single sign [\+\-]?, and should otherwise return the defaultValue.

Instead we have the following behavior:

If nonNegative == false, then it parses the following:
"++2" -> 2
"+-2" -> -2
"--2" -> -2
"-+2" -> -2

If nonNegative == true, then it parses the following:
"++2" -> 2
"+-2" -> defaultValue
"--2" -> defaultValue
"-+2" -> defaultValue

Comment 56

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

Comment on attachment 560853 [details] [diff] [review]
Patch v10

--- a/content/html/content/test/reflect.js
+++ b/content/html/content/test/reflect.js
+function reflectInt(aParameters)
+{
+  function testExponential(value) {
+    return !!/^[ \t\n\f\r]*[\+\-]?[0-9]+e[0-9]+/.exec(value);
+  }

> Add a note with the bug number here as well, please.

+  function stringToInteger(value, nonNegative, defaultValue) {
+    } else if (/^[ \t\n\f\r]*\-0/.exec(value)) {
+        // Special case: nonNegative && value leads with "-0", return 0
+        return 0;

This is a bug per spec, right? Please file and note the bug number. And indentation is off.

+  valuesToTest.forEach(function(v) {
+

This blank line should go.

+    intValue = stringToInteger(v, nonNegative, defaultValue);

'var intValue'

+    //} else if ((v == "0x10FFFF" || v == "-0xABCDEF") && defaultValue != 0) {

This line should go entirely.

+    } else if ((v == "0x10FFFF" || v == "-0xABCDEF") && defaultValue != 0) {

+    } else {
+      is(element[attr], intValue,  element.localName +

Got two spaces before element.localName here.

With these changes, r=me.

Comment 57

[Mounir Lamouri (:mounir)]

Comment on attachment 560853 [details] [diff] [review]
Patch v10

Review of attachment 560853 [details] [diff] [review]:
-----------------------------------------------------------------

r=mounir with Ms2ger comments applied.

Ainsley, as soon as you will attach the updated patch, I will push it.

Comment 58

[Mounir Lamouri (:mounir)]

Patch sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=64ea3a5a6297

Comment 59

[Mounir Lamouri (:mounir)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #58)
> Patch sent to try:
> https://tbpl.mozilla.org/?tree=Try&rev=64ea3a5a6297

It seems to be green on try.

Comment 60

[Ainsley Chong]

Attachment: Patch v11

Next patch with fixes.

I separated the "-0" parsing error into it's own case.

Also I split out the exponential case two two separate cases with more specific if clauses. It sounds like these will be resolved shortly anyways with Atul's patch.

Comment 61

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

Comment on attachment 561384 [details] [diff] [review]
Patch v11

\o/
 |
/|

Comment 62

[Mounir Lamouri (:mounir)]

Pushed to mozilla-inbound. Should be merged to mozilla-central soon (takes usually less than a day).

Ainsley, this patch was hard and I believe it is your first patch. You didn't chose the easy path but you made it! Congratulations! :)

Comment 63

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/514ff3a903a6