Add new HTML5 FAPI element: email

Hi,

Why is it necessary to run the validation through valid_email_address since HTML5's validation is automatic? Older browsers' support?

@Dave Reid question for ya: How are we going to deal with the multiple attribute? I ask because the select element has a specific #multiple property for this, and the multiple attribute is allowed (even if it's not implemented yet) on all of these new input elements, including the e-mail type.

Okay, sure. I was just wondering. Thanks! ;)

#13: 1174620-html5-email-element.patch queued for re-testing.

Puppet turned the change off, so fixed and trying again.

#13: 1174620-html5-email-element.patch queued for re-testing.

#13: 1174620-html5-email-element.patch queued for re-testing.

Well, it does look like progress. Looks like this is a valid result?

Unfortunately, you guys will probably find quite quickly that SimpleTest does not support setting POST values on fields of type="email" (or any other HTML5 elements), at least in my testing. We're doing the same thing over in the Webform issue queue and I'm not having the most luck with tests so far. In any case, we need to test the e-mail field specifically to make sure it's working as expected.

As mention by @quicksketch simpletest doesn't support the post of the new element.

The fix is to modified DrupalWebTestCase and add the email type.

Patch coming up. Plus this needs a re-rolled.

Actually lets wait till the #1174640: Add new HTML5 FAPI element: number element gets in so we don't have to re-roll it again.

@quicksketch and anyone else interested in D7 support I added a patch to the contrib simpletest module #1415926: Simpletest doesn't allow the new html5 elements to be submitted properly. to add the new form elements.

Ha sorry for the x-post, I completely missed that.

Adding this to the current HTML5 sprint.

There were a few merge conflicts when rebasing onto 8.x, so here is a reroll.

Look at the interdiff for these new things:

• trim() addresses, because that might be common sense, but also because the contact form tests explicitly have test for e-mail adresses padded with whitespace.
• Fix an assertion that was failing due to a string change.
• Remove the test method UserValidationTestCase::testMailAddresses(), because the function user_validate_mail() that is tested there was removed.

Added tests.

Nice. Here's what's missing on this patch.

+++ b/core/includes/form.incundefined
@@ -3703,8 +3703,48 @@ function theme_textfield($variables) {
+  _form_set_class($element, array('form-text', 'form-email'));

We are no longer doing both classes. We now only do the class element. So it should only be _form_set_class($element, array('form-email')); and the css for that new element should be added :)

Also I hate to nitpick this patch. But should we really have the trim? I think this is a bad idea. If I enter " myemaiL@aol.com" that should be invalid. Same goes for any other whitespace. We shouldn't be trying to correct a user mistake. Even worse I would hate to have to always use trim because we are allowing spaces to be allowed in the 'email' field.

Besides that the patch looks great :)

Oh actually I realized something else. We no longer require this change.

