Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with input field, maxlength attribute and long placeholder text #36

Merged
merged 2 commits into from
Sep 25, 2013

Conversation

paracycle
Copy link
Contributor

The current code causes a problem when the input field has a maxlength attribute and the placeholder text is longer than that length. The current pull request solves this by clearing (and storing) the maxlength value when the placeholder is shown and restoring it when the placeholder should be hidden.

While achieving this, I also removed code duplication in makeKeyupHandler.

Ufuk Kayserilioglu added 2 commits September 25, 2013 16:07
- If we don't get rid of the `maxlength` attribute on the input element,
  sometimes it's the case that the element doesn't accept any new input
  to clear the placeholder because max-length is already exceeded.
@jamesallardice
Copy link
Owner

Thanks! I have no idea how this has not been discovered before now. Nice work. I'm going to merge this in but then make a couple of minor changes, just to bring the code style inline with the rest of the file (e.g. a single var statement per scope).

Thanks again.

jamesallardice pushed a commit that referenced this pull request Sep 25, 2013
Problem with input field, maxlength attribute and long placeholder text
@jamesallardice jamesallardice merged commit 971668c into jamesallardice:master Sep 25, 2013
@paracycle
Copy link
Contributor Author

Hey, thanks for the prompt response. I guess the requirements to recreate the case wouldn't be all that common. I just had a 11 char maxlength text field with a long placeholder text. At first, I thought the library was not working properly at all. 😄

By the way, it is a really nice library, saved me a ton of work. Thanks. 🙏

Also, (this is a bit unrelated) would you mind including the build folder in the repository? That way we can just install the library with bower and move on.

@jamesallardice
Copy link
Owner

Thanks, I'm glad you like it 😃 are you using it on a live site that I can see? It's always nice to see it in action on real sites.

I am planning to get it on Bower soon. I want to make a few more small changes first, but it should be available soon. In the meantime the built version (and unminified version) can be downloaded from the site on the gh-pages branch: http://jamesallardice.github.io/Placeholders.js

@paracycle
Copy link
Contributor Author

The site is not live at the moment, but we are putting the final touches (which involves adding IE compliance, hence your polyfill).

It seems Placeholders.js is already on Bower http://sindresorhus.com/bower-components/#!/search/Placeholders.js (somebody must have registered it). All we need to use it directly is for the build folder to be in the repo. I already did this for my own fork and am working from it flawlessly.

@leejordan
Copy link

I think this pull request might have caused a bug in firefox3.6 whereby if an input doesn't have a maxlength set, the placeholder.js data attribute data-placeholder-maxlength is set to 0.

This renders the input unuseable in firefox3.6 and adding a maxlength attribute makes it work again (at least according to my limited testing so far)

@paracycle
Copy link
Contributor Author

@leejordan I see. I guess my wrong assumption was to think that

var maxLength = elem.getAttribute(ATTR_MAXLENGTH);

would return null if the attribute didn't exist. According to your report, this seems not to be the case in Firefox 3.6.

@jamesallardice A quick fix I can think of is to first check to see if the attribute is actually there, ie:

var maxLength = elem.hasAttribute(ATTR_MAXLENGTH) && elem.getAttribute(ATTR_MAXLENGTH);

What do you think?

@leejordan
Copy link

I just confirmed this by switching back to 2.1.0 (before this pull request was merged) - and it works perfectly in firefox 3.6 now.

@leejordan
Copy link

looks like elem.getAttribute(ATTR_MAXLENGTH) = -1 on firefox3.6 for inputs without a maxlength field. This gives me the hasty fix I need just now:

        if (maxLength != -1) {
            elem.setAttribute("maxLength", maxLength);
            elem.removeAttribute(ATTR_MAXLENGTH);
        }

@paracycle
Copy link
Contributor Author

Good find, however, I suggest you at least do the check like:

if (maxLength > 0)

@leejordan
Copy link

yea just corrected it to that. Will you make a new pull request out of interest? Thanks for your help.

@paracycle
Copy link
Contributor Author

@leejordan Just created Pull Request #56

@leejordan
Copy link

Thanks very much for your help, saved me the effort :)

@paracycle
Copy link
Contributor Author

Unfortunately, I didn't have any time to test it. Could you please test in FF3.6 and IE and update the PR with your findings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants