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 'urlfield' Form API element.
Questions:
1. Do we add validation?
2. Should local/internal path textfields be converted to an urlfield (e.g. URL alias field on nodes)?
Comment | File | Size | Author |
---|---|---|---|
#66 | 1174630-html5-url-66.patch | 16.43 KB | Niklas Fiekas |
#66 | 1174630-interdiff-66.txt | 2 KB | Niklas Fiekas |
#62 | 1174630-interdiff-62.txt | 496 bytes | Niklas Fiekas |
#62 | 1174630-html5-url-62.patch | 15.99 KB | Niklas Fiekas |
#60 | Screenshot at 2012-03-03 08:58:02.png | 9.5 KB | cosmicdreams |
Comments
Comment #1
Dave ReidComment #2
ericduran CreditAttribution: ericduran commentedWe probably should add validation, same validation thats done in the browser.
We shouldn't use this for local/internal path textfields mainly because the url field is supposed to be an absolute URL so if we use it for local/internal path the field will be :invalid.
Comment #3
Dave ReidYeah I double checked and the url element can only be used for absolute URLs, so that's a pretty easy decision as to how we validate it then: valid_url($url, TRUE).
Comment #4
Dave ReidComment #6
Dave ReidComment #7
Dave ReidIncorrectly named validation function.
Comment #9
Dave ReidComment #11
Dave ReidComment #12
Dave ReidDropbox did not sync in time...
Comment #13
jhedstromDoes the maxlength of 128 even need to be defined? If so, it should probably be longer than that, as some URLs are quite long.
Comment #14
Dave ReidI'd be fine going with a default of 255 since that makes sense, but this was the default of the existing element textfield which was used for URLs.
Comment #15
Dave ReidComment #16
ericduran CreditAttribution: ericduran commented@Dave, do we want to add the autocomplete to the url elements? I think it might be better to just leave the autocomplete stuff to the regular text fields.
We can always add them later.
Comment #17
Dave ReidI don't see why not since this is essentially a text field anyway, it should probably support autocompletion.
Comment #18
ericduran CreditAttribution: ericduran commentedComment #19
kingandy CreditAttribution: kingandy commentedAutocomplete on a URL field can cause a problem with iOs devices ... if autocomplete is on, it'll treat it as a semantic text field and offer suggestions from the dictionary - basically, they equate "autocomplete" with "autocorrect". The problem is that not only will it try to autocorrect domains to similar dictionary words, but if you cancel them out you'll get domains (or domain fragments) in your autocorrect dictionary.
I realise this is technically a problem for iOs rather than Drupal/HTML5, but I'd prefer to see autocomplete disabled on URL fields by default.
Comment #20
attiks CreditAttribution: attiks commentedsubscribing for #1239910: META: tracking other issues about validation
Comment #21
ericduran CreditAttribution: ericduran commentedComment #22
JacineCleaning up tags.
Comment #23
Everett Zufelt CreditAttribution: Everett Zufelt commented@kingandy
Can you please provide documentation on the iOS autocomplete issue on URLs? My suspicion is that this problem is when the @autocomplete attribute is set to "on", or rather not set to "off". This likely has nothing to do with our autocomplete implementation in autocomplete.js.
That is, we can set @autocomplete="off" on the field, and still allow lookup values from the server w/ autocomplete.js.
Comment #24
kingandy CreditAttribution: kingandy commentedI think you're right, yes. Since this was an HTML 5 thread, and I'd just come from doing some @autocomplete integration myself, I assumed that's what was under discussion and completely forgot the word was also used for Drupal's javascript/callback-based mechanism...
So, yes, switching off the user-agent based autocomplete and injecting our own suggestions via the established javascript should be completely fine :)
Comment #25
xjmRerolled for core/ and the death of
profile.module
.Comment #26
ericduran CreditAttribution: ericduran commentedThis issue has actually been block by #1174634: Add new HTML5 FAPI element: telephone
We probably should tackle that one pretty soon, and wrap up all these form elements.
Comment #27
ericduran CreditAttribution: ericduran commentedHere's another rerolled this time with a couple of extra test.
This is probably going to need to be rerolled yet one more time after #1174634: Add new HTML5 FAPI element: telephone gets in but lets see if this passes with the extra test.
Comment #28
Dave ReidMake sure to check this CSS too :)
Comment #29
ericduran CreditAttribution: ericduran commentedYea, lol I wanted to make sure I got the test done right 1st, because I'm pretty sure i screw something up. Adding the css now.
Comment #30
Dave ReidSide note that I think we may also need to add a selector for this in core/misc/vertical-tabs.css for
.vertical-tabs .form-type-url input
.Comment #32
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedComment #33
ericduran CreditAttribution: ericduran commentedSo I tested this and the browser actually lets you submit with extra space on the url field so the trim makes sense here.
So this looks good to me. Marking it RTBC.
Comment #34
Dave ReidWe're missing tests to confirm the trimming behavior like are present on the e-mail patch.
What is this addition? I couldn't find anything similar in the 'tel' FAPI patch that went in?
Comment #35
ericduran CreditAttribution: ericduran commentedYea, that extra content doesn't belong there.
This should be in the loop instead. It also looks like we missed a test in the tel element.
Comment #36
ericduran CreditAttribution: ericduran commentedActually it's still missing the trim test.
Comment #37
Dave ReidI was wrong - that hunk of code is needed since we can't use element value or hijack values of non-URL values otherwise the form submit fails validation. Sorry ericduran. :(
Comment #38
Dave ReidAdding to current sprint. So in summary, we still need tests to confirm validation and trimming of this field.
Comment #39
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYep, exactly. So ingnore #35 or were there any other changes?
Comment #40
Dave ReidWe're still missing a test to confirm that the form submits a trimmed URL field value.
Comment #41
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOf course. Thus "+Needs tests". I was talking about the patch in #35.
Comment #42
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk. Here are some tests.
Comment #43
aspilicious CreditAttribution: aspilicious commentedLooks good to me...
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commentedDoes #42 need manual testing? Looks like it's nearly ready to RTBC
Comment #45
Dave ReidI think this section conflicts with the recent test followups for the tel element and needs to be re-rolled.
Comment should explain why we're adding our disabled element here manually and not in the foreach() loop - because we need to set our default and hijack values as 'URLs' and not just any string values.
Comment #46
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRight, thanks. Rebased and tried to make the comment more descriptive.
Comment #47
aspilicious CreditAttribution: aspilicious commented#46: 1174630-html5-url-46.patch queued for re-testing.
Comment #48
aspilicious CreditAttribution: aspilicious commented#46: 1174630-html5-url-46.patch queued for re-testing.
Comment #50
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOh yes, #1174620: Add new HTML5 FAPI element: email got in :)
Rebased.
Comment #51
cosmicdreams CreditAttribution: cosmicdreams commentedAs a result of this patch should I be able to add a url field to a content type?
Comment #52
ericduran CreditAttribution: ericduran commented@cosmicdreams no, those are done in follow up issues.
Comment #53
cosmicdreams CreditAttribution: cosmicdreams commentedgotcha. I manually tested this patch and it applied fine. I'm now looking for ways to see the change in action. Is there any manual testing that is required?
Comment #54
aspilicious CreditAttribution: aspilicious commentedcosmicdreams, you can create a form api element in code
Comment #55
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOr
drush en form_test
(or hacking the module to be not hidden - it's located in core/modules/simpletest/test - or anything else that enables it)and visit form-test/url.(Edit: Yay, you said you have drush.)
Comment #56
cosmicdreams CreditAttribution: cosmicdreams commentedOK, finally got drush 5 to work with Drupal 8 and I was able to create this screenshot. Is this what I am supposed to be seeing?
Comment #57
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedActually it's supposed to look like that:
Can you try clearing the cache?
Comment #58
cosmicdreams CreditAttribution: cosmicdreams commentedNope that didn't work for me, you should get someone else to test.
Comment #59
xjmTagging for more testage. Be sure to clear your site cache after applying the patch. Also document which browser you are using.
Comment #60
cosmicdreams CreditAttribution: cosmicdreams commentedThere must have been something wrong with the way I applied the patch previously. I was able to apply the patch cleanly this morning and now I see that the form-teste/url is working properly
I vote for RTBC
Comment #61
aspilicious CreditAttribution: aspilicious commentedconsTructor
Just out of curiosity why isn't there an email tag in here?
Comment #62
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedFixed.
#email: Good catch. Looks like we forgot to add testcoverage for disabled email elements. I'll reopen #1174620: Add new HTML5 FAPI element: email.
Comment #63
aspilicious CreditAttribution: aspilicious commentedI don't want to followup this patch like we had to with email.
Are we sure we covered everything? (Placeholder, disabled, ....)
I have the feeling for example we don't test a correct url (without being trimmed).
Comment #64
aspilicious CreditAttribution: aspilicious commented#62: 1174630-html5-url-62.patch queued for re-testing.
Comment #65
ericduran CreditAttribution: ericduran commentedYea, we are missing a simple correct url test. Even though it is tested in other test, is not explicit.
Comment #66
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRebased and added tests for simple valid URLs. Tests for disabled elements and placeholders are there - that's why you could see email was missing in the context. So I think we are complete "testcoverage-wise".
Comment #67
aspilicious CreditAttribution: aspilicious commentedteam html5 gogogo!
ps: reviewed this *again*, didn't found any problems.
Comment #68
Dave ReidConfirmed this looks good to fly.
Comment #69
catchCommitted/pushed to 8.x, thanks!
Comment #70
Dave ReidWoot! Added this to the existing HTML5 FAPI change notification at http://drupal.org/node/1315186! Great work everyone!
Comment #71
sunGreat work, all! :)
Created a follow-up to improve the usability: #1473220: Allow users to omit the http:// prefix in URL form input elements