+++ b/core/includes/form.incundefined
@@ -3703,8 +3703,48 @@ function theme_textfield($variables) {
+/**
+ * Return the autocompletion HTML for a form element.
+ *
+ * @param $element
+ *   The renderable element to process for autocompletion.
+ *
+ * @return
+ *   The rendered autocompletion element HTML, or an empty string if the field
+ *   has no autocompletion enabled.
+ */
+function form_add_autocomplete(&$element) {
   $extra = '';
-  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'])) {
+
+  if (!empty($element['#autocomplete_path']) && drupal_valid_path($element['#autocomplete_path'])) {
     drupal_add_library('system', 'drupal.autocomplete');
     $element['#attributes']['class'][] = 'form-autocomplete';
 
@@ -3717,9 +3757,21 @@ function theme_textfield($variables) {

@@ -3717,9 +3757,21 @@ function theme_textfield($variables) {
     $extra = '<input' . drupal_attributes($attributes) . ' />';
   }
 
-  $output = '<input' . drupal_attributes($element['#attributes']) . ' />';
+  return $extra;
+}
 

Instead it should be done inside the theme_email field. I know it sucks, but the refactoring for the autocomplete is happening on a separate issue (#1309394: Process #autocomplete_path for all form elements; remove custom/duplicated code from theme_textfield()

Thanks for reviewing.

Even worse I would hate to have to always use trim because we are allowing spaces to be allowed in the 'email' field.

You wouldn't have to, because the value will be trimmed. But anyway, yeah, we probably shouldn't do that. Would we update the contact form tests accordingly, so that they don't expect whitespace-padded e-mail addresses to work?

Ahh, I see you're setting the value back with the trim. I completely miss that. You can feel free to ignore my trim comment ;-)

Ok, did these things.

Ahh, I see you're setting the value back with the trim. I completely miss that. You can feel free to ignore my trim comment ;-)

Yeah ... but it might still be a valid point. Browsers wouldn't even let through foo@foo.com. So maybe, maybe it's cleaner to adjust the contact form test cases and don't trim anything. Edit: I take this back. testRequiredFields() makes similar assumptions, too.

The previous patch looks great. Here's an update with a couple of fixes.

Fixes are:
- Fix the '$element['#attributes']['type'] = 'tel';' and changed it to email.
- Remove the form_add_autocomplete function as that's being handle in a different issue.
- Remove the trim, because we always said we'll follow what the browsers do. I tested this and if you add spaces the broswer won't let you submit. So we shouldn't do it differently.

Thats all the changes. :)

Thanks.

Fix the '$element['#attributes']['type'] = 'tel';' and changed it to email.

This one is funny, having that I didn't even copy&paste it from tel, but that I was typing it while having the tel element on the right :)

Changes:

• Only try to validate if not empty.

However we are still expecting test failures due to the now different error messages. This brings me to a point: The error message is currently like that: The e-mail address <em> nfoKbDWr@example.com</em> is not valid.. But this is what the user sees then: The e-mail address nfoKbDWr@example.com is not valid (the whitespace isn't visible). So ... what are we going to do about that?

Yea, I guess the trim can only help. Fine by me.

Ok, thanks. Added back trim.

This one is also RTBC.

Notes on the RTBC Status:
- This patch does exactly what we agree to do with all the elements for now.
- Follows the same pattern as the other new element aka "tel" element

This should be good to go :)

Oh ... yes, EMAIL_MAX_LENGTH as the #maxlength should be fine.

About the first point: Yes, I actually thought about that. I think it should be like that, because t('The e-mail address invalid is not valid.') would be a new localizable string, and such strings shouldn't be introduced in test cases. Only t()-strings that are already defined in other places should/must be used.

Right, thanks :) Ignore this one.

Oh ... that was #54 again. Now what was meanted to be #56.

+++ b/core/modules/user/user.moduleundefined
@@ -1149,20 +1123,15 @@ function user_account_form_validate($form, &$form_state) {
   // Trim whitespace from mail, to prevent confusing 'e-mail not valid'
   // warnings often caused by cutting and pasting.

This comment is not needed anymore.

Besides of that ==> RTBC

Right. Thanks.

Good...

Is it me, or is there a test missing.

Right now an invalid email is tested and an email address with spaces around it. We also need to test just a regular email address without spaces around it.

If this is already somewhere I'm sorry and feel free to mark it it RTBC again.

Hmm yeah you're right... Although you can say it should work because of the valid with spaces...

Ok, added that.

Until now there were only implicit tests through site installation, user registration and contact form.

@Niklas Fiekas you're awesome :)

We'll wait for the test bot. But this looks good.

Should be rly rtbc now...

I've marked #1309394: Process #autocomplete_path for all form elements; remove custom/duplicated code from theme_textfield() back to needs review, it'd be really good tg get that one in so as not have to keep duplicating that theme function autocomplete stuff all the time.

Committed/pushed to 8.x. Thanks!

@catch it looks like this commit didn't actually make it up to drupal.org. Same thing with a couple of other issues.

Tried again :)

Awesome! Great work, guys! :)

Somehow wasn't subscribed to this yet. Perhaps I'm also missing other issues.

Would be cool if anyone of you could remind us of new form elements with autocomplete support landing in core over in #1309394: Process #autocomplete_path for all form elements; remove custom/duplicated code from theme_textfield(), as long as that didn't land yet. :)

Looks like we forgot to add testcoverage for disabled email elements and email elements with placeholders. Reopening to add this. (Untested patch attached.)

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -261,7 +261,7 @@ class FormsTestCase extends DrupalWebTestCase {
+    $this->assertEqual(count($disabled_elements), 36, 'The correct elements have the disabled property in the HTML code.');

Should be 35?

Oh, indeed. Typo there.

back to rtbc

Committed the missing tests to 8.x.

Sweet! Thanks everyone :D

I assume this needs a change notice?

Thanks @davereid :D

Removing the sprint tag.