Add new HTML5 FAPI element: telephone

We should server-side whatever validation the spec says should be done client side: No more, no less. (At least that's my default position.)

Should I test this with Drupal 8 HEAD?

From a site builder's perspective, what should I expect to see?

tried to apply the patch, but I guess I'm doing it wrong.

I believe the 'tel' entry was left out of includes/common.inc - drupal_common_theme(). It seemed to work fine after adding the entry. New patch attached.

adding tags

Here's an updated patch with a few tests (tested in the same level of detail as textarea and password elements).

This is getting just silly and out of hand. Are we going to add a new, identical theme function for every single fancy HTML5 decorated textfield element?

@chx I think everyone is just trying to make as little change as possible. If you think this should be done better please let us know.

I can see how extracting the theme_textfield to possibly take a type parameter might be better but again, we're just following what core does here, which is declare a theme function for every html element. @see theme_textfield, theme_password, theme_hidden, theme_checkbox even though they all share about 95% of the same code.

I'm confused as to why theme_textfield is being touched in this patch.

updated tag

#9 no longer applies cleanly to 8.x so I re-rolled. The only substantive change I made was to simplify the tel test, grouping tel with textfield, textarea, and hidden elements.

@ericduran, the reason theme_textfield() is being touched is because the autocomplete code used to live within it, but was pulled out into form_add_autocomplete() to allow theme_tel() and other functions to use it. You can see Dave Reid's initial patch in #5 contains the autocomplete change.

I dunno about this -- if the best we can come up with is theme_tel, then be it, but why dont we simply call theme_textfield from inside of it when the code is 100% identical as far as i can see? Not theme('textfield') but theme_textfield.

@chx, I can see what you mean about calling theme_textfield() within theme_tel(), but do we want to complicate theme_textfield to that degree? Consider the different input types that fall back to a plain textfield, notice how little code the changes, but each element offers vastly different client-side validation, rendering, and server-side storage possibilities.

I think abstracting all of these elements to theme_textfield() may seem manageable today — pre-implementation — but imagine a scenario in the future where people pop into #drupal-contribute asking how to render a tel or email input, very similar to how people ask where the #class FormAPI attribute is today. Or worse, people asking why a datetime input stores a string instead of a timestamp in the db.

In the interest of DX, I think it will be better to separate these elements and allow them to evolve separately.

Sure. Let's add more bloat. I cant care less, TBH.

@chx, So how about we have something like this instead:

<?php
$form['telephone_field'] => array(
 '#type' => 'input', // Remove textfield, tel, number, etc
 '#input_type' => 'tel' // Support different input type, since they're all the same text, tel, number, etc...
);
?>

Then we can have one theme function for theme_input which can support anything in the "type" attribute. (by type I mean the html input type attribute.)

This should pretty much eliminate any code duplication on the regular <input /> field.

@chx, we're open to any other recommendation. Let me know what you think.

@ericduran that's way way more in line with what I was thinking -- namely add an additional attribute to #type text and handle it that way. Yours is , how to say, less semantic on the server end? Surely #type text means more than #type input. Do you imagine #type input #input_type checkbox? And what about checkboxes/radios which dont have a direct HTML equivalent?

A big problem in the past has been elements that are semantically the same to the programmer but different to HTML. They tended to produce different resulting form state results, which made switching between them way too difficult. From a PHP perspective, a single-select and radio buttons are the same thing. Similarly, checkboxes and multi-select are the same thing. (And there's a variety of fancier compound elements that also produce the same logical result, or should.)

Whatever we do here should keep that in mind. It should be really really easy for logically-identical form elements/widgets to be swapped out for each other without breaking stuff. How best to do that here I'm not sure, but I highlight it as a consideration. :-)

If you guys want to see this stuff backported, chime in here with arguments for it:

#1268826: Consider allowing new HTML5 FAPI elements to be backported to D7

It would be really nice to see that happen, but I'm inclined to forget about D7 entirely at this point, unless it starts to look better really soon. IMO the momentum on this initiative has slowed down significantly lately, and D7 is not my concern, so I don't want to waste cycles on something that has little chance of happening.

.

sub

#1268826: Consider allowing new HTML5 FAPI elements to be backported to D7 is now closed. Every reply except for me and Dave was against it, so no sense wasting our time on pipe dreams. :(

Re #18, what are the theme function(s) gonna look like for these? I assume this means they will be combined? Also, if we are going to use this issue to add a bunch of types, then can we please update the title and issue summary? Thanks!

I can see @chx's point. We clearly cannot group all new input fields under 'text', but, perhaps some. E.g. 'range' is not text, but search / tel (maybe, really numeric), etc. maybe. I am completely opposed to #type=input for the reasons stated, radio is an input type, but would not be handled in #type=input, but rather #type=radio.

I think that it is all rather confusing from a dx perspective unless we do a separate #type for each element. It might seem like bloat, but these are not candy, they are semantic elements that will be with us for decades, and we have no idea how we might want to change our implementation today.

Can someone in support of the #type=text #input_type=foo|bar please provide a list of input types that should be collected under #type=text?

Cleaning up tags. Also, I really need an issue summary or update here. FAPI issues have been very quiet lately and I badly need to know what's going on with this.

Everett has very accurately summarized the issue of consolidation:

I think that it is all rather confusing from a dx perspective unless we do a separate #type for each element. It might seem like bloat, but these are not candy, they are semantic elements that will be with us for decades, and we have no idea how we might want to change our implementation today.

I'd love to keep these FAPI issues moving. If we can't agree on a final solution, could we at least start by making separate functions, and consider consolidating similarities at a later time?

Yeah... There aren't THAT many new elements, so I don't think it's really a bloat problem. The place where consolidation makes sense (to me) is for the date inputs. Otherwise, it's kinda messy and potentially risky to combine them. I imagine some module in contrib that wants to provide a polyfill for something, and the thought of it overriding theme_input() for one type makes me a tad nauseous TBH. Obviously there are other ways to deal with that, such as theme hook suggestions, but if we've only got one base hook, that guarantees that all preprocessing code will go in template_preprocess_input() or whatever, which could end up being a big mess.

I'm really just trying to figure out what the plan is here. It's not clear to me how those of you who want to consolidate these want to approach it. This issue is currently holding up all the other FAPI issues. We need to come some sort of agreement ASAP, so bumping this to major.

@davereid and @ericduran do you have any thoughts on how you want to see this done?

I honestly think that for now, we should add the elements just like we do with every other element.

We have an element_info declaration and we have a separate theme function for that element.

As @Jacine mention, there really isn't that many new elements. If somehow we think this adds bloat then we can refactor it once we have all the element in. Also that would probably be easier because we'll have a better idea of what property every element has and what they have in common.

I agree with Eric. Implementing the current API model first lets us see later what centralization would make sense to do. Once a couple of elements are implemented in the current approach it should become blindingly obvious what, if any, centralization would be sensible and we can then do that more easily.

Thanks for the quick feedback @rupl, @ericduran and @Crell.

@chx Does this seem like a reasonable plan to you?

+++ b/includes/form.inc
@@ -3660,8 +3660,47 @@ function theme_textfield($variables) {
+function theme_tel($variables) {
+  $element = $variables['element'];
+  element_set_attributes($element, array('type', 'id', 'name', 'value', 'size', 'maxlength', 'placeholder'));
+  _form_set_class($element, array('form-text', 'form-tel'));
+
+  $extra = form_add_autocomplete($element);
+  $output = '<input' . drupal_attributes($element['#attributes']) . ' />';
+
+  return $output . $extra;
+}
+

I just confirmed with @chx in IRC that all he meant in #15 is that he wants the above replaced with something like this:

function theme_tel($variables) {
  $variables['element']['#attributes']['class'][] = 'form-tel';
  return theme_textfield($variables);
}

He's in agreement with the rest of #14, as apparently so is nearly everyone else here, so setting back to needs review. What do the rest of you think of the above change though?

Note that this still leaves "tel" as a separate theme function for purposes of preprocessing and overriding. This only changes the internal implementation of the default theme_tel().

To clarify: I am fine with #33. That addresses my main concern. I didnt raise the multiple #type problem and I don't care much about that.

I think I'm also fine with #33, but have some questions:

1. Does it really make sense to have a .form-text class on TEL elements in terms of theming and eventually applying special styles to TEL elements?
2. I'm not sure... would it make sense to properly fix/change that #autocomplete processing into a proper #process or #after_build in a separate issue first? This kind of manual invocation of form_add_autocomplete() in a theme function doesn't really look sustainable or clean to me. Obviously only concerned about this, as we're basically about to repeat this pattern for ~10+ other new form element types.

@sun regarding point 1:

I believe .form-text is still necessary on some of these elements, telephone included. They are text fields with additional functionality. Leaving .form-text in place and adding .form-TYPE would allow for global text field styles, while providing an easy way to enhance specific inputs like telephone.

If a UA cannot understand one of these new input types, the fallback behavior is generally a plain text field. Similarly if an older browser cannot parse input[type='tel'] or another fancy selector, .form-tel acts as a convenience class similar to .first, .last, and zebra classes that are currently provided in core.

I agree with @sun's concerns in #36, but I don't think we need any of those classes on inputs anymore. I've created issues for both points:

1. #1309392: Remove _form_set_class()
2. #1309394: Process #autocomplete_path for all form elements; remove custom/duplicated code from theme_textfield()

Hopefully those issues resolve the outstanding concerns and we can move forward.

FYI: I posted a patch for #1309392: Remove _form_set_class(), but I need some help with 3 of the test assumptions (not sure how to target the wrapper with xpath).

#33 Is a great approach in terms of reusability. However, we run into a problem with reusing theme_textfield:
$element['#attributes']['type'] = 'text';
The "type" attribute will get clobbered.

If theme_textfeld is to be used as a general base, then we should conditionally add the "text" type attribute to make sure we aren't overwriting anyone.

  if (!isset($element['#attributes']['type'])) {
    $element['#attributes']['type'] = 'text';
  }

We can probably refine theme_textfield to extend this idea a little further, but that can be a separate patch ;)

Also, considering Jacine's comment on #38, we can omit 'form-tel' from the class array since we can both depend on the wrapper classes and CSS3 attributes selectors.

+++ b/includes/form.incundefined
@@ -3690,12 +3690,49 @@ function theme_hidden($variables) {
+  ¶
+  if (!isset($element['#attributes']['type'])) {
+    $element['#attributes']['type'] = 'text';
+  }

Extra white spaces

+++ b/includes/form.incundefined
@@ -3690,12 +3690,49 @@ function theme_hidden($variables) {
+ */
+function form_add_autocomplete(&$element) {

This shouldn't be added here, There's a separate issue about this.

-19 days to next Drupal core point release.

I really didn't like the recommendations. Here's a patch. This just replace theme_textfield with theme_inputfield which just makes more sense.

After this gets in we can probably wrap a bunch of other element in this one theme function which could cover almost all the input elements.

#42: 1174634-42.patch queued for re-testing.

Here's a re-rolled, I expected this to break things, but it shouldn't break this bad. Maybe is the bot. Not sure yet.

lol, I just realized what I was doing. Here's a better one, same approach. Ideally in the future we could move textfield to text, which honestly makes more sense anyways.

Anyways I imagine this will spark some controversy, But essentially this will allow us to remove a lot of duplicate code.

Ok, I swear I review this one locally before uploading.

Wow, I'm soooo sorry for all the emails :-/

Ok this issue in in danger of just getting completely sidetracked.

As mention in previous post this should not be the place to make these kind of optimization, shame on me for even going down that path.

This is going to add a very simple telephone element using the same pattern we already use on every other element in drupal.

Please keep following comments related to telephone field and telephone field only. Also I'm going to create a couple of follow up issues about optimizing the autocomplete portion if that doesn't already exist.

Thanks.

Here's the patch. Extremely simple, extremely familiar to #14.

Lets wait for the bot before reviewing I wouldn't want to waste anyone's time. Thanks!

Edit:
Oh actually the other issues already exist and have been mention multiple times on this thread. So for anyone interesting :
#1309394: Process #autocomplete_path for all form elements; remove custom/duplicated code from theme_textfield()
#1309392: Remove _form_set_class()

whoops, I forgot one.

We're also missing some default CSS as the form fields look differently on Garland and Seven then normal text fields do. Searching in CSS for 'form-text' I found the obvious ones but there seem to be *lots* of instances in bartik that apply only to 'text fields':

This is exactly what I was referring to in #37. This input is a text field, period. It happens to be a specialized textfield when modern user-agents can enhance it. We should leave the .form-text class for this element. Rather than tweaking multiple themes and doubling selectors all over the place just keep the class.

Yea, I agree with @rupl on the css. This is what we do for these fields in D7.

Ok after "much" discussion on the #drupal-html5 channel with davereid, rupl, jacine, aspilicious. We're all ok with this patch.

So we're going to RTBC.

I wonder if this should actually be 'telfield'? (I know I'm the one who originally started with 'tel'). Because using 'tel' also automatically registers theme_tel() for the form element, what if down the road HTML9 introduces a field for outputting telephone numbers?

Thanks, @Dave Reid, for raising this excellent architectural and future-oriented concern!

I'm basically following that mindset, but at the same time, need to acknowledge that 'textfield' is pretty much the only form element #type that has the "field" suffix in it. Additionally, there is #431812: Rename FAPI type 'textfield' to 'text' globally, but that issue kinda stalled as it's highly debatable, and even in light of that, it's very very unlikely that HTML will ever introduce a TEXT element on its own — contrary to a relatively reasonable and more likely possible TEL element. Right now, you could have a #type 'text' element; whereas if there's going to be a TEL element, you'd have to hack away with #type 'telelement' or similar.

That said, I'm OK to go with #type 'tel' for now. We should, however, have a very close eye on consistency (with regard to existing form elements and the other, new element #types we're introducing for HTML5 within Drupal) as well as on the HTML spec itself.

I've bumped #1309394: Process #autocomplete_path for all form elements; remove custom/duplicated code from theme_textfield() to major task, although I'm sure there's an older issue for that, at least, it's been annoying me for years (not that I ever sat down to fix it).

Committed/pushed this one to 8.x. Will need a change notification.

The patch in #66 looks good to me. It adds the tel type in checks for placeholder text and disabled elements.

Committed/pushed to 8.x, thanks!