Add pref to enable landing of experimental forms features

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: raphc, Assigned: raphc)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla16
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Adding a pref to allow landing some experimental implementation of the input element, see Bug 563637
(Assignee)

Comment 1

5 years ago
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.
Attachment #632769 - Flags: review?(mounir)
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.
Attachment #632769 - Flags: review?(mounir)
(Assignee)

Comment 3

5 years ago
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
Attachment #632769 - Attachment is obsolete: true
Attachment #634516 - Flags: review?(mounir)
(Assignee)

Comment 4

5 years ago
Created attachment 634520 [details] [diff] [review]
patch

Aaand wrong patch...
Attachment #634516 - Attachment is obsolete: true
Attachment #634516 - Flags: review?(mounir)
Attachment #634520 - Flags: review?(mounir)
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 "//"
Attachment #634520 - Flags: review?(mounir)
Raphael, you might want to have a look at:
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
(Assignee)

Comment 7

5 years ago
Created attachment 634990 [details] [diff] [review]
patch
Attachment #634520 - Attachment is obsolete: true
Attachment #634990 - Flags: review?(mounir)
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.
Attachment #634990 - Flags: review?(mounir) → review+
(Assignee)

Comment 9

5 years ago
(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 ;)
(Assignee)

Comment 11

5 years ago
Created attachment 635387 [details] [diff] [review]
patch
Attachment #634990 - Attachment is obsolete: true
Attachment #635387 - Flags: checkin?(mounir)
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.
Attachment #635387 - Flags: checkin?(mounir)
(Assignee)

Updated

5 years ago
Blocks: 769385
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).
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Blocks: 446510
You need to log in before you can comment on or make changes to this bug.