Browser autocomplete feature does not seem desirable for the capcha response form field.

Index: image_captcha.module
===================================================================
--- image_captcha.module	(revision 690)
+++ image_captcha.module	(working copy)
@@ -230,7 +230,6 @@
           '#weight' => 0,
           '#required' => TRUE,
           '#size' => 15,
-          '#attributes' => array('autocomplete' => 'off')
         );
 
         // Handle the case insensitive validation option combined with ignoring spaces.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

soxofaan’s picture

Component: Image Captcha (image_captcha) » User interface
Status: Needs review » Needs work
Issue tags: +captcha, +low-hanging fruit

Good idea.

Should also be applied to simple math CAPTCHA.

Flagging it as "low-hanging fruit", so aspirant co-maintainers can tackle this one

vivekkhurana’s picture

FileSize
1.29 KB

Patch attached which adds autocomplete off to image_captcha and math captcha.

soxofaan’s picture

Version: 6.x-2.3-rc2 » 6.x-2.x-dev
Status: Needs work » Needs review

Hi Vivek,

Thanks for your patch!
The CAPTCHA module has simpletest coverage, so when you submit a patch, you should set the status to "needs review" so the automated testing system can do its job.

Status: Needs review » Needs work

The last submitted patch, captcha-[893170].patch, failed testing.

soxofaan’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Needs work » Patch (to be ported)
Issue tags: -captcha

Ok, the patch looked all right to me so I committed it:
http://drupal.org/cvs?commit=439220
I committed it before the test system kicked in and that's probably why it reported failure of applying the path.

Anyway: thanks a lot and to be ported to D7

pavel.karoukin’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.26 KB

Ported to D7

soxofaan’s picture

Status: Needs review » Fixed
elachlan’s picture

Status: Fixed » Needs work

This change breaks XHTML compliance.

It is suggested in this article to use !ATTLIST just after the Doctype - https://wiki.mozilla.org/The_autocomplete_attribute_and_web_documents_us...

eg.

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd" [
<!ATTLIST form autocomplete CDATA #IMPLIED>
]>
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
soxofaan’s picture

Title: Add autocomplete off for captcha_response form field » XHTML compliance of "autocomplete=off" on captcha_response form field
Issue tags: +xhtml strict

(changing title accordingly)

pavel.karoukin’s picture

With that, my xhtml properly validates, no need to create a custom DTD. Only gotcha is that you must send the application/xhtml+xml header with the document, but if you are using xhtml, you should be sending that header anyway.

So #8 solution will work only if page served as XHTML page. If it served as HTML page - this will still fail?

If so, may be we should stick to JS solution? I.e. injecting something like:

  document.getElementById( "MyInput" ).setAttribute( "autocomplete","off" )
elachlan’s picture

Add this to the captcha.js file

	//Turn off Auto-Complete for input feild 
	$("#edit-captcha-response").attr("autocomplete", "off");

How can we run a test for it?

elachlan’s picture

FileSize
690 bytes

Patch with changes. I did it manually as I don't have CVS.

elachlan’s picture

Status: Needs work » Needs review

Patch needs tests.

Also the original changes will need to be reverted.

Status: Needs review » Needs work

The last submitted patch, captcha-[881156].patch, failed testing.

elachlan’s picture

Status: Needs work » Needs review
FileSize
690 bytes

Resubmitting patch. Hopefully fixed.

Status: Needs review » Needs work

The last submitted patch, captcha-[881156].patch, failed testing.

elachlan’s picture

Status: Needs work » Needs review
FileSize
691 bytes

Learning the hard way not to do it manually. If this fails I'm going to leave it to someone else.

Status: Needs review » Needs work

The last submitted patch, captcha-[881156].patch, failed testing.

elachlan’s picture

Status: Needs work » Needs review
FileSize
2.59 KB

OK, now it should work.

pavel.karoukin’s picture

elachlan,

Few suggestions:
1) Make sure to use jQuery() instead $(). It will be more safe to use this way with other JS libraries which can overtake $()
2) Why "captchaAdmin"? I believe something like plain Drupal.behaviors.Captcha would be better, or may be this behavior already taken by something?

elachlan’s picture

it should be similar to image_captcha.js

