Fixes #204 and #153.#248
Conversation
Makes sure this.range is defined when calling selection.getContaining and selection.placeMarkers. Makes sure the current selection is inside the scribe element before placing markers.
|
This is a duplicate of #231. |
|
Yes, the difference is it fixes the issue with less changes to the source and also fixes #153 |
|
Okay I'm happy to merge this - but in regards to the issues in #231, I think @OliverJAsh might have some opinions about it. Although checking for a range makes sense to me as does checking that we're actually in the scribe.el. The selection does get set if focus is outside the scribe.el - just try selecting the whole page and it happens. |
There was a problem hiding this comment.
This makes it safer, although I'd also be curious to know what code calls placeMarkers at such a time that this expression is true (i.e. the selection is outside of the Scribe DOM).
There was a problem hiding this comment.
Here is a reproduction. Just click the Set Content button 10 times and right click + inspect.
http://jsfiddle.net/npqf0Lgu/1/
Since in our use case we need to programmatically update the contents of the scribe, this is what happens (em markers are left around all over the document).
Additional explanation is at end of #153 here: #153 (comment)
There was a problem hiding this comment.
Okay - that looks like a good case. I'm happy with this branch if @theefer is. See no reason not to merge this in.
|
👍 |
5 participants
Makes sure
this.rangeis defined when callingselection.getContainingandselection.placeMarkers. Also makes sure the current selection is inside the scribe element before placing markers. Fixes #204 and #153.