NicolasCARPi / jquery_jeditable

jQuery edit in place plugin. Extendable via plugin architecture. Plugins for plugin. Really.
https://jeditable.elabftw.net
MIT License
1.74k stars 458 forks source link

reset replaces field as html which removes ability to later edit markup #203

Closed mcbmcb0 closed 5 years ago

mcbmcb0 commented 5 years ago

when reset is called (ie, cancel button) the contents of the edited field are replaced by the .html() of the original (this.revert). so if I'm editing text with tags then the markup is effectively removed from later editing. the problem is Line 587: $(self).html(self.revert);

if this was $(self).text(self.revert); then the contents of the field would be replaced as they originally were - markup displayed and editable.

could this be changed? or an option added on how reset functions?

I've tried working around it by returning false but then can not edit that element again: onedit: function(settings, original) { revert = $(this).text(); }, onreset: function(settings, original) { $(this).text(revert); return false; },

Thanks

NicolasCARPi commented 5 years ago

What version are you using? We don't have the same line 587.

mcbmcb0 commented 5 years ago

oops. i meant Line 458. latest version off github. it doesn't include a version number

NicolasCARPi commented 5 years ago

The thing I'm not sure about, is if this was made on purpose or not (keep in mind I didn't write this lib, I'm just the maintainer). IMHO it makes sense to remove the html bits of the inputs. Users should never have to see/edit HTML.

Now maybe having an option that doesn't trim html tags would be beneficial, idk.

it doesn't include a version number

FYI yes it does, it's in package.json.

mcbmcb0 commented 5 years ago

i get your point. don't want it turning into another ck or tinymce. the problem with the current behaviour is if you edit a field with tags, cancel, then edit again the tags will be gone. So the content is modified even though editing was cancelled.

in a way this issue is an extension of https://github.com/NicolasCARPi/jquery_jeditable/issues/110 relating to encoded edits, where self.revert was changed from .html() to .text()

I can think of a couple of solutions: 1- create an option for editing raw text or html - basically select between various $(self).html(.. and $(self).text(. this could include reverting self.revert to html (ie, issue 110) according to the option. at least the behaviour would then be consistent. 2- revive this issue: https://github.com/NicolasCARPi/jquery_jeditable/issues/182 and add an onresetcallback or similar function so cancel behaviour can be modified after the reset function. 3- expose the 'self.editing' property so if i return false from onreset{... then i can rewrite the reset function without further editing being disabled for the element (as when return false then 'self.editing=true')

  1. allow returning null from onreset{... to re-enable editing: ie replace L457: if (false !== onreset.apply(form, [settings, self])) { with if (null=== onreset.apply(form, [settings, self])) { self.editing = false; }esle if (false !== onreset.apply(form, [settings, self])) { 4a - better still just move L459 self.editing = false; to outside of that conditional statement (after L467) - should it not always be returned to false if the edit had been cancelled?

I think '4A' would be easiest, '1' would be best, '3' would be more versatile, '2' would be useful. thanks for considering this.

and sorry... i didn't see the version number as I just copied the raw js - 2.0.11

NicolasCARPi commented 5 years ago

@mcbmcb0 Please try the version from the experimental branch and let me know if it fixes your issue.

mcbmcb0 commented 5 years ago

Yes! that works for me. with 2.0.12 if I edit, cancel, then re-edit then the value stays the same. hence.. any tags / html entities remain!

BTW I do think that L459 self.editing = false; is in the wrong place. currently if the user returns false from onerror then self.editing remains false, so that field can not be edited again. this reset should be outside of the conditional statement, ie move to after L467. A bug? Should I raise another issue or are you happy with that here?

thanks!

NicolasCARPi commented 5 years ago

Please open another issue yes.