Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Split from #675348: META: Support HTML5 form input elements to add a new 'emailfield' Form API element.
Questions:
1. Do we add validation?
2. Do we add any field formatters for this new type (or put as followup)?
Comment | File | Size | Author |
---|---|---|---|
#74 | 1174620-html5-email-fixup-testcoverage-74.patch | 2.39 KB | Niklas Fiekas |
#74 | 1174620-interdiff-74.txt | 773 bytes | Niklas Fiekas |
#71 | 1174620-html5-email-fixup-testcoverage-71.patch | 2.39 KB | Niklas Fiekas |
#63 | 1174620-html5-email-63.patch | 19.63 KB | Niklas Fiekas |
#63 | 1174620-interdiff-63.txt | 860 bytes | Niklas Fiekas |
Comments
Comment #1
Dave Reid1. I think we run it through valid_email_address in the default validator.
2. Follow-up separate issue.
Comment #2
Dave ReidComment #3
Dave ReidInitial patch, based on the branch '1174620-html5-emailfield' from http://drupal.org/sandbox/davereid/1086890
Comment #5
Dave ReidTodos:
1. Figure out why this fails install
2. Seven theme lacks styling.
Comment #6
Dave ReidRenamed element to 'email' from 'emailfield' as per #675348-53: META: Support HTML5 form input elements.
Comment #7
Dave ReidComment #8
Dave ReidComment #10
Dave ReidDuh, the element needs the default #element_validate...
Comment #12
Dave ReidUm, so I tested on a clean D8 checkout + patch and it installed just fine for me. I am not sure what is up.
Comment #13
Dave ReidSo let's try this one more time.
Comment #15
Dave Reid#13: 1174620-html5-email-element.patch queued for re-testing.
Comment #17
Ralt CreditAttribution: Ralt commentedHi,
Why is it necessary to run the validation through valid_email_address since HTML5's validation is automatic? Older browsers' support?
Comment #18
Dave Reid@Ralt: Correct, for HTML5-enabled browsers the validation will happen in the browser, and then double-checked on the server side to support any non-HTML5 supporting browsers.
Comment #19
Jacine@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.Comment #20
Dave ReidI feel like the multiple attribute needs to be its own separate issue because we already provide the 'duplicate' functionality in Field API's 'multiple value' feature which could possibly be cleaned up to support it. Either way it requires a lot more discussion and work. Note that using $form['emailelement']['#multiple'] = TRUE doesn't actually work right now.
Comment #21
JacineOkay, sure. I was just wondering. Thanks! ;)
Comment #22
Dave Reid#13: 1174620-html5-email-element.patch queued for re-testing.
Comment #24
rfay#13: 1174620-html5-email-element.patch queued for re-testing.
Comment #26
rfayPuppet turned the change off, so fixed and trying again.
Comment #27
rfay#13: 1174620-html5-email-element.patch queued for re-testing.
Comment #28
rfay#13: 1174620-html5-email-element.patch queued for re-testing.
Comment #30
rfayWell, it does look like progress. Looks like this is a valid result?
Comment #31
quicksketchUnfortunately, 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.
Comment #32
ericduran CreditAttribution: ericduran commentedAs 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.
Comment #33
ericduran CreditAttribution: ericduran commentedActually lets wait till the #1174640: Add new HTML5 FAPI element: number element gets in so we don't have to re-roll it again.
Comment #34
Dave Reidemail element is supported in the 6.x-2.x of SimpleTest which is used by PIFR: #1177106: False install failure when testing HTML5 email element patch
Comment #35
ericduran CreditAttribution: ericduran commented@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.
Comment #36
ericduran CreditAttribution: ericduran commentedHa sorry for the x-post, I completely missed that.
Comment #37
JacineAdding this to the current HTML5 sprint.
Comment #38
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThere were a few merge conflicts when rebasing onto 8.x, so here is a reroll.
Look at the interdiff for these new things:
Comment #39
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAdded tests.
Comment #40
ericduran CreditAttribution: ericduran commentedNice. Here's what's missing on this patch.
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 :)
Comment #41
ericduran CreditAttribution: ericduran commentedOh actually I realized something else. We no longer require this change.
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()
Comment #42
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for reviewing.
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?
Comment #43
ericduran CreditAttribution: ericduran commentedAhh, I see you're setting the value back with the trim. I completely miss that. You can feel free to ignore my trim comment ;-)
Comment #44
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk, did these things.
Yeah ... but it might still be a valid point. Browsers wouldn't even let throughEdit: I take this back. testRequiredFields() makes similar assumptions, too.foo@foo.com
. So maybe, maybe it's cleaner to adjust the contact form test cases and don't trim anything.Comment #45
ericduran CreditAttribution: ericduran commentedThe 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. :)
Comment #47
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks.
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:
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?Comment #49
Dave ReidLet's just trim the form value like with url. It doesn't make sense to have spaces before or after an e-mail address, so let's just make the element more usable and trim it.
Comment #50
ericduran CreditAttribution: ericduran commentedYea, I guess the trim can only help. Fine by me.
Comment #51
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk, thanks. Added back trim.
Comment #52
ericduran CreditAttribution: ericduran commentedThis 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 :)
Comment #53
Dave ReidShould probably be just $this->assertText(t('The e-mail address invalid is not valid.'), 'Valid e-mail required.')); for consistency with the other assertions like with the url element.
Should we consider making the default maxlength for e-mail fields the constant EMAIL_MAX_LENGTH instead? I think we have way more uses of 254/255 characters than 128 which could help simplify some FAPI code.
Marking only back to needs review.
Comment #54
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh ... 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.
Comment #55
Dave ReidWe can remove this now.
And this one.
And this one. :)
And this one too. :)
Comment #56
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRight, thanks :)Ignore this one.Comment #57
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh ... that was #54 again. Now what was meanted to be #56.
Comment #58
aspilicious CreditAttribution: aspilicious commentedThis comment is not needed anymore.
Besides of that ==> RTBC
Comment #59
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRight. Thanks.
Comment #60
aspilicious CreditAttribution: aspilicious commentedGood...
Comment #61
ericduran CreditAttribution: ericduran commentedIs 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.
Comment #62
aspilicious CreditAttribution: aspilicious commentedHmm yeah you're right... Although you can say it should work because of the valid with spaces...
Comment #63
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk, added that.
Until now there were only implicit tests through site installation, user registration and contact form.
Comment #64
ericduran CreditAttribution: ericduran commented@Niklas Fiekas you're awesome :)
We'll wait for the test bot. But this looks good.
Comment #65
aspilicious CreditAttribution: aspilicious commentedShould be rly rtbc now...
Comment #66
catchI'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!
Comment #67
ericduran CreditAttribution: ericduran commented@catch it looks like this commit didn't actually make it up to drupal.org. Same thing with a couple of other issues.
Comment #68
catchTried again :)
Comment #69
sunAwesome! 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. :)
Comment #70
Dave ReidNote I added 'email' to the change notification at http://drupal.org/node/1315186
Comment #71
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks like we forgot to add testcoverage for disabled email elements and email elements with placeholders. Reopening to add this. (Untested patch attached.)
Comment #73
aspilicious CreditAttribution: aspilicious commentedShould be 35?
Comment #74
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh, indeed. Typo there.
Comment #75
aspilicious CreditAttribution: aspilicious commentedback to rtbc
Comment #76
Dries CreditAttribution: Dries commentedCommitted the missing tests to 8.x.
Comment #77
JacineSweet! Thanks everyone :D
I assume this needs a change notice?
Comment #78
Dave ReidSee #70.
Comment #79
JacineThanks @davereid :D
Removing the sprint tag.