Last Comment Bug 764481 - Add pref to enable landing of experimental forms features
: Add pref to enable landing of experimental forms features
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: Raphael Catolino (:raphc)
:
:
Mentors:
Depends on:
Blocks: 563637 446510 769385
  Show dependency treegraph
 
Reported: 2012-06-13 10:41 PDT by Raphael Catolino (:raphc)
Modified: 2012-12-28 11:56 PST (History)
3 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Only enable input type=number implementation if "dom.experimental_forms" is true. Add pref in relevant tests (46.45 KB, patch)
2012-06-13 11:01 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (5.10 KB, patch)
2012-06-19 11:43 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (14.74 KB, patch)
2012-06-19 11:47 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (15.61 KB, patch)
2012-06-20 11:44 PDT, Raphael Catolino (:raphc)
mounir: review+
Details | Diff | Splinter Review
patch (15.76 KB, patch)
2012-06-21 11:25 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review

Description Raphael Catolino (:raphc) 2012-06-13 10:41:39 PDT
Adding a pref to allow landing some experimental implementation of the input element, see Bug 563637
Comment 1 Raphael Catolino (:raphc) 2012-06-13 11:01:19 PDT
Created attachment 632769 [details] [diff] [review]
Only enable input type=number implementation if "dom.experimental_forms" is true. Add pref in relevant tests

Some test files in this patch are part of other bugs (Bug 635499, Bug 635553, Bug 636737, Bug 345624, Bug 345512, Bug 565538) and might not have landed yet.
Comment 2 Mounir Lamouri (:mounir) 2012-06-19 03:46:10 PDT
Comment on attachment 632769 [details] [diff] [review]
Only enable input type=number implementation if "dom.experimental_forms" is true. Add pref in relevant tests

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

