angular-ui / ui-codemirror

This directive allows you to add CodeMirror to your textarea elements.
http://angular-ui.github.io/ui-codemirror
MIT License
378 stars 193 forks source link

Adding characters to codemirror textarea set ng-model to undefined #51

Closed stevedomin closed 7 years ago

stevedomin commented 10 years ago

Adding characters (space, letters, anything...) to a codemirror textarea seems to set the ng-model to undefined (after the textarea loses focus?).

On the other hand, removing character or adding lines seems to work completely fine.

This issue is best described by example so here's a plnkr reproducing it: http://plnkr.co/edit/AETLQPq24prFQZxisnL9?p=preview

Steps to reproduce:

  1. Add characters to the console.log() line
  2. The changes should be reflected in output below the codemirror textarea
  3. Click on the button: the text in the output disappear (and if you check the Chrome console you can see cmModel is now undefined)
  4. Add more characters, click on the button, nothing changes
  5. Remove some characters (or add a line), click on the button, the text appears again in the output.
  6. Add characters: same behaviour as before, cmModel becomes undefined.

I'm out of options on this one, I've no idea what's causing this.

whitehat101 commented 10 years ago

I'm going to look at this a little more, but the easy workaround is to use a div instead of a textarea.

<div ui-codemirror="cmOption" ng-model="cmModel"></div>
whitehat101 commented 10 years ago

I know why it's happening, but I don't know a good way to fix this.

https://github.com/angular-ui/ui-codemirror/blob/master/src/ui-codemirror.js#L41

tElement.replaceWith(cm_el);

This is not a great solution, but I was able to fix the behavior by replacing line 41 with the following:

if(tElement[0].nodeName === 'TEXTAREA') {
  tElement.css({display:'none'}).after(cm_el);
} else {
  tElement.replaceWith(cm_el);
}

I'd guess that ngModel is binding to the textarea before ui-codemirror gets it, and when the textarea is removed that ngModel only gets undefined when trying to read the value of the removed textarea. The double-update is from the textarea's ngModel and ui-codemirror's ngModel.

stevedomin commented 10 years ago

@whitehat101 Awesome! Thanks a lot for having a look at this.

Why do you think your proposed solution is not the good way to fix this?

whitehat101 commented 10 years ago

Making the textarea invisible (display:none) seemed like a workaround, seems like leaving trash in the dom.

I played with the directive's priority, and couldn't get results. There are probably a half-dozen open issues about the use of <textarea>s (#49 #43 #35). I tried a search and replace in the spec file (div => textarea) and all the tests still passed—so we're not even testing the strange behavior on divs, yet.

This project used to require textareas, and used fromTextArea to init CM. Maybe, we should check if we have a textarea and use the "old" constructor, else use the "current" constructor.

If fromTextArea has other issues, making the textarea invisible might be a reasonable way to better support textareas.

napcs commented 10 years ago

Was just bitten by this myself. I patched in @whitehat101's solution and it solved my problem.

whitehat101 commented 10 years ago

@douglasduteil is supporting ui-codemirror on a textarea something that this project would like to support?

There are a lot of open issues that could probably be closed by either updating the docs to say just use a div, using a workaround like I suggested on April 28th, or reviving the old constructor for textareas like I suggested on April 29th.

In what direction should this project be heading?

SawyerHood commented 10 years ago

Ahhhhhh I wish I would've seen this a few hours ago. I switched from using the textarea element with the directive as an attribute and instead started using ui-codemirror as a standalone element and everything works as intended.

douglasduteil commented 10 years ago

Hi @whitehat101.

I'm sorry for not being there the whole time... I'll be fixing the ng repeat problem with basic postlinking. I think it's will be fixing most today issues this way :+1: (see #59 )

For the future I don't have direction line. Sadly I don't have projects where I'm pushing this directive to the edge so I'm listening to what the community needs.

stevedomin commented 7 years ago

Feel free to reopen if someone else come with the issue!