Adding a pref to allow landing some experimental implementation of the input element, see Bug 563637

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 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.

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

Aaand wrong patch...

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 "//"

Raphael, you might want to have a look at: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

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.

(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).

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 ;)

Pushed to try with <input type='number'> support: https://tbpl.mozilla.org/?tree=Try&rev=f47136316912

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.

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).