//TODO: change this to Drupal.behaviors.captchaadmin = function (context) {}
 $(document).ready(function(){
 
+	//Turn off Auto-Complete for input feild
+	$("#edit-captcha-response").attr("autocomplete", "off");

But as you can see we are moving it to "captchaadmin". I believe it has the same function as document.ready?

Conversion of the image_captcha.js would be a good idea soon as well.

Edit: I believe jQuery is used, it references it at the end of the file. Also $() is used throughout the file.

gregarios’s picture

Priority: Normal » Major

XHTML compliance should be extended to the 6.x-2.3 branch as well, as it is required by many professional website developers and customers. Raising importance, since it is also important to have autocomplete turned off. The JQuery method in #21 should work well to fix the compliance issues.

Note: The $(document).ready(function(){ is the generic way of naming any function that is to happen after the document loads (in JQuery).

elachlan’s picture

Version: 7.x-1.x-dev » 6.x-2.x-dev
FileSize
2.71 KB

6.x-2.x branch patch.

elachlan’s picture

Sorry patch format.

Status: Needs review » Needs work

The last submitted patch, captcha-6.x-2.3-branch-[881156].patch, failed testing.

pavel.karoukin’s picture

@gregarios, in Drupal it's prefered to use Drupal.behaviors instead $(document).ready(function(){

Here you can read more with some nice examples - http://www.nicklewis.org/drupal-hackers-cookbook/jquery-js/unlocking-dru...

soxofaan’s picture

Hi all,

some remarks:

  • I just committed http://drupal.org/cvs?commit=452052 so that the JavaScript attaching in the CAPTCHA and Image CAPTCHA module works as encouraged by Drupal coding standards (http://drupal.org/update/modules/5/6#javascript-behaviors). This should clear up some confusion in this thread.
  • As already noted, Drupal.behaviors.captchaAdmin, should be limited to JavaScript magic for the CAPTCHA admin pages. The JavaScript for adding the autocomplete attribute should be in a (new) "captcha" behavior, e.g.:
    Drupal.behaviors.captcha = function (context) {
      // Turn off autocompletion  for the CAPTCHA response field
      $("#edit-captcha-response").attr("autocomplete", "off");
    }
    
  • Also note that "captcha.js" is only added to the CAPTCHA administration page, see "captcha.admin.inc", around line 44:
      // Use javascript for some added usability on admin form.
      drupal_add_js(drupal_get_path('module', 'captcha') .'/captcha.js');
    

    If the JavaScript for adding the autocompletion should also work on non-administration pages (duh), it should also be added to pages with a CAPTCHA. For example, add a drupal_add_js call in the captcha_process function (I'm thinking out loud here, it should be tested for real).

elachlan’s picture

If you move it to the captcha.inc you won't need to have the same code in administration.

This is because the captcha.admin.inc includes the captch.inc file.

Would this be done in captcha_set_form_id_setting? Where is the captcha_process you are talking about?

elachlan’s picture

http://dev.w3.org/html5/spec/common-input-element-attributes.html#the-au...

Does this mean that HTML 5 makes autocomplete valid html??

If so should we still fix it or just leave it?

elachlan’s picture

In image_captcha.user.inc on line 44 I think this should be inserted.

<?php
	// Add javascript
    drupal_add_js(drupal_get_path('module', 'image_captcha') . '/image_captcha.js');
?>

This is because its just before the image gets sent to the user?

I feel this is wrong because it should be standardised across all captcha implementations.

Should we place it in captcha.module instead in the captcha_captcha function which is an implementation of hook_captcha()

You could place the code just before the break, I am just guessing here, does this make send?

I am very lost in this code. Help would be appreciated.

powery’s picture

subscribe

apprayo’s picture

subscribe

elachlan’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Needs work » Needs review
FileSize
2.5 KB

Finally a Patch.

I think I worked it out. Not 100% Sure though :S

Fingers Crossed!

elachlan’s picture

ok cool passed the test, can I get someone to independently verify that it works??

It might be a good idea to separate the jQuery for admin and user.

What does everyone else think??

soxofaan’s picture

Status: Needs review » Fixed

moved the drupal_add_js() from _captcha_insert_captcha_element() to captcha_element_process() from elechlan's patch in #33 and committed:
http://drupal.org/cvs?commit=495694 (6.x-2.x)
http://drupal.org/cvs?commit=495712 (7.x-1.x)

thanks everyone

Status: Fixed » Closed (fixed)

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