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.
CommentFileSizeAuthor
#33 captcha.patch2.5 KBelachlan
#24 captcha-6.x-2.3-branch-[881156].patch2.65 KBelachlan
#23 captcha-6.x-2.3-branch-[881156].patch2.71 KBelachlan
#19 captcha-[881156].patch2.59 KBelachlan
#17 captcha-[881156].patch691 byteselachlan
#15 captcha-[881156].patch690 byteselachlan
#12 captcha-[881156].patch690 byteselachlan
#6 captcha-[881156].patch1.26 KBpavel.karoukin
#2 captcha-[893170].patch1.29 KBvivekkhurana

Comments

soxofaan’s picture

soxofaan commented
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

vivekkhurana commented
StatusFileSize
new captcha-[893170].patch1.29 KB

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

soxofaan’s picture

soxofaan commented
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

soxofaan commented
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

pavel.karoukin commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new captcha-[881156].patch1.26 KB

Ported to D7

soxofaan’s picture

soxofaan commented
Status: Needs review » Fixed

Committed: http://drupal.org/cvs?commit=448672

thanks!

elachlan’s picture

elachlan commented
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

soxofaan commented
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

pavel.karoukin commented

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

elachlan commented

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

elachlan commented
StatusFileSize
new captcha-[881156].patch690 bytes

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

elachlan’s picture

elachlan commented
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

elachlan commented
Status: Needs work » Needs review
StatusFileSize
new captcha-[881156].patch690 bytes

Resubmitting patch. Hopefully fixed.

Status: Needs review » Needs work

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

elachlan’s picture

elachlan commented
Status: Needs work » Needs review
StatusFileSize
new captcha-[881156].patch691 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

elachlan commented
Status: Needs work » Needs review
StatusFileSize
new captcha-[881156].patch2.59 KB

OK, now it should work.

pavel.karoukin’s picture

pavel.karoukin commented

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

elachlan commented

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

gregarios commented
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

elachlan commented
Version: 7.x-1.x-dev » 6.x-2.x-dev
StatusFileSize
new captcha-6.x-2.3-branch-[881156].patch2.71 KB

6.x-2.x branch patch.

elachlan’s picture

elachlan commented
StatusFileSize
new captcha-6.x-2.3-branch-[881156].patch2.65 KB

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

pavel.karoukin commented

@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

soxofaan commented

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

elachlan commented

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

elachlan commented

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

elachlan commented

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

powery commented

subscribe

apprayo’s picture

apprayo commented
Issue tags: -autocomplete, -xhtml strict, -low-hanging fruit

subscribe

elachlan’s picture

elachlan commented
Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new captcha.patch2.5 KB

Finally a Patch.

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

Fingers Crossed!

elachlan’s picture

elachlan commented

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

soxofaan commented
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.