Posted by doublejosh on May 13, 2009 at 8:35pm
12 followers
| Project: | Compact Forms |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Noticed that when this module is applied to the user login form, and the browser auto-populates with past values, the labels are still placed over the top. Perhaps the check needs to be done "later."
Or perhaps this module needs a rewrite to use this type of approach...
<input value="title" onfocus="toggle('fieldId');" onblur="toggle('fieldId');" />...don't mean to second guess the code functionality. Several sorries.
Comments
#1
Marking as duplicate of #165608: Move the label text to the form field value. Please search for existing issues before submitting a new one. You can follow up on that issue instead to track its status. If any information from this issue is missing in the other issue, please make sure you provide it over there.
However, thanks for taking the time to report this issue.
#2
I'm marking this not a duplicate as I read through that other issue and it didn't seem to be related to the field autofill issue that I am also having. I would be more than happy to be proven wrong as long as it results in a solution.
Cheers,
Deciphered.
#3
Example image.
#4
I can confirm this in Google Chrome on Drupal 6.
#5
Here's a complete fix of this issue.
#6
+++ compact_forms.js 19 Sep 2009 17:29:49 -0000@@ -1,4 +1,4 @@
-// $Id: compact_forms.js,v 1.6 2009/08/06 20:27:22 sun Exp $
+// $Id: compact_forms.js,v 1.4 2009/05/06 23:43:39 sun Exp $
Are you sure you rolled that patch against the latest version in CVS HEAD?
I'm on crack. Are you, too?
#7
Sorry, my bad, copy-paste issue... Here's a correct patch.
#8
Even better patch attached here.
Some notes about the patch:
1. Hiding label instantly cures label flickering (regular .hide() doesn't work in safari).
2. Safari fills it's data without triggering any event, after full page load. So, I see no other way to handle it.
#9
Sorry, misspelled the function name.
#10
Probably the last patch. Firefox doesn't like .hide() by some reason.
#11
+++ compact_forms.js 19 Sep 2009 19:40:17 -0000@@ -19,7 +19,7 @@
if ($field.val() != '') {
- $label.fadeOut(1);
+ $label.css('display', 'none');
}
The next one who'll touch this code will replace this with .hide().
+++ compact_forms.js 19 Sep 2009 19:40:17 -0000@@ -50,6 +50,18 @@
+ // Chrome adds his passwords after page load, so we should track it on change.
Exceeds 80 chars.
+++ compact_forms.js 19 Sep 2009 19:40:17 -0000@@ -62,3 +74,19 @@
})(jQuery);
+
+// Safari leaves no other way to check auto-filled data.
+if ($.browser.safari) {
This compatibility code for Safari still needs to live within the jQuery closure, because $ is only safely available there.
btw, is $.browser still available in jQuery 1.3 ?
+++ compact_forms.js 19 Sep 2009 19:40:17 -0000@@ -62,3 +74,19 @@
+ $(window).load(function(){
$(window).load(), to my knowledge, is the same as $.ready(), so we should use that instead.
+++ compact_forms.js 19 Sep 2009 19:40:17 -0000@@ -62,3 +74,19 @@
+ function checkCompactForms() {
This helper function should have a JSDoc description explaining why it is needed, and the function name should be within the scope of Compact Forms and also could be more precise, either
Drupal.compactForms.fixSafari
or
Drupal.compactForms.onLoad
If you go for the latter, then the JSDoc should be generic, and inline comments should explain the tweak for Safari instead.
This review is powered by Dreditor.
#12
sun, you're killing me :)
#13
OK, heres a new one with your advices applied.
#14
Go ahead! I like your patch! :)
#15
Doesn't appear to resolve the issue I have.
When I have multiple accounts saved for a page, after I enter the user name it auto-populates the password field but doesn't hide the title, resulting in the image at #3
#16
Which browser do you use?
#17
Sorry, thought I mentioned it.
Firefox 3.5.3
#18
This patch did fix a couple bugs with Prefilled forms. But there's still a lingering issue with Autofill.
Video attached. Shows Firefox first then Safari.
http://screenr.com/DiB
#19
I hope this helps. If Firefox remembers your password and you clear the password field then shift+tab out to the username field and tab back in to the password field Firefox re-enters your password. This is not equal to '' and so the label doesn't fade out.
Below we store the default value (the prepopulated value from Firefox) and compare to this...
Around line 14
var $label = $(this);
var $field = $('#' + $label.attr('for'));
if ($field.attr('type') != 'text' && $field.attr('type') != 'password') {
return;
}
var $default_value = $field.val();
if ($default_value != '') {
$label.fadeOut(1);
}
Around line 40
$field.focus(function () {
if($(this).val() === $default_value || $(this).val() === '') {
$label.fadeOut('fast');
}
});
$field.blur(function () {
if($(this).val() === '') {
$label.fadeIn('slow');
}
});
I wonder however what would happen if you had more than one user's password stored
#20
#13 path work for me!!!
#21
Please test again with this one. Doesn't really change the functionality of the previous patch, but implements the changes a bit differently.
#22
Hi all, thanks for working on this. Still getting the issue with Firefox after applying #21.
#23
The same problem when you use the manager of passwords.
Ctr+Enter in my Opera 9.52
#24
patched #21 against 6.x-1.x-dev and works as expected for me. Would like to see this in an actual release if others can confirm it fixes the issue correctly.
#25
Actually seems to be working for me now! Maybe I patched against the wrong version. Also, never understood why this was marked critical.
#26
Is everyone talking about #21 ?
#27
Yes, I was certainly speaking of your patch #21. Works great. :-)
#28
I can confirm #21 works great with saved passwords (or without) in Firefox 3.6.3 and Chrome 6.0.408.1 dev
#29
Actually, please take a good look at #19 ... it is still an issue.
#30
Ok I have combined #19 and #21 - please review (does no body else get this problem??).
#31
Err... something went wrong with #30 ... another patch attached
#32
For what it's worth (in case more issues come up in the future), I have one site running the Acquia slate theme, which has a compact form functionality built in that handles auto populated fields / incorrect password errors beautifully. Before I came across this thread I was going to try and port the javascript into this or a custom module. Perhaps others would also be interested to see how they accomplished the task.
#33
I can't imagine their approach and code being vastly different to this module. However, a technical review and comparison would be nice to do. Ideally perhaps in a separate issue though, as this one is quite focused.
Not sure what happened here and what the status of this issue/patch is. It seems like some people confirmed that #21 works flawlessly. But there's a revised patch in #31 that accounts for some additional detail mentioned but not fully described in #19, and which has a lot of coding style issues that would need to be fixed.
Would it be a good idea to commit #21 and move the additional changes of #19/#31 into a new issue or follow-up patch?
#34
Patch in 21 works and is an improvement on what's currently in repository. Let's get that in and move forward with a new issue as you suggested.
#35
Thanks for reporting, reviewing, and testing! Committed to all branches.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.
#36
Awesome!
#37
Brilliant, I was getting tired of patching this! #19 still applies and I have supplied a patch in #903122: Autofill can cause both password field value and password field label to appear concurrently. - please review.
#38
Automatically closed -- issue fixed for 2 weeks with no activity.