I haven't reviewed the test changes because the indentation changes make them hard to read and the indentation change doesn't seem that useful. Could you try to have the smaller diff possible for those tests?

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2608,5 @@
>        if (success) {
>          newType = aResult.GetEnumValue();
> +        if (newType == NS_FORM_INPUT_NUMBER && !Preferences::GetBool("dom.experimental_forms",false)){
> +            newType = kInputDefaultType->value;
> +            aResult.SetTo(newType,nsnull);

I think you need to do:
aResult.SetTo(newType, aValue);

Otherwise, if you do:
input.type = 'number';
and do:
alert(input.type); // you will get 'text'
and:
alert(input.getAttribute('type')); // you will get 'text'

but you want to have "number" for getAttribute().

You should actually add a test for that.

::: content/html/content/test/forms/test_experimental_forms_pref.html
@@ +19,5 @@
> +<script type="application/javascript">
> +
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var input = document.getElementById('i');

Just for information, doing:
var input = document.createElement('input');

would have work well.

@@ +22,5 @@
> +
> +  var input = document.getElementById('i');
> +
> +  input.type="number";
> +  isnot(input.type,"number","");

nit: put spaces between '='.

But actually I would prefer if you were only checking the preference value here and adding another pushPrefEnv() with 'false'. The idea is that the preference defualt value will depend on the platform and if it happen that the test is run on a platform with a default value being 'true' it might be hard to understand why the test is failing.

@@ +26,5 @@
> +  isnot(input.type,"number","");
> +
> +  SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", true]]}, function() {
> +    input.type="number";
> +    is(input.type,"number","");

Put a comment.
Comment 3 Raphael Catolino (:raphc) 2012-06-19 11:43:52 PDT
Created attachment 634516 [details] [diff] [review]
patch

removed tabs (the diff might look better, but not the code...)
changed test-experimental-forms-pref.html according to comments.
added default value in /module/libpref/src/init/all.js
Comment 4 Raphael Catolino (:raphc) 2012-06-19 11:47:31 PDT
Created attachment 634520 [details] [diff] [review]
patch

Aaand wrong patch...
Comment 5 Mounir Lamouri (:mounir) 2012-06-19 12:26:34 PDT
Comment on attachment 634520 [details] [diff] [review]
patch

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

Mostly nits but I would like to see the patch with the fixes.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2606,5 @@
>        PRInt32 newType;
>        bool success = aResult.ParseEnumValue(aValue, kInputTypeTable, false);
>        if (success) {
>          newType = aResult.GetEnumValue();
> +        if (newType == NS_FORM_INPUT_NUMBER && !Preferences::GetBool("dom.experimental_forms",false)){

nit: Spaces after commas and lines should end at the 80th column.

::: content/html/content/test/forms/test_experimental_forms_pref.html
@@ +21,5 @@
> +
> +  SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({'set':[["dom.experimental_forms",false]]}, function(){
> +    input.type = "number";
> +    //If the experimental forms are disabled, input.type should not be number :

no need for that comment, it is paraphrasing the test description.

@@ +22,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({'set':[["dom.experimental_forms",false]]}, function(){
> +    input.type = "number";
> +    //If the experimental forms are disabled, input.type should not be number :
> +    isnot(input.type,"number","input type shouldn't be number when the experimental forms are disabled");

is(input.type, "text", "'number' should be ignored when experimental forms are disabled");

also, put spaces after commas.

@@ +23,5 @@
> +  SpecialPowers.pushPrefEnv({'set':[["dom.experimental_forms",false]]}, function(){
> +    input.type = "number";
> +    //If the experimental forms are disabled, input.type should not be number :
> +    isnot(input.type,"number","input type shouldn't be number when the experimental forms are disabled");
> +    //However the attribute 'type' is left unchanged :

no need for that comment too.

@@ +30,5 @@
> +    SpecialPowers.pushPrefEnv({'set':[["dom.experimental_forms",true]]}, function(){
> +      input.type = "text";
> +      input.type = "number";
> +      //If the experimentals forms are enabled, input.type should be number :
> +      is(input.type,"number","input type should be number when the experimental forms are enabled");

is(input.type, "number", "'number' should be allowed when experimental forms are enabled");

::: content/html/content/test/forms/test_min_attribute.html
@@ +160,5 @@
>  
> +SimpleTest.finish();
> +});
> +
> +SimpleTest.waitForExplicitFinish();

nit: put that before "pushPrefEnv".

::: modules/libpref/src/init/all.js
@@ +632,5 @@
>  // changed)
>  pref("dom.new_bindings", true);
>  pref("dom.experimental_bindings", true);
>  
> +//Don't use new input types

nit: space after "//"
Comment 6 Mounir Lamouri (:mounir) 2012-06-19 12:26:52 PDT
Raphael, you might want to have a look at:
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
Comment 7 Raphael Catolino (:raphc) 2012-06-20 11:44:24 PDT
Created attachment 634990 [details] [diff] [review]
patch
Comment 8 Mounir Lamouri (:mounir) 2012-06-21 06:24:32 PDT
Comment on attachment 634990 [details] [diff] [review]
patch

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

r=me with the changes listed below.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2607,5 @@
>        bool success = aResult.ParseEnumValue(aValue, kInputTypeTable, false);
>        if (success) {
>          newType = aResult.GetEnumValue();
> +        if (newType == NS_FORM_INPUT_NUMBER && 
> +            !Preferences::GetBool("dom.experimental_forms", false)){

nit: you are missing a space before "{"

::: content/html/content/test/forms/test_experimental_forms_pref.html
@@ +21,5 @@
> +
> +  SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", false]]}, function() {
> +    input.type = "number";
> +    isnot(input.type, "number", "input type shouldn't be number when the experimental forms are disabled");

is(input.type, "text", "...");

@@ +25,5 @@
> +    isnot(input.type, "number", "input type shouldn't be number when the experimental forms are disabled");
> +    is(input.getAttribute('type'), "number", "input 'type' attribute should not change");
> +
> +    SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms",true]]}, function() {
> +      input.type = "text";

No need for that AFAICT.
Comment 9 Raphael Catolino (:raphc) 2012-06-21 06:50:04 PDT
(In reply to Mounir Lamouri (:mounir) from comment #8)

> @@ +25,5 @@
> > +    isnot(input.type, "number", "input type shouldn't be number when the experimental forms are disabled");
> > +    is(input.getAttribute('type'), "number", "input 'type' attribute should not change");
> > +
> > +    SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms",true]]}, function() {
> > +      input.type = "text";
> 
> No need for that AFAICT.

You mean no need for 
>input.type = "text"; ?

If we only call 
>input.type = "number";
nsHTMLInputElement::ParseAttribute() doesn't get called and the new pref won't change the behavior of the input element. I figured it was due to some kind of optimization (don't re-parse the attribute if the attribute hasn't changed). And it looks like http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#5391 
does just that. (I haven't delved into the code too much, so I'm not sure though).
Comment 10 Mounir Lamouri (:mounir) 2012-06-21 06:52:03 PDT
Oh, that would be reasonable indeed.
input.type = 'Number' _might_ work (haven't checked the code) but between that and reverting to 'text', I would prefer the later.
In any case, can you put a comment ;)
Comment 11 Raphael Catolino (:raphc) 2012-06-21 11:25:02 PDT
Created attachment 635387 [details] [diff] [review]
patch
Comment 12 Mounir Lamouri (:mounir) 2012-06-21 11:38:16 PDT
Pushed to try with <input type='number'> support:
https://tbpl.mozilla.org/?tree=Try&rev=f47136316912
Comment 13 Mounir Lamouri (:mounir) 2012-06-23 07:24:15 PDT
Comment on attachment 635387 [details] [diff] [review]
patch

I forgot that bug 636627 hadn't an updated patch. I have updated it. As soon as I get a r+, I will push <input type='number'> behind that pref.
Comment 14 Mounir Lamouri (:mounir) 2012-07-05 08:58:24 PDT
https://hg.mozilla.org/mozilla-central/rev/ee2c5f2928b6

More information for dev-doc-needed:
For the moment <input type='number'> is disabled by default. You can enable it with the pref added by this patch. It will likely be enabled on Mobile (Android + B2G).

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