Mess with maxlength only if specified #56
Conversation
It seems the previous patch was causing problems in Firefox 3.6 since it was returning -1 for the value of the maxlength attribute if the attribute was never supplied in the first place. This patch tries to detect if the maxlength was supplied, and only proceeds to manipulate it if it is present.
|
If it's useful, I tested in firefox 3.6 and ie8 and it works for both. |
| elem.removeAttribute("maxLength"); | ||
| if (elem.hasAttribute("maxLength")) { | ||
| maxLength = elem.getAttribute("maxLength") || elem.maxLength; | ||
| if (parseInt(maxLength, 10) > 0) { |
There was a problem hiding this comment.
Is parseInt needed here? The > operator should coerce its operands to Number. If it is needed, why is it not needed on line 91?
|
Thanks for this, and apologies for not testing the original PR thoroughly enough. I've added one small comment to the diff. Once that's cleared up I'll get this tested properly and merged as soon as possible. |
Mess with maxlength only if specified
|
I merged this but unfortunately noticed when testing that |
|
Hi @jamesallardice, I am extremely sorry for the poor quality initial pull-request and the rushed second one. I totally missed your comment on the diff and didn't realize that Your fix will work fine. However, I have realized that the initial logic in my initial pull-request could have been refined. I'll work on a better refinement that supports IE6 and up as well, and send you another pull-request. Thanks for all your efforts. |
It seems the previous patch was causing problems in Firefox 3.6 since it
was returning -1 for the value of the maxlength attribute if the
attribute was never supplied in the first place.
This patch tries to detect if the maxlength was supplied, and only
proceeds to manipulate it if it is present.
Note: Unfortunately, I had no time to test this change in an actual browser. Please, please, test it in IE8 and Firefox 3.6 before merging. Thanks and sorry for the extra work.