The default bug view has changed. See this FAQ.

Test input.type reflection with reflect.js

VERIFIED FIXED in mozilla7

Status

()

Core
DOM: Core & HTML
--
minor
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: mounir)

Tracking

Trunk
mozilla7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
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.
Attachment #542121 - Flags: review?(Ms2ger)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]
(Reporter)

Comment 2

6 years ago
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.
Attachment #542121 - Flags: review?(Ms2ger) → review+
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Thanks for the fast review Ms2ger :)
Flags: in-testsuite+
Whiteboard: [needs review] → [inbound]
http://hg.mozilla.org/mozilla-central/rev/663cc0ea79cc
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7

Comment 7

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.