828938 - Change test_input_sanitization.html type='date' handling to be like other types

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

Basically, this test mimic the sanitization in JS. That way, we can test a lot of values for each types. Though, we might want to clean that up because the tests has around 25K tests now ;)

Comment 1

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

Comment on attachment 700320 [details] [diff] [review]
Patch

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

Looks good, but I'd like to see how you deal with the new tests I suggested ;)

::: content/html/content/test/forms/test_input_sanitization.html
@@ +40,5 @@
>    "text", "search", "url", "tel", "email", "password", "date", "datetime",
>    "month", "week", "time", "datetime-local", "number", "range", "color",
>  ];
>  
> +function sanitizeValue(aType, aValue)

Pointer to <http://www.whatwg.org/html/#value-sanitization-algorithm>? Actually, that's a rubbish link, but better than nothing, I guess.

@@ +50,5 @@
> +    case "tel":
> +      return aValue.replace(/[\n\r]/g, "");
> +    case "url":
> +    case "email":
> +      return aValue.replace(/[\n\r]/g, "").replace(/^\s+|\s+$/g, "");

I don't think \s is correct. Please add some tests that include \u000B, \u00A0, \u2028, \u2029 and \uFEFF :)

@@ +52,5 @@
> +    case "url":
> +    case "email":
> +      return aValue.replace(/[\n\r]/g, "").replace(/^\s+|\s+$/g, "");
> +    case "number":
> +      return (parseFloat(aValue) + "" === aValue) ? aValue : "";

This may even be correct... Please use String(parseFloat(aValue)), though.

Actually, I don't think it is correct; please test exponential notation with lower and upper case 'e'.

@@ +55,5 @@
> +    case "number":
> +      return (parseFloat(aValue) + "" === aValue) ? aValue : "";
> +    case "date":
> +      function getNumbersOfDaysInMonth(aMonth, aYear) {
> +        if (aMonth == 2) {

s/==/===/g

@@ +75,5 @@
> +      if (month > 12 || month < 1) {
> +        return "";
> +      }
> +      var day = match[3];
> +      return day == 0 || getNumbersOfDaysInMonth(month, year) < day ? "" : aValue;

Wrap this around as

return 1 <= day && day <= getNumbersOfDaysInMonth(month, year) ? aValue : "";

@@ +77,5 @@
> +      }
> +      var day = match[3];
> +      return day == 0 || getNumbersOfDaysInMonth(month, year) < day ? "" : aValue;
> +    case "time":
> +      return ""; 

Trailing whitespace, and add a TODO.

And for all the TODO cases, add ok(false).

@@ +97,5 @@
> +}
> +
> +function checkSanitizing(element)
> +{
> +  if (element.type == 'time') {

This seems like a strange check. Don't we need to skip the other datetimes/range/color too?

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to :Ms2ger from comment #1)
> @@ +50,5 @@
> > +    case "tel":
> > +      return aValue.replace(/[\n\r]/g, "");
> > +    case "url":
> > +    case "email":
> > +      return aValue.replace(/[\n\r]/g, "").replace(/^\s+|\s+$/g, "");
> 
> I don't think \s is correct. Please add some tests that include \u000B,
> \u00A0, \u2028, \u2029 and \uFEFF :)
> 
> @@ +52,5 @@
> > +    case "url":
> > +    case "email":
> > +      return aValue.replace(/[\n\r]/g, "").replace(/^\s+|\s+$/g, "");
> > +    case "number":
> > +      return (parseFloat(aValue) + "" === aValue) ? aValue : "";
> 
> This may even be correct... Please use String(parseFloat(aValue)), though.
> 
> Actually, I don't think it is correct; please test exponential notation with
> lower and upper case 'e'.

If you do not mind, I would prefer to keep those two changes out of this patch to keep things sane. I will definitely open a follow-up for those.

> This seems like a strange check. Don't we need to skip the other datetimes/range/color too?

It's simply because <input type='time'> is recognized as a valid type but the value sanitization isn't done yet. The patch is being worked on though (bug 781569).

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 700459 [details] [diff] [review]
Patch

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

::: content/html/content/test/forms/test_input_sanitization.html
@@ +78,5 @@
> +      }
> +      var day = match[3];
> +      return 1 <= day && day <= getNumbersOfDaysInMonth(month, year) ? aValue : "";
> +    case "time":
> +      return ""; 

I forgot the trailing whitespace but it is being removed in bug 781569.

Comment 5

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

Comment on attachment 700459 [details] [diff] [review]
Patch

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

Alright, alright. But don't forget the followup :)

Comment 6

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/060da3a0e960

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/060da3a0e960