Closed (fixed)
Project:
Compact Forms
Version:
6.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
13 May 2009 at 20:35 UTC
Updated:
20 Sep 2010 at 00:10 UTC
Jump to comment: Most recent file
Comments
Comment #1
sunMarking 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.
Comment #2
decipheredI'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.
Comment #3
decipheredExample image.
Comment #4
cookieofdoom commentedI can confirm this in Google Chrome on Drupal 6.
Comment #5
neochief commentedHere's a complete fix of this issue.
Comment #6
sunAre you sure you rolled that patch against the latest version in CVS HEAD?
I'm on crack. Are you, too?
Comment #7
neochief commentedSorry, my bad, copy-paste issue... Here's a correct patch.
Comment #8
neochief commentedEven 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.
Comment #9
neochief commentedSorry, misspelled the function name.
Comment #10
neochief commentedProbably the last patch. Firefox doesn't like .hide() by some reason.
Comment #11
sunThe next one who'll touch this code will replace this with .hide().
Exceeds 80 chars.
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 ?
$(window).load(), to my knowledge, is the same as $.ready(), so we should use that instead.
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.
Comment #12
neochief commentedsun, you're killing me :)
Comment #13
neochief commentedOK, heres a new one with your advices applied.
Comment #14
sunGo ahead! I like your patch! :)
Comment #15
decipheredDoesn'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
Comment #16
neochief commentedWhich browser do you use?
Comment #17
decipheredSorry, thought I mentioned it.
Firefox 3.5.3
Comment #18
davidburnsThis 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
Comment #19
mstrelan commentedI 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
Around line 40
I wonder however what would happen if you had more than one user's password stored
Comment #20
Andrew Gorokhovets commented#13 path work for me!!!
Comment #21
sunPlease test again with this one. Doesn't really change the functionality of the previous patch, but implements the changes a bit differently.
Comment #22
dafederHi all, thanks for working on this. Still getting the issue with Firefox after applying #21.
Comment #23
Andrew Gorokhovets commentedThe same problem when you use the manager of passwords.
Ctr+Enter in my Opera 9.52
Comment #24
digitalfrontiersmediapatched #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.
Comment #25
dafederActually seems to be working for me now! Maybe I patched against the wrong version. Also, never understood why this was marked critical.
Comment #26
sunIs everyone talking about #21 ?
Comment #27
digitalfrontiersmediaYes, I was certainly speaking of your patch #21. Works great. :-)
Comment #28
mstrelan commentedI can confirm #21 works great with saved passwords (or without) in Firefox 3.6.3 and Chrome 6.0.408.1 dev
Comment #29
mstrelan commentedActually, please take a good look at #19 ... it is still an issue.
Comment #30
mstrelan commentedOk I have combined #19 and #21 - please review (does no body else get this problem??).
Comment #31
mstrelan commentedErr... something went wrong with #30 ... another patch attached
Comment #32
mark. commentedFor 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.
Comment #33
sunI 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?
Comment #34
davidburnsPatch 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.
Comment #35
sunThanks 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.
Comment #36
doublejosh commentedAwesome!
Comment #37
mstrelan commentedBrilliant, 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.