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)?

CommentFileSizeAuthor
#66 1174630-html5-url-66.patch16.43 KBNiklas Fiekas
#66 1174630-interdiff-66.txt2 KBNiklas Fiekas
#62 1174630-interdiff-62.txt496 bytesNiklas Fiekas
#62 1174630-html5-url-62.patch15.99 KBNiklas Fiekas
#60 Screenshot at 2012-03-03 08:58:02.png9.5 KBcosmicdreams
#57 html5-url-fields.png6.78 KBNiklas Fiekas
#56 Screenshot at 2012-03-02 18:20:34.png9.51 KBcosmicdreams
#50 1174630-html5-url-50.patch15.99 KBNiklas Fiekas
#46 1174630-html5-url-46.patch16.12 KBNiklas Fiekas
#42 1174630-html5-url-42.patch15.61 KBNiklas Fiekas
#42 1174630-interdiff-42.txt3.03 KBNiklas Fiekas
#35 1174630-html5-url-35.patch13.47 KBericduran
#32 1174630-html5-url-32.patch13.03 KBNiklas Fiekas
#32 1174630-interdiff-32.txt3.74 KBNiklas Fiekas
#27 1174630-27.patch11.76 KBericduran
#25 url_element-1174630-25.patch9.88 KBxjm
#15 1174630-html5-url-element.patch12.58 KBDave Reid
#12 1174630-html5-url-element.patch12.2 KBDave Reid
#11 1174630-html5-url-element.patch12.15 KBDave Reid
#9 1174630-html5-url-element.patch12.15 KBDave Reid
#7 1174630-html5-url-element.patch12.15 KBDave Reid
#6 1174628-html5-url-element.patch10.98 KBDave Reid
#4 1174628-html5-url-element.patch10.98 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Assigned: Unassigned » Dave Reid
ericduran’s picture

ericduran CreditAttribution: ericduran commented

We 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.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Yeah 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).

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Active » Needs review
FileSize
1174628-html5-url-element.patch10.98 KB

Status: Needs review » Needs work

The last submitted patch, 1174628-html5-url-element.patch, failed testing.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Needs review
FileSize
1174628-html5-url-element.patch10.98 KB
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
1174630-html5-url-element.patch12.15 KB

Incorrectly named validation function.

Status: Needs review » Needs work

The last submitted patch, 1174630-html5-url-element.patch, failed testing.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Needs review
FileSize
1174630-html5-url-element.patch12.15 KB

Status: Needs review » Needs work

The last submitted patch, 1174630-html5-url-element.patch, failed testing.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Needs review
FileSize
1174630-html5-url-element.patch12.15 KB
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
1174630-html5-url-element.patch12.2 KB

Dropbox did not sync in time...

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

Does the maxlength of 128 even need to be defined? If so, it should probably be longer than that, as some URLs are quite long.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I'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.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
1174630-html5-url-element.patch12.58 KB
ericduran’s picture

ericduran CreditAttribution: ericduran commented
Status: Needs review » Needs work

@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.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I don't see why not since this is essentially a text field anyway, it should probably support autocompletion.

ericduran’s picture

ericduran CreditAttribution: ericduran commented
Status: Needs work » Needs review
kingandy’s picture

kingandy CreditAttribution: kingandy commented

Autocomplete 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.

attiks’s picture

attiks CreditAttribution: attiks commented

subscribing for #1239910: META: tracking other issues about validation

ericduran’s picture

ericduran CreditAttribution: ericduran commented
Issue tags: +HTML5 Sprint: August 2011 - 2
Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Issue tags: -HTML5 Sprint: August 2011 - 2

Cleaning up tags.

Everett Zufelt’s picture

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.

kingandy’s picture

kingandy CreditAttribution: kingandy commented

I 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 :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
url_element-1174630-25.patch9.88 KB

Rerolled for core/ and the death of profile.module.

ericduran’s picture

ericduran CreditAttribution: ericduran commented

This 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.

ericduran’s picture

ericduran CreditAttribution: ericduran commented
FileSize
1174630-27.patch11.76 KB

Here'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.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Make sure to check this CSS too :)

ericduran’s picture

ericduran CreditAttribution: ericduran commented

Yea, 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.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Side 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.

Status: Needs review » Needs work

The last submitted patch, 1174630-27.patch, failed testing.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs work » Needs review
FileSize
1174630-interdiff-32.txt3.74 KB
1174630-html5-url-32.patch13.03 KB
  • Rebased onto 8.x and fixed merge conflicts.
  • Fixed CSS. Not taking into account #30, yet.
  • Fixed test failures.
ericduran’s picture

ericduran CreditAttribution: ericduran commented
Status: Needs review » Reviewed & tested by the community

So 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.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Reviewed & tested by the community » Needs work

We're missing tests to confirm the trimming behavior like are present on the e-mail patch.

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1205,6 +1205,14 @@ function _form_test_disabled_elements($form, &$form_state) {
     );
   }
 
