557620 - Implement <input type="tel">

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

The input element telephone state is very similar to search and text states. There is no validation mechanism because telephone numbers are quite different between countries.

Comment 1

[d]

For input elements, like the telephone state, where there are no visual changes (unless we come up with something nice), what does actually need to be implemented?

Comment 2

[Mounir Lamouri (:mounir)]

Make sure the type is recognized and make it works correctly.
There is also "User agents may change the punctuation of values that the user enters." even if I do not really get what that means... I should ask the working group.
In my opinion, we can add a specific UI like the search state may have a magnifying glasses, this one may have a telephone icon. We will see.

Comment 3

[d]

(In reply to comment #2)
> Make sure the type is recognized and make it works correctly.
> There is also "User agents may change the punctuation of values that the user
> enters." even if I do not really get what that means... I should ask the
> working group.
> In my opinion, we can add a specific UI like the search state may have a
> magnifying glasses, this one may have a telephone icon. We will see.

I see, and yes, an UI-indication would be good. About the punctuation, I think it is the ways phone numbers can be split up. For example:

"555-55-55-55"
"5555-55-55-55"

I think it depends on if it's a home-number or a cellphone number in those cases. The iPhone splits them up in different ways while being typed in, depending on the length I think. If we want to implement something similar, we could look at that system (or any other product that splits them up this way).

Still, perhaps you should ask the working group to be sure that this is actually the kind of "punctuation" that they are talking about.

(P.S. I am no good with phone numbers, those examples might be very odd, but should give you an idea of what I mean.)

Comment 4

[Mounir Lamouri (:mounir)]

I do not think it is related to splitting. At least, I hope it isn't because the value will probably be checked by the webpage to make sure it is valid. Depending on countries, phone numbers can have 3-digit blocks or 2-digit blocks or even more complex structure. So, the UA can't add '-' itself.

Anyway, I've just send a message to the WG.

Comment 5

[Ed Lee :Mardak]

volkmar, are these "Implement" bugs not doing anything with form fill? Are there separate bugs elsewhere to handle smarter auto-completion for these fields?

The Contacts Labs add-on stores contact information like names, emails, telephone numbers, etc. Right now we only autofill email addresses, but telephone, etc would be pretty doable too.

Comment 6

[Mounir Lamouri (:mounir)]

Actually, email and telephone bugs I've opened are about having these types working as expected by the specs. You may want to open a new bug related to autocompletion with Contacts Labs data depending on each of them.

By the way, you should know there is a bug related to the new autocompletion attribute (bug 557628). You don't need it for the feature you want but you should know autocompletion can be disabled with these new attribute.

If you open new bugs for autocompletion with Contacts Labs data, please add me in CC.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Tests v1

Like search type (bug 456229), I don't know how I should test that. I would have too many things to test if I would like to test it correctly.
Olli, if you have any recommendation.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This patch is adding the 'tel' state for the input element.
I launched a discussion about the punctuation change on whatwg ML. I do not think it should be done and it looks like I'm not alone so this patch is not doing anything related to that. Anyway, it is optional so...

The style of the telephone input should be done in a follow-up I think.

Comment 9

[Mounir Lamouri (:mounir)]

By the way, Magne, you were right about "User agents may change the punctuation of values that the user enters."... But I think doing that would be a bad idea.

Comment 10

[d]

(In reply to comment #9)
> By the way, Magne, you were right about "User agents may change the punctuation
> of values that the user enters."... But I think doing that would be a bad idea.

I agree. It might create confusion between the user and the serverside validator of a site, so it's better to leave the validation to the server/site itself and let the user correct it accordingly.

Comment 11

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

Comment on attachment 437553 [details] [diff] [review]
Patch v1

>@@ -1464,17 +1465,18 @@ nsHTMLFormElement::IsDefaultSubmitElemen
> PRBool
> nsHTMLFormElement::HasSingleTextControl() const
> {
>   // Input text controls are always in the elements list.
>   PRUint32 numTextControlsFound = 0;
>   PRUint32 length = mControls->mElements.Length();
>   for (PRUint32 i = 0; i < length && numTextControlsFound < 2; ++i) {
>     PRInt32 type = mControls->mElements[i]->GetType();
>-    if (type == NS_FORM_INPUT_TEXT || type == NS_FORM_INPUT_PASSWORD) {
>+    if (type == NS_FORM_INPUT_TEXT || type == NS_FORM_INPUT_PASSWORD ||
>+        type == NS_FORM_INPUT_TELEPHONE) {

Since this kind of change is pretty common in the patch, could you perhaps make
a helper method.
Something like
PRBool IsSingleLineTextControl(PRint32 aType, PRBool aIncludePassword);
That could probably a (static?) method in nsGenericHTMLFormElement.

or perhaps just
PRBool IsSingleLineTextControl(PRBool aIncludePassword); in nsIFormControl?
or whatever works the best.

Comment 12

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

Comment on attachment 437552 [details] [diff] [review]
Tests v1

You could at least test that modifying type attribute (both content and idl) works well.
Test setting the attribute to some invalid value and then to tel, and perhaps to checkbox and then back to tel...something like that.

And some reftest would be good; you could probably just make sure that <input type="tel"> looks like <input type="text">. And would be great to test even dynamic changes; from checkbox to tel and vise versa.

Comment 13

[Mounir Lamouri (:mounir)]

For the tests, I think I could add type check in bug 551670 tests as there is a function checking that. Ok for the reftests.

For the helper method, I agree it would be better.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Tests v2

reftests have been added and tests from bug 551670 have been changed to check the tel type behavior.

Comment 15

[Mounir Lamouri (:mounir)]

I'm marking this bug as depending on bug 551670 because it is changing its tests so bug 551670 will have to be landed first.

Comment 16

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Tests v2.1

Typo fix

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: Patch v2.1

Typo fix

Comment 19

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

Comment on attachment 437882 [details] [diff] [review]
Patch v2.1


>+  /**
>+   * Returns true if this is a control which has a text field.
>+   * @param  aExcludePassword  to have NS_FORM_INPUT_PASSWORD returning false.
>+   * @return Wheter this is a single line text control.
>+   */
>+  virtual PRBool IsTextControl(PRBool aExcludePassword) const = 0;
>+
>+  /**
>+   * Returns true if this is a control which has a single line text field.
>+   * @param  aExcludePassword  to have NS_FORM_INPUT_PASSWORD returning false.
>+   * @return Wheter this is a text control.
>+   */
>+  virtual PRBool IsSingleLineTextControl(PRBool aExcludePassword) const = 0;

You got the @return comments wrong way in these two.
And it is whether, not wheter.
>     // If readonly is changed for text and password we need to handle
>     // :read-only / :read-write
>     if (aNotify && aName == nsGkAtoms::readonly &&
>-        (mType == NS_FORM_INPUT_TEXT || mType == NS_FORM_INPUT_PASSWORD)) {
>+        IsSingleLineTextControl(PR_FALSE)) {
Update the comment "if readonly is changed..."

Have you checked that autocomplete works with type="tel"? (or does it even need to?)
Does sessionstore handle type="tel" properly? (or does it even need to?)

Comment 20

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

Does contextmenu work with type="tel"?

There are things like http://mxr.mozilla.org/mozilla-central/source/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp#1747

What about formfillcontroller? (which does also some of the autocomplete)

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/src/nsStorageFormHistory.cpp#456

I wonder how to change this all so that when adding the input types, there is
no need to change everything again.

Comment 21

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

There is also http://mxr.mozilla.org/mozilla-central/source/toolkit/spatial-navigation/SpatialNavigation.js#136
Though I'm not sure if fennec still uses spatialnavigation.

Comment 22

[Mounir Lamouri (:mounir)]

CC'ing David for a11y.

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Patch v3

This patch is fixing issues mentioned before. I also checked deeply to find other places where mozIsTextField or IsSingleTextLineControl was needed. It has been fixed everytime except for the password manager (follow-up bug 558370). It is highly probable I've missed some places but the base looks ok.
By the way, session store and autocomplete are working.

Do you want me to add new tests ?

By the way, it looks like this patch creates a bug (can't do any input except in menus). I'm not sure it comes from the patch or something else. I will look deeper at that later.

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.1

Some |type != NS_FORM_INPUT_TEXT| were changed to |IsSingleLineTextControl()|, missing the negation. This is now fixed and fixing the bug I saw.

I think we should remove "Check Spelling" from the context menu for input types different than 'text' and 'search'. I'm going to open a follow-up for that.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> CC'ing David for a11y.

It sounds there is nothing to do anything special in a11y support. At least I don't know how to say this input is used to type telephone number to AT via existing APIs. However I should get some feedback from the AT groups if we could do anything.

When patch is ready it's necessary to get Marco to test it.

Comment 26

[Marco Zehe (:MarcoZ)]

We could "invent" a new state or the like similar to password edits or the like to explicitly state that this is for telephone numbers. Do these inputs also look different in some way? I am specifically thinking of spin edits, where you can input a value (and no letters), but also have arrows to increase or decrease the value. Screen readers do specify that these are "spin edit controls". So if the styling or layout of telephone inputs is special, we should advocate for a new state or a11y role to make these clearly recognizable also for blind users.

Comment 27

[Mounir Lamouri (:mounir)]

The tel state will probably look like the text state. Maybe a small icon will be added but nothing important.
I think 'number' is going to be a 'spin edit controls'.

For more information about the tel state style, you should follow bug 558063.

Comment 28

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

Comment on attachment 438350 [details] [diff] [review]
Patch v3.1


>+#define NS_FORM_BUTTON_BUTTON    1
>+#define NS_FORM_BUTTON_RESET     2
>+#define NS_FORM_BUTTON_SUBMIT    3
>+#define NS_FORM_FIELDSET         4
>+#define NS_FORM_INPUT_BUTTON     5
>+#define NS_FORM_INPUT_CHECKBOX   6
>+#define NS_FORM_INPUT_FILE       7
>+#define NS_FORM_INPUT_HIDDEN     8
>+#define NS_FORM_INPUT_RESET      9
>+#define NS_FORM_INPUT_IMAGE     10
>+#define NS_FORM_INPUT_PASSWORD  11
>+#define NS_FORM_INPUT_RADIO     12
>+#define NS_FORM_INPUT_SUBMIT    13
>+#define NS_FORM_INPUT_TELEPHONE 14
Perhaps this could be actually 
NS_FORM_INPUT_TEL because the type is "tel"
>diff -r 90d264fda571 dom/interfaces/html/nsIDOMNSHTMLInputElement.idl
>--- a/dom/interfaces/html/nsIDOMNSHTMLInputElement.idl	Sat Apr 10 16:00:43 2010 +0200
>+++ b/dom/interfaces/html/nsIDOMNSHTMLInputElement.idl	Sun Apr 11 14:00:20 2010 +0200
>@@ -63,9 +63,15 @@ interface nsIDOMNSHTMLInputElement : nsI
>   void mozGetFileNameArray([optional] out unsigned long aLength,
>                            [array,size_is(aLength), retval] out wstring aFileNames);
>   void mozSetFileNameArray([array,size_is(aLength)] in wstring aFileNames,
>                            in unsigned long aLength);
> 
> 	/* convenience */
>   void                      setSelectionRange(in long selectionStart,
>                                               in long selectionEnd);
>+
>+  /**
>+   * This non-standard method prevents to check types manually to know if the
>+   * element is a text field.
>+   */
>+  boolean mozIsTextField(in boolean aExcludePassword);
> };
You're changing the interface, so please update its iid.

>diff -r 90d264fda571 toolkit/spatial-navigation/SpatialNavigation.js
>--- a/toolkit/spatial-navigation/SpatialNavigation.js	Sat Apr 10 16:00:43 2010 +0200
>+++ b/toolkit/spatial-navigation/SpatialNavigation.js	Sun Apr 11 14:00:20 2010 +0200
>@@ -128,18 +128,19 @@ function _onInputKeyPress (event, callba
> 
>   // check to see if we are in a textarea or text input element, and if so,
>   // ensure that we let the arrow keys work properly.
>   if (target instanceof Ci.nsIDOMHTMLHtmlElement) {
>       _focusNextUsingCmdDispatcher(key, callback);
>       return;
>   }
> 
>-  if ((target instanceof Ci.nsIDOMHTMLInputElement && (target.type == "text" || target.type == "password")) ||
>-      target instanceof Ci.nsIDOMHTMLTextAreaElement ) {
>+  if ((target instanceof Ci.nsIDOMNSHTMLInputElement &&
>+       bestElementToFocus.mozIsTextField(false)) ||
>+      target instanceof Ci.nsIDOMHTMLTextAreaElement) {
Why bestElementToFocus.mozIsTextField(false), why not target.mozIsTextField(false)


browser/ and toolkit/ parts look ok to me, but dao or gavin or someone should review them.
And because this changes API, this need sr. Sicking could probably do that.
IMO having mozIsTextField is ok, and it is useful thing.
(Perhaps even something for HTML5)

Comment 29

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.2

r=smaug

This is fixing the issues mentioned in previous comment.

Dao, could you review the toolkit/ and browser/ parts of this bug ?
Jonas, could you super-review the content part ?

Thanks :)

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 438976 [details] [diff] [review]
Patch v3.2

>diff -r 16e9ee5ae28f browser/base/content/browser.js

>+    if (((el instanceof HTMLInputElement && el.mozIsTextField(true)) ||
>+        type == "hidden" || type == "textarea") ||

The indentation in the second line is off, needs one more space.

>diff -r 16e9ee5ae28f toolkit/components/satchel/src/nsFormFillController.cpp

Somebody else should review this, e.g. dolske.

Comment 31

[Mounir Lamouri (:mounir)]

Comment on attachment 438976 [details] [diff] [review]
Patch v3.2

Thank you for your review Dão. I will fix the indentation later (waiting for other changes).
Asking dolske to review per previous comment.

Comment 32

[Justin Dolske [:Dolske]]

Comment on attachment 438976 [details] [diff] [review]
Patch v3.2

So, a metaissue here is if Form Manager should save the values from the new HTML5 input types in the *current* DB or not... We'll lose the type when storing a value, and can't offer suggestions previously entered for the current input type, but that doesn't seem like a problem. Not saving anything would certainly be less useful.

Would be good to file a followup on having Form Manager remember input types, and use those when determining what suggestions to offer for an input.


>--- a/toolkit/components/satchel/src/nsFormFillController.cpp
...
>-    if (type.LowerCaseEqualsLiteral("text") && !isReadOnly &&
>+    nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(input);
>+    if (formControl && formControl->IsSingleLineTextControl(PR_TRUE) &&
>+        !isReadOnly &&
>         (!autocomplete.LowerCaseEqualsLiteral("off") || isPwmgrInput)) {

Is nsIFormControl a scriptable interface? This code (well, nsStorageFormHistory, but not nsFormFillController) is being ported to JS and will be ready to land soonish -- see bug 439716. It'll need to be able to call this, or else have it's own list of control types matching this.


>--- a/toolkit/components/satchel/src/nsStorageFormHistory.cpp
...
>-      if (!type.LowerCaseEqualsLiteral("text"))
>+      // Save values from input fields which support autocomplete attribute
>+      // and having autocomplete enabled.
>+      // autocomplete is supported for type = 'text', 'search', 'url',
>+      // 'telephone', 'email', 'password', 'datetime', 'date', 'week', 'time',
>+      // 'datetime-local', 'number', 'range' and 'color'.
>+      // See: http://dev.w3.org/html5/spec/forms.html#the-input-element

This comment is a little misleading and long. I'd just nuke the whole thing, or briefly mention "// Check if input is a type supported by Form Manager".

>+      // XXX: this is checking if the input element suports the autocomplete
>+      // attribute but it is not final because most types listed above are not
>+      // implemented.

Nuke this too. No "XXX" comments, please. :)

>+      nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(inputElt);
>+      if (formControl && !formControl->IsSingleLineTextControl(PR_TRUE))
>         continue;

Should this be if (!formControl || !formControl->...) ?

>+      // TODO: autocomplete is not disabled only when equals to 'off',
>+      // bug 557628.

I don't think this is correct. Presumably you're talking about it having the "default" value, but that's handled by checking form.autocomplete earlier in ::Notify.

We could, perhaps, handle the case of having a input with autocomplete=on inside a form with autocomplete=off, though I'm not sure if we want to (seems inefficient). This is fodder for another bug, though.

Finally, does IsSingleLineTextControl() have test coverage somewhere? If not, it would be good to add here (see test_form_submission.html). I don't feel suprt strongly about this, though, we can (and will) add tests when form manager picks up full support for HTML5 inputs.

Comment 33

[Mounir Lamouri (:mounir)]

(In reply to comment #32)
> (From update of attachment 438976 [details] [diff] [review])
> So, a metaissue here is if Form Manager should save the values from the new
> HTML5 input types in the *current* DB or not... We'll lose the type when
> storing a value, and can't offer suggestions previously entered for the current
> input type, but that doesn't seem like a problem. Not saving anything would
> certainly be less useful.
> 
> Would be good to file a followup on having Form Manager remember input types,
> and use those when determining what suggestions to offer for an input.

I've open bug 562625. I think it's better to have the values saved even if the Form Manager doesn't remember the input type for the moment.

> >--- a/toolkit/components/satchel/src/nsFormFillController.cpp
> ...
> >-    if (type.LowerCaseEqualsLiteral("text") && !isReadOnly &&
> >+    nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(input);
> >+    if (formControl && formControl->IsSingleLineTextControl(PR_TRUE) &&
> >+        !isReadOnly &&
> >         (!autocomplete.LowerCaseEqualsLiteral("off") || isPwmgrInput)) {
> 
> Is nsIFormControl a scriptable interface? This code (well,
> nsStorageFormHistory, but not nsFormFillController) is being ported to JS and
> will be ready to land soonish -- see bug 439716. It'll need to be able to call
> this, or else have it's own list of control types matching this.

In JS, you can do the same think by QI to nsIDOMNSHTMLInputElement and call mozIsTextField(true).

> >--- a/toolkit/components/satchel/src/nsStorageFormHistory.cpp
> ...
> >-      if (!type.LowerCaseEqualsLiteral("text"))
> >+      // Save values from input fields which support autocomplete attribute
> >+      // and having autocomplete enabled.
> >+      // autocomplete is supported for type = 'text', 'search', 'url',
> >+      // 'telephone', 'email', 'password', 'datetime', 'date', 'week', 'time',
> >+      // 'datetime-local', 'number', 'range' and 'color'.
> >+      // See: http://dev.w3.org/html5/spec/forms.html#the-input-element
> 
> This comment is a little misleading and long. I'd just nuke the whole thing, or
> briefly mention "// Check if input is a type supported by Form Manager".
> 
> >+      // XXX: this is checking if the input element suports the autocomplete
> >+      // attribute but it is not final because most types listed above are not
> >+      // implemented.
> 
> Nuke this too. No "XXX" comments, please. :)

I will remove the comments.

> >+      nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(inputElt);
> >+      if (formControl && !formControl->IsSingleLineTextControl(PR_TRUE))
> >         continue;
> 
> Should this be if (!formControl || !formControl->...) ?

Indeed. Actually, checking for |formControl| is maybe useless as we are doing a QI from |inputElt| which must be a nsIFormControl.

> >+      // TODO: autocomplete is not disabled only when equals to 'off',
> >+      // bug 557628.
> 
> I don't think this is correct. Presumably you're talking about it having the
> "default" value, but that's handled by checking form.autocomplete earlier in
> ::Notify.
> 
> We could, perhaps, handle the case of having a input with autocomplete=on
> inside a form with autocomplete=off, though I'm not sure if we want to (seems
> inefficient). This is fodder for another bug, though.

Please, see bug 557628 where I've attached a patch trying to fix that. I will remove the comment so we can have this discussion in bug 557628.

> Finally, does IsSingleLineTextControl() have test coverage somewhere? If not,
> it would be good to add here (see test_form_submission.html). I don't feel
> suprt strongly about this, though, we can (and will) add tests when form
> manager picks up full support for HTML5 inputs.

I would say |IsSingleLineTextControl| is partly tested by the tests for 'text' and 'password' types. For the 'tel' type, I don't think I will be able to test _everything_. I will add something in test_form_submission.html for the tel input type.

Thank you for your comments :)
I will update the tests and the patch and re-ask for a review.

Comment 34

[Mounir Lamouri (:mounir)]

Attachment: Tests v3

test_form_submission.html is now checking for <input type='tel'>. I tried to create some helper functions so I will be able to add 'email', 'url' and 'search' easily.

Comment 35

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.3

r=smaug,dao sr=jonas

Justin, could you review this updated patch ?

Comment 36

[Justin Dolske [:Dolske]]

Comment on attachment 442455 [details] [diff] [review]
Tests v3

I'd rather not add <input type="tel"> to all the existing tests, in the interest of keeping each on focused on one specific thing (and to avoid potentially having to change a bunch of existing tests when we do better support for HTML5 form controls). All I was really asking about is if there was test coverage to make sure IsSingleLineTextControl() returns true for type==tel...

Probably easiest to add that to test_bug557620.html as a mozIsTextField test, and not change test_form_submission at all (or keep only the change for <form id="form101">).

Comment 37

[Mounir Lamouri (:mounir)]

Attachment: Tests v3.1

r=smaug

I've removed the form_submission changes and I've added a test for .mozIsTextField.
The change is trivial, no need to re-review I think.

Comment 38

[Mounir Lamouri (:mounir)]

Attachment: Patch v3.4

r=smaug,dao,dolske sr=sicking

I'm updating the patch to be sure it will apply correctly on the trunk.
I've launched the patch on the try server 10 hours later and with no errors.

Comment 39

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/930c42e06471

Comment 40

[Eric Shepherd [:sheppy]]

Documented.