1278737 - Add 'month' to the list of valid types attributes for <input>

Status: RESOLVED FIXED

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

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8764834 [details] [diff] [review]
patch, v1.

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

Olli, may I have your review on this? If you're too busy, can you redirect to the right person? :) Thanks.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8764834 [details] [diff] [review]
patch, v1.

>@@ -5511,17 +5519,19 @@ HTMLInputElement::ParseAttribute(int32_t
>       bool success = aResult.ParseEnumValue(aValue, kInputTypeTable, false);
>       if (success) {
>         newType = aResult.GetEnumValue();
>         if ((IsExperimentalMobileType(newType) &&
>              !Preferences::GetBool("dom.experimental_forms", false)) ||
>             (newType == NS_FORM_INPUT_NUMBER &&
>              !Preferences::GetBool("dom.forms.number", false)) ||
>             (newType == NS_FORM_INPUT_COLOR &&
>-             !Preferences::GetBool("dom.forms.color", false))) {
>+             !Preferences::GetBool("dom.forms.color", false)) ||
>+            (IsDateTimeInputType(newType) &&
>+             !Preferences::GetBool("dom.forms.datetime", false))) {
Doesn't this break date etc. picker on mobile. Or explain why it doesn't do that.


IsExperimentalMobileType &&  !Preferences::GetBool("dom.experimental_forms", false)
is false there, since dom.experimental_forms is true, but
then 
IsDateTimeInputType(newType) && !Preferences::GetBool("dom.forms.datetime", false)
becomes true, since dom.forms.datetime is false.

You may want to take a look at HTMLInputElement::IsExperimentalMobileType, which is behaving a bit unusual way, but that anyhow got
changed recently when dom.forms.datepicker was added.

Comment 4

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #3)
> Comment on attachment 8764834 [details] [diff] [review]
> patch, v1.
> 
> >@@ -5511,17 +5519,19 @@ HTMLInputElement::ParseAttribute(int32_t
> >       bool success = aResult.ParseEnumValue(aValue, kInputTypeTable, false);
> >       if (success) {
> >         newType = aResult.GetEnumValue();
> >         if ((IsExperimentalMobileType(newType) &&
> >              !Preferences::GetBool("dom.experimental_forms", false)) ||
> >             (newType == NS_FORM_INPUT_NUMBER &&
> >              !Preferences::GetBool("dom.forms.number", false)) ||
> >             (newType == NS_FORM_INPUT_COLOR &&
> >-             !Preferences::GetBool("dom.forms.color", false))) {
> >+             !Preferences::GetBool("dom.forms.color", false)) ||
> >+            (IsDateTimeInputType(newType) &&
> >+             !Preferences::GetBool("dom.forms.datetime", false))) {
> Doesn't this break date etc. picker on mobile. Or explain why it doesn't do
> that.
> 
> 
> IsExperimentalMobileType &&  !Preferences::GetBool("dom.experimental_forms",
> false)
> is false there, since dom.experimental_forms is true, but
> then 
> IsDateTimeInputType(newType) && !Preferences::GetBool("dom.forms.datetime",
> false)
> becomes true, since dom.forms.datetime is false.
> 
> You may want to take a look at HTMLInputElement::IsExperimentalMobileType,
> which is behaving a bit unusual way, but that anyhow got
> changed recently when dom.forms.datepicker was added.

Yes, you're right, it may break data etc. picker on mobile, I'll upload a new patch to fix this. Thanks!

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Kind of messy now with all the prefs, but I think dom.forms.datepicker will eventually merge with dom.forms.datetime.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8766187 [details] [diff] [review]
patch, v2.



>+  bool DoesStepApply() const {
...

>-  bool DoesValueAsNumberApply() const { return DoesMinMaxApply(); }
>+  bool DoesValueAsNumberApply() const {
nit, { goes to its own line when the method body isn't in the same line as method name.






>+++ b/dom/html/test/forms/test_experimental_forms_pref.html
>@@ -15,37 +15,62 @@ https://bugzilla.mozilla.org/show_bug.cg
> <div id="content" style="display: none" >
> </div>
> <pre id="test">
> <script type="application/javascript">
> 
>   var input = document.createElement("input");
> 
>   SimpleTest.waitForExplicitFinish();
>-  SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", false], ["dom.forms.datepicker",false]]}, function() {
>-    input.type = "date";
>-    is(input.type, "text", "input type shouldn't be date when the experimental forms are disabled");
>-    is(input.getAttribute('type'), "date", "input 'type' attribute should not change");
>+  SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", false],
>+    ["dom.forms.datepicker", false], ["dom.forms.datetime", false]]},
>+    function() {
>+      input.type = "date";
>+      is(input.type, "text",
>+         "input type shouldn't be date when the experimental forms are disabled");
>+      is(input.getAttribute('type'), "date",
>+         "input 'type' attribute should not change");
> 
>-    SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms",true], ["dom.forms.datepicker",false]]}, function() {
>-      // Change the type of input to text and then back to date,
>-      // so that HTMLInputElement::ParseAttribute gets called with the pref enabled.
>-      input.type = "text";
>-      input.type = "date";
>-      is(input.type, "date", "input type should be date when the experimental forms are enabled");
>-      is(input.getAttribute('type'), "date", "input 'type' attribute should not change");
>+      SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", true],
>+        ["dom.forms.datepicker", false], ["dom.forms.datetime", false]]},
>+        function() {
>+          // Change the type of input to text and then back to date,
>+          // so that HTMLInputElement::ParseAttribute gets called with the pref enabled.
>+          input.type = "text";
>+          input.type = "date";
>+          is(input.type, "date",
>+             "input type should be date when the experimental forms are enabled");
>+          is(input.getAttribute('type'), "date",
>+             "input 'type' attribute should not change");
> 
>-      SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms",false], ["dom.forms.datepicker",true]]}, function() {
>-        // Change the type of input to text and then back to date,
>-        // so that HTMLInputElement::ParseAttribute gets called with the pref enabled.
>-        input.type = "text";
>-        input.type = "date";
>-        is(input.type, "date", "input type should be date when the datepicker is enabled");
>-        is(input.getAttribute('type'), "date", "input 'type' attribute should not change");
>+          SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", false],
>+            ["dom.forms.datepicker", true], ["dom.forms.datetime", false]]},
>+            function() {
>+              // Change the type of input to text and then back to date,
>+              // so that HTMLInputElement::ParseAttribute gets called with the pref enabled.
>+              input.type = "text";
>+              input.type = "date";
>+              is(input.type, "date",
>+                 "input type should be date when the datepicker is enabled");
>+              is(input.getAttribute('type'), "date",
>+                 "input 'type' attribute should not change");
> 
>-        SimpleTest.finish();
>+              SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", false],
>+                ["dom.forms.datepicker", false], ["dom.forms.datetime", true]]},
>+                function() {
>+                  // Change the type of input to text and then back to date,
>+                  // so that HTMLInputElement::ParseAttribute gets called with the pref enabled.
>+                  input.type = "text";
>+                  input.type = "date";
>+                  is(input.type, "date",
>+                     "input type should be date when the datetime is enabled");
>+                  is(input.getAttribute('type'), "date",
>+                     "input 'type' attribute should not change");
>+
>+                  SimpleTest.finish();
>+              });
>+          });
>       });
>-    });
Would be nice to have this test to be converted some generic test testing all input types which depend on prefs. Could be done in some other bug too when adding support for new types.


