Last Comment Bug 665866 - Test input.type reflection with reflect.js
: Test input.type reflection with reflect.js
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla7
Assigned To: Mounir Lamouri (:mounir)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-21 05:27 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-09-08 06:31 PDT (History)
2 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (14.80 KB, patch)
2011-06-27 04:10 PDT, Mounir Lamouri (:mounir)
Ms2ger: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-06-21 05:27:56 PDT

    
Comment 1 Mounir Lamouri (:mounir) 2011-06-27 04:10:44 PDT
Created attachment 542121 [details] [diff] [review]
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 (⌚ UTC+1/+2) 2011-06-27 04:38:37 PDT
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) 2011-06-27 06:24:11 PDT
(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) 2011-06-27 06:29:13 PDT
(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) 2011-06-27 06:34:13 PDT
Thanks for the fast review Ms2ger :)
Comment 7 Ioana (away) 2011-09-08 06:31:15 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.