+  // URL.
+  $form['disabled_container']['disabled_container_url']= array(
+    '#type' => 'url',
+    '#title' => 'url',
+    '#default_value' => 'http://example.com',
+    '#test_hijack_value' => 'http://example.com/foo',
+  );
+
   // Text format.
   $form['text_format'] = array(
     '#type' => 'text_format',

What is this addition? I couldn't find anything similar in the 'tel' FAPI patch that went in?

ericduran’s picture

ericduran CreditAttribution: ericduran commented
Status: Needs work » Needs review
FileSize
1174630-html5-url-35.patch13.47 KB

Yea, 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.

ericduran’s picture

ericduran CreditAttribution: ericduran commented
Status: Needs review » Needs work

Actually it's still missing the trim test.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I 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. :(

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Issue tags: +sprint

Adding to current sprint. So in summary, we still need tests to confirm validation and trimming of this field.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Issue tags: +Needs tests

I 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. :(

Yep, exactly. So ingnore #35 or were there any other changes?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

We're still missing a test to confirm that the form submits a trimmed URL field value.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented

Of course. Thus "+Needs tests". I was talking about the patch in #35.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1174630-interdiff-42.txt3.03 KB
1174630-html5-url-42.patch15.61 KB

Ok. Here are some tests.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Looks good to me...

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

Does #42 need manual testing? Looks like it's nearly ready to RTBC

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Needs work 
+++ b/core/modules/simpletest/tests/form.testundefined
@@ -261,7 +264,7 @@ class FormsTestCase extends DrupalWebTestCase {
 
     // All the elements should be marked as disabled, including the ones below
     // the disabled container.
-    $this->assertEqual(count($disabled_elements), 33, t('The correct elements have the disabled property in the HTML code.'));
+    $this->assertEqual(count($disabled_elements), 34, 'The correct elements have the disabled property in the HTML code.');
 
     $this->drupalPost(NULL, $edit, t('Submit'));
     $returned_values['hijacked'] = drupal_json_decode($this->content);
@@ -397,7 +400,7 @@ class FormElementTestCase extends DrupalWebTestCase {

@@ -397,7 +400,7 @@ class FormElementTestCase extends DrupalWebTestCase {
     $expected = 'placeholder-text';
     // Test to make sure textfields and passwords have the proper placeholder
     // text.
-    foreach (array('textfield', 'password') as $type) {
+    foreach (array('textfield', 'url', 'password') as $type) {
       $element = $this->xpath('//input[@id=:id and @placeholder=:expected]', array(
         ':id' => 'edit-' . $type,
         ':expected' => $expected,

I think this section conflicts with the recent test followups for the tel element and needs to be re-rolled.

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1205,6 +1244,14 @@ function _form_test_disabled_elements($form, &$form_state) {
+  // URL.

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.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs work » Needs review
FileSize
1174630-html5-url-46.patch16.12 KB

Right, thanks. Rebased and tried to make the comment more descriptive.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Issue tags: -html5, -sprint

#46: 1174630-html5-url-46.patch queued for re-testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

#46: 1174630-html5-url-46.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +html5, +sprint

The last submitted patch, 1174630-html5-url-46.patch, failed testing.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs work » Needs review
FileSize
1174630-html5-url-50.patch15.99 KB

Oh yes, #1174620: Add new HTML5 FAPI element: email got in :)

Rebased.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

As a result of this patch should I be able to add a url field to a content type?

ericduran’s picture

ericduran CreditAttribution: ericduran commented

@cosmicdreams no, those are done in follow up issues.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

gotcha. 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?

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

cosmicdreams, you can create a form api element in code

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented

Or 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.)

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented
FileSize
Screenshot at 2012-03-02 18:20:34.png9.51 KB

OK, 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?

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
FileSize
html5-url-fields.png6.78 KB

Actually it's supposed to look like that:
html5-url-fields.png
Can you try clearing the cache?

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

Nope that didn't work for me, you should get someone else to test.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Needs manual testing

Tagging for more testage. Be sure to clear your site cache after applying the patch. Also document which browser you are using.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented
FileSize
Screenshot at 2012-03-03 08:58:02.png9.5 KB

There 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

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented 
+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1137,6 +1143,39 @@ function form_test_email_submit($form, &$form_state) {
+ * Form consructor for testing #type 'url' elements.

consTructor

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -1235,7 +1274,7 @@ function _form_test_disabled_elements($form, &$form_state) {
+  foreach (array('textfield', 'textarea', 'hidden', 'tel', 'url') as $type) {

Just out of curiosity why isn't there an email tag in here?

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
FileSize
1174630-html5-url-62.patch15.99 KB
1174630-interdiff-62.txt496 bytes

Fixed.

#email: Good catch. Looks like we forgot to add testcoverage for disabled email elements. I'll reopen #1174620: Add new HTML5 FAPI element: email.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

I 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).

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

#62: 1174630-html5-url-62.patch queued for re-testing.

ericduran’s picture

ericduran CreditAttribution: ericduran commented
Status: Needs review » Needs work

Yea, we are missing a simple correct url test. Even though it is tested in other test, is not explicit.

Niklas Fiekas’s picture

Niklas Fiekas CreditAttribution: Niklas Fiekas commented
Status: Needs work » Needs review
FileSize
1174630-interdiff-66.txt2 KB
1174630-html5-url-66.patch16.43 KB

Rebased 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".

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

team html5 gogogo!

ps: reviewed this *again*, didn't found any problems.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Confirmed this looks good to fly.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Woot! Added this to the existing HTML5 FAPI change notification at http://drupal.org/node/1315186! Great work everyone!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Great work, all! :)

Created a follow-up to improve the usability: #1473220: Allow users to omit the http:// prefix in URL form input elements

Automatically closed -- issue fixed for 2 weeks with no activity.