Add support for contentEditable tags #1418
Conversation
|
Please add a unit test and make the build pass again |
| @@ -606,7 +606,12 @@ $.extend( $.validator, { | |||
| return element.validity.badInput ? false : $element.val(); | |||
| } | |||
|
|
|||
| val = $element.val(); | |||
| if (element.hasAttribute('contenteditable')) { | |||
|
Sorry! Total failure to read the contribution guidelines. X-X Should be fine now. Had to add one or two more lines because I forgot that I had some handling code in my own app which I've now moved to the plugin. |
|
@dorner dont worry thanks for your contribution. Could you have another look because the build is still failing.. |
|
Gak, forgot to run grunt after I fixed the tests -_- Everything should be fine now. |
| @@ -549,20 +549,26 @@ $.extend( $.validator, { | |||
|
|
|||
| // select all valid inputs inside the form (no submit or reset buttons) | |||
| return $( this.currentForm ) | |||
| .find( "input, select, textarea" ) | |||
| .find( "input, select, textarea, [contenteditable]" ) | |||
There was a problem hiding this comment.
@jzaefferer not sure, but could this get a perf problem?
|
Hi @dorner, I've tried to use your code but get an "TypeError: e is undefined" by clicking into the contenteditable. Any suggestions? Thanks, Jakob |
|
Haven't seen that personally... can you open Firebug / Chrome Tools and give me a stack trace? |
|
Any updates on this? I'm looking for this feature as well |
|
@CameronJ did you test the changes from this PR? Do those work for you in a x-browser manner? I am fine with merging it, in case it gets rebased in case you confirm it works. |
| .not( ":submit, :reset, :image, [disabled]" ) | ||
| .not( this.settings.ignore ) | ||
| .filter( function() { | ||
| if ( !this.name && validator.settings.debug && window.console ) { | ||
| var name = this.name || $(this).attr("name"); // for contenteditable |
There was a problem hiding this comment.
Please use tabs instead of spaces. Also, please add one space after ( and one before ) in $(this).attr("name")
|
Whitespace issues should be fixed. |
| .not( ":submit, :reset, :image, [disabled]" ) | ||
| .not( this.settings.ignore ) | ||
| .filter( function() { | ||
| if ( !this.name && validator.settings.debug && window.console ) { | ||
| var name = this.name || $( this ).attr("name"); // for contenteditable |
There was a problem hiding this comment.
var name = this.name || $( this ).attr("name");
↑ ↑
// one space after ( and one before )
// =>
var name = this.name || $( this ).attr( "name" );(same as bellow)
|
Otherwise, it LGTM :) The only thing needed after adjusting as per comments, is to rebase your branch on top of master (upstream) Thanks :) |
Currently the validation library only works on classic form elements. However, there is no reason it can't work on rich text elements that have names and use contentEditable. This change adds support for these tags.
dorner
force-pushed
the
content-editable
branch
from
8c2228c
to
b0ac381
Compare
|
OK, think everything should be fine now. 8-) |
|
@CameronJ does this patch work for you? |
| console.error( "%o has no name assigned", this ); | ||
| } | ||
|
|
||
| // set form expando on contenteditable | ||
| if ( this.hasAttribute( "contenteditable" ) ) { |
There was a problem hiding this comment.
Please add a comment that we use hasAttribute to support IE and mention the version
There was a problem hiding this comment.
Other way around - I mentioned above that hasAttribute is a clean way of checking but it does not support IE versions before 8. In my company we've dropped support of IE7 a while ago, not sure if you want me to comment on this or not.
There was a problem hiding this comment.
Ah, misread it. I am fine with not supporting IE7
There was a problem hiding this comment.
I believe hasAttribute isn't supported completely by IE8 either.
This may be better written as: $(this).is( "[contenteditible]" )
|
@staabm I have not tested this, I was just looking to see if there was an PR or feature request logged for this feature, and found this thread and realized it was a bit stagnant for awhile. LOVE jQuery Validate, and am about to integrate a different wysiwyg editor which only uses div with content editable and would like to avoid using a different validation method for just that one field. On this site though, I only need support for current browser versions and one prior, so my cross browser requirements are a lot less stringent than the majority of implementations. Let me know if you would like me to do some rudimentary testing on this anyway, even though it will cover a very limited set of browsers & versions. |
|
@CameronJ would be great if you could test this PR so we have another pair of eyeballs checking things are working as expected |
|
Fixed the whitespace issue again. -_- |
|
Sorry guys, I didn't get a chance to look at this today. I will look at it tomorrow and verify the PR. Looking at the code that changed, I do not see anything that would be a problem at all for me... But, Element.hasAttribute is only supported fully by IE9+, not IE8. I added this comment inline in 2 places above, where hasAttribute is being used. Thanks for restarting this PR and helping get it merged! |
|
Really? Everything I've seen says it is supported unless you're in "IE7 Standards mode". Just curious what your source is? |
|
I was using W3Schools for my references, I have often found them to be accurate; however, if the MSDN page shows that it's supported in IE8, then it sounds like W3Schools may have gotten this one wrong. |
Add support for contentEditable tags
|
I have run the qunit tests on a couple different browsers and everything is passing and looking good. Also, I did a very rudimentary integration of this and everything seems to be working as expected. |
|
Yay! 8D |
Currently the validation library only works on classic form elements. However, there is no reason it can't work on rich text elements that have names and use contentEditable. This change adds support for these tags.