Pref handling is indeed a bit messy. Perhaps there is some way to merge IsExperimentalMobileType and IsDateTimeEnabled - if there is, that can be done in a separate bug.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

Thanks Olli, I decided to add the test for testing all input types which depend on prefs in this bug, hope it's better now.
Since I changed the tests a bit, re-asking your review.

Comment 8

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0324f68730d4

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/444a4a7233f7
Add 'month' to the list of valid types attributes for <input>. r=smaug

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequent web platform test 2 failure on OSX in form-validation-reportValidity.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/304be069e80b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=dd2cdd2663620b0ee532786e5609bdddce6aa847
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=31075820&repo=mozilla-inbound

09:10:05     INFO - TEST-PASS | /html/semantics/forms/constraints/form-validation-reportValidity.html | [INPUT in MONTH status] no constraint 
09:10:05     INFO - TEST-PASS | /html/semantics/forms/constraints/form-validation-reportValidity.html | [INPUT in MONTH status] no constraint (in a form) 
09:10:05     INFO - TEST-UNEXPECTED-FAIL | /html/semantics/forms/constraints/form-validation-reportValidity.html | [INPUT in MONTH status] suffering from an overflow - assert_false: The reportValidity method should be false. expected false got true
09:10:05     INFO - validator.test_reportValidity/<@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:232:9
09:10:05     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1398:20
09:10:05     INFO - test@http://web-platform.test:8000/resources/testharness.js:496:9
09:10:05     INFO - validator.test_reportValidity@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:215:5
09:10:05     INFO - validator.run_test@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:357:13
09:10:05     INFO - @http://web-platform.test:8000/html/semantics/forms/constraints/form-validation-reportValidity.html:144:3
09:10:05     INFO - 
09:10:05     INFO - TEST-UNEXPECTED-FAIL | /html/semantics/forms/constraints/form-validation-reportValidity.html | [INPUT in MONTH status] suffering from an overflow (in a form) - assert_false: The reportValidity method of the element's form owner should return false. expected false got true
09:10:05     INFO - validator.test_reportValidity/<@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:254:9
09:10:05     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1398:20
09:10:05     INFO - test@http://web-platform.test:8000/resources/testharness.js:496:9
09:10:05     INFO - validator.test_reportValidity@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:237:5
09:10:05     INFO - validator.run_test@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:357:13
09:10:05     INFO - @http://web-platform.test:8000/html/semantics/forms/constraints/form-validation-reportValidity.html:144:3
09:10:05     INFO - 
09:10:05     INFO - TEST-UNEXPECTED-FAIL | /html/semantics/forms/constraints/form-validation-reportValidity.html | [INPUT in MONTH status] suffering from an underflow - assert_false: The reportValidity method should be false. expected false got true
09:10:05     INFO - validator.test_reportValidity/<@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:232:9
09:10:05     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1398:20
09:10:05     INFO - test@http://web-platform.test:8000/resources/testharness.js:496:9
09:10:05     INFO - validator.test_reportValidity@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:215:5
09:10:05     INFO - validator.run_test@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:357:13
09:10:05     INFO - @http://web-platform.test:8000/html/semantics/forms/constraints/form-validation-reportValidity.html:144:3
09:10:05     INFO - 
09:10:05     INFO - TEST-UNEXPECTED-FAIL | /html/semantics/forms/constraints/form-validation-reportValidity.html | [INPUT in MONTH status] suffering from an underflow (in a form) - assert_false: The reportValidity method of the element's form owner should return false. expected false got true
09:10:05     INFO - validator.test_reportValidity/<@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:254:9
09:10:05     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1398:20
09:10:05     INFO - test@http://web-platform.test:8000/resources/testharness.js:496:9
09:10:05     INFO - validator.test_reportValidity@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:237:5
09:10:05     INFO - validator.run_test@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:357:13
09:10:05     INFO - @http://web-platform.test:8000/html/semantics/forms/constraints/form-validation-reportValidity.html:144:3
09:10:05     INFO - 
09:10:05     INFO - TEST-UNEXPECTED-FAIL | /html/semantics/forms/constraints/form-validation-reportValidity.html | [INPUT in MONTH status] suffering from a step mismatch - assert_false: The reportValidity method should be false. expected false got true
09:10:05     INFO - validator.test_reportValidity/<@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:232:9
09:10:05     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1398:20
09:10:05     INFO - test@http://web-platform.test:8000/resources/testharness.js:496:9
09:10:05     INFO - validator.test_reportValidity@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:215:5
09:10:05     INFO - validator.run_test@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:357:13
09:10:05     INFO - @http://web-platform.test:8000/html/semantics/forms/constraints/form-validation-reportValidity.html:144:3
09:10:05     INFO - 
09:10:05     INFO - TEST-UNEXPECTED-FAIL | /html/semantics/forms/constraints/form-validation-reportValidity.html | [INPUT in MONTH status] suffering from a step mismatch (in a form) - assert_false: The reportValidity method of the element's form owner should return false. expected false got true
09:10:05     INFO - validator.test_reportValidity/<@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:254:9
09:10:05     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1398:20
09:10:05     INFO - test@http://web-platform.test:8000/resources/testharness.js:496:9
09:10:05     INFO - validator.test_reportValidity@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:237:5
09:10:05     INFO - validator.run_test@http://web-platform.test:8000/html/semantics/forms/constraints/support/validator.js:357:13
09:10:05     INFO - @http://web-platform.test:8000/html/semantics/forms/constraints/form-validation-reportValidity.html:144:3
09:10:05     INFO - .........................................
09:10:05     INFO - TEST-TIMEOUT | /html/semantics/forms/constraints/form-validation-reportValidity.html | took 15168ms

Comment 11

[Jessica Jong [:jessica] (inactive)]

Oh, didn't catch this since the test "form-validation-reportValidity.html" is expected to timeout [1], but sometimes it's still able to ran some of the tests, so need to update the wpt expectation  data accordingly. Will upload a new patch to fix this.

[1] http://hg.mozilla.org/mozilla-central/file/39dffbba7642/testing/web-platform/meta/html/semantics/forms/constraints/form-validation-reportValidity.html.ini#l3

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v4.

fixed form-validation-reportValidity.html, carrying r+.

Comment 13

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28dcc677a7de

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/469a7975c195
Add 'month' to the list of valid types attributes for <input>. r=smaug

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/469a7975c195