665866 - Test input.type reflection with reflect.js

Status: VERIFIED FIXED

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

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Updating the method in reflect.js, the element.dir reflection test and making input.type and button.type using it.

Comment 2

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

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

Just a couple of nits.

--- /dev/null
+++ b/content/html/content/test/forms/test_button_attributes_reflection.html
+// .type
--- /dev/null
+++ b/content/html/content/test/forms/test_input_attributes_reflection.html
+// .type

Doesn't really seem necessary.

--- a/content/html/content/test/reflect.js
+++ b/content/html/content/test/reflect.js
+  if (aDefaultValue === undefined) {
+    aDefaultValue = "";
+  }
+  if (aUnsupportedValues === undefined) {
+    aUnsupportedValues = [];
+  }

I'm don't like setting arguments much. How about
  var defaultValue = (aDefaultValue !== undefined) ? aDefaultValue : "";
  var unsupportedValues = (aUnsupportedValues !== undefined) ? aUnsupportedValues : [];

+
+  // Explicitely check default value.

"Explicitly", and "the default value".

+  aElement.removeAttribute(aAttr);
+  is(aElement[aAttr], aDefaultValue,
+     "When no attribute is set, the value should be the default value.");
+
+  // Check valid values.
+  aValidValues.forEach(function (v) {
     aElement.setAttribute(aAttr, v);
-    is(aElement[aAttr], v);
-    is(aElement.getAttribute(aAttr), v);
+    is(aElement[aAttr], v,
+       v + " should be accepted as a valid value for " + aAttr);
+    is(aElement.getAttribute(aAttr), v,
+       "Content attribute should return the value that has been set.");

"the value it has been set to"

     aElement.removeAttribute(aAttr);
 
     aElement.setAttribute(aAttr, v.toUpperCase());
-    is(aElement[aAttr], v);
-    is(aElement.getAttribute(aAttr), v.toUpperCase());
+    is(aElement[aAttr], v,
+       "Enumerated attributes should be case insensitive.");

"case-insensitive"

+    is(aElement.getAttribute(aAttr), v.toUpperCase(),
+       "Content attribute should be upper-cased.");

s/be upper-cased/not be lower-cased/

     aElement.removeAttribute(aAttr);
 
     aElement[aAttr] = v;
-    is(aElement[aAttr], v);
-    is(aElement.getAttribute(aAttr), v);
+    is(aElement[aAttr], v,
+       v + " should be accepted as a valid value for " + aAttr);
+    is(aElement.getAttribute(aAttr), v,
+       "Content attribute should return the value that has been set.");

As above.

     aElement.removeAttribute(aAttr);
 
     aElement[aAttr] = v.toUpperCase();
-    is(aElement[aAttr], v);
-    is(aElement.getAttribute(aAttr), v.toUpperCase());
+    is(aElement[aAttr], v,
+       "Enumerated attributes should be case insensitive.");
+    is(aElement.getAttribute(aAttr), v.toUpperCase(),
+       "Content attribute should be upper-cased.");

As above.

     aElement.removeAttribute(aAttr);
   });
-  ["cheesecake"].concat(aUnsupportedValues).forEach(function (v) {
+
+  // Check invalid values.
+  aInvalidValues.forEach(function (v) {
     aElement.setAttribute(aAttr, v);
-    is(aElement[aAttr], "");
-    is(aElement.getAttribute(aAttr), v);
+    is(aElement[aAttr], aDefaultValue,
+       "When the value is set to an invalid value, the default value should be returned.");

s/value/content attribute/

+    is(aElement.getAttribute(aAttr), v,
+       "Content attribute should not have been changed.");
     aElement.removeAttribute(aAttr);
 
     aElement[aAttr] = v;
-    is(aElement[aAttr], "");
-    is(aElement.getAttribute(aAttr), v);
+    is(aElement[aAttr], aDefaultValue,
+       "When the value is set to an invalid value, the default value should be returned.");

And here.

+  // Basicially, it's like the checks for the valid values but with some todo's.

"Basically"

+  aUnsupportedValues.forEach(function (v) {

Same nits. Now if only I'd thought of writing these myself... ;)

--- a/content/html/content/test/test_bug551670.html
+++ /dev/null
-function checkType(e1, e2, defaultType, otherType, wrongType)
-  e1.type = otherType;
-  e1.setAttribute('type', '');

No comment.

Looks good otherwise; r=me for what that's worth, and thanks for doing this.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
> Comment on attachment 542121 [details] [diff] [review] [review]
> Patch v1
> 
> Just a couple of nits.

We probably have a different definition of "a couple of nits" :)

> --- /dev/null
> +++ b/content/html/content/test/forms/test_button_attributes_reflection.html
> +// .type
> --- /dev/null
> +++ b/content/html/content/test/forms/test_input_attributes_reflection.html
> +// .type
> 
> Doesn't really seem necessary.

The idea is to test all attributes reflection in this file.

> +    is(aElement.getAttribute(aAttr), v.toUpperCase(),
> +       "Content attribute should be upper-cased.");
> 
> s/be upper-cased/not be lower-cased/

Same thing, really.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to comment #2)
>      aElement.removeAttribute(aAttr);
>    });
> -  ["cheesecake"].concat(aUnsupportedValues).forEach(function (v) {
> +
> +  // Check invalid values.
> +  aInvalidValues.forEach(function (v) {
>      aElement.setAttribute(aAttr, v);
> -    is(aElement[aAttr], "");
> -    is(aElement.getAttribute(aAttr), v);
> +    is(aElement[aAttr], aDefaultValue,
> +       "When the value is set to an invalid value, the default value should
> be returned.");
> 
> s/value/content attribute/
> 
> +    is(aElement.getAttribute(aAttr), v,
> +       "Content attribute should not have been changed.");
>      aElement.removeAttribute(aAttr);
>  
>      aElement[aAttr] = v;
> -    is(aElement[aAttr], "");
> -    is(aElement.getAttribute(aAttr), v);
> +    is(aElement[aAttr], aDefaultValue,
> +       "When the value is set to an invalid value, the default value should
> be returned.");
> 
> And here.

It's not the content attribute that has been set here.

Comment 5

[Mounir Lamouri (:mounir)]

Thanks for the fast review Ms2ger :)

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/663cc0ea79cc

Comment 7

[Ioana (away)]

All the tests listed here http://hg.mozilla.org/mozilla-central/rev/663cc0ea79cc now use the reflectLimitedEnumerated function from reflect.js to test input.type reflection. This can be verified by opening the above link and then opening the tests listed there.