Merge pull request #6692 from edx/anton/edxnotes-fix-text-line-breaks

TNL-1078: Lines don't break correctly in Student Notes.

lms/static/js/edxnotes/models/note.js

@@ -1,6 +1,6 @@
 ;(function (define) {
 'use strict';
-define(['backbone', 'underscore.string'], function (Backbone) {
+define(['backbone', 'js/edxnotes/utils/utils', 'underscore.string'], function (Backbone, Utils) {
     var NoteModel = Backbone.Model.extend({
         defaults: {
             'id': null,
@@ -42,7 +42,7 @@ define(['backbone', 'underscore.string'], function (Backbone) {
             }
         },
 
-        getNoteText: function () {
+        getQuote: function () {
             var message = this.get('quote');
 
             if (!this.get('is_expanded') && this.get('show_link')) {
@@ -50,6 +50,10 @@ define(['backbone', 'underscore.string'], function (Backbone) {
             }
 
             return message;
+        },
+
+        getText: function () {
+            return Utils.nl2br(this.get('text'));
         }
 
     });

lms/static/js/edxnotes/utils/utils.js

@@ -0,0 +1,24 @@
+;(function (define, undefined) {
+'use strict';
+define([], function($, _) {
+    /**
+     * Replaces all newlines in a string by HTML line breaks.
+     * @param {String} str The input string.
+     * @return `String` with '<br>' instead all newlines (\r\n, \n\r, \n and \r).
+     * @example
+     *   nl2br("This\r\nis\n\ra\nstring\r")
+     *   Output:
+     *   This<br>
+     *   is<br>
+     *   a<br>
+     *   string<br>
+     */
+    var nl2br = function (str) {
+        return (str + '').replace(/(\r\n|\n\r|\r|\n)/g, '<br>');
+    }
+
+    return {
+        nl2br: nl2br
+    };
+});
+}).call(this, define || RequireJS.define);

lms/static/js/edxnotes/views/note_item.js

@@ -29,9 +29,10 @@ define([
         },
 
         getContext: function () {
-            return $.extend({
+            return $.extend({}, this.model.toJSON(), {
-                message: this.model.getNoteText()
+                message: this.model.getQuote(),
-            }, this.model.toJSON());
+                text: this.model.getText()
+            });
         },
 
         toggleNote: function () {

lms/static/js/edxnotes/views/shim.js

@@ -1,6 +1,8 @@
 ;(function (define, undefined) {
 'use strict';
-define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
+define([
+    'jquery', 'underscore', 'annotator', 'js/edxnotes/utils/utils'
+], function ($, _, Annotator, Utils) {
     var _t = Annotator._t;
 
     /**
@@ -115,6 +117,36 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
         '</li>'
     ].join('');
 
+    /**
+     * Overrides Annotator._setupViewer to add a "click" event on viewer and to
+     * improve line breaks.
+     **/
+    Annotator.prototype._setupViewer = function () {
+        var self = this;
+        this.viewer = new Annotator.Viewer({readOnly: this.options.readOnly});
+        this.viewer.element.on('click', _.bind(this.onNoteClick, this));
+        this.viewer.hide()
+            .on("edit", this.onEditAnnotation)
+            .on("delete", this.onDeleteAnnotation)
+            .addField({
+                load: function (field, annotation) {
+                    if (annotation.text) {
+                        $(field).html(Utils.nl2br(Annotator.Util.escape(annotation.text)));
+                    } else {
+                        $(field).html('<i>' + _t('No Comment') + '</i>');
+                        self.publish('annotationViewerTextField', [field, annotation]);
+                    }
+                }
+            })
+            .element.appendTo(this.wrapper).bind({
+                "mouseover": this.clearViewerHideTimer,
+                "mouseout":  this.startViewerHideTimer
+            });
+        return this;
+    };
+
+    Annotator.Editor.prototype.isShown = Annotator.Viewer.prototype.isShown;
+
     /**
      * Modifies Annotator.onHighlightMouseover to avoid showing the viewer if the
      * editor is opened.
@@ -131,17 +163,6 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
         Annotator.prototype._setupViewer
     );
 
-    /**
-     * Modifies Annotator._setupViewer to add a "click" event on viewer.
-     **/
-    Annotator.prototype._setupViewer = _.compose(
-        function () {
-            this.viewer.element.on('click', _.bind(this.onNoteClick, this));
-            return this;
-        },
-        Annotator.prototype._setupViewer
-    );
-
     /**
      * Modifies Annotator._setupWrapper to add a "click" event on '.annotator-hl'.
      **/

lms/static/js/spec/edxnotes/models/note_spec.js

@@ -5,8 +5,8 @@ define([
     describe('EdxNotes NoteModel', function() {
         beforeEach(function () {
             this.collection = new NotesCollection([
-                {quote: Helpers.LONG_TEXT},
+                {quote: Helpers.LONG_TEXT, text: 'text\n with\r\nline\n\rbreaks \r'},
-                {quote: Helpers.SHORT_TEXT}
+                {quote: Helpers.SHORT_TEXT, text: 'text\n with\r\nline\n\rbreaks \r'}
             ]);
         });
 
@@ -17,18 +17,23 @@ define([
             expect(this.collection.at(1).get('show_link')).toBeFalsy();
         });
 
-        it('can return appropriate note text', function () {
+        it('can return appropriate `quote`', function () {
             var model = this.collection.at(0);
 
             // is_expanded = false, show_link = true
-            expect(model.getNoteText()).toBe(Helpers.PRUNED_TEXT);
+            expect(model.getQuote()).toBe(Helpers.PRUNED_TEXT);
             model.set('is_expanded', true);
             // is_expanded = true, show_link = true
-            expect(model.getNoteText()).toBe(Helpers.LONG_TEXT);
+            expect(model.getQuote()).toBe(Helpers.LONG_TEXT);
             model.set('show_link', false);
             model.set('is_expanded', false);
             // is_expanded = false, show_link = false
-            expect(model.getNoteText()).toBe(Helpers.LONG_TEXT);
+            expect(model.getQuote()).toBe(Helpers.LONG_TEXT);
+        });
+
+        it('can return appropriate `text`', function () {
+            var model = this.collection.at(0);
+            expect(model.getText()).toBe('text<br> with<br>line<br>breaks <br>');
         });
     });
 });

lms/static/js/spec/edxnotes/views/shim_spec.js

@@ -135,5 +135,88 @@ define([
                 'click', '.annotator-hl'
             );
         });
+
+        describe('_setupViewer', function () {
+            var mockViewer = null;
+
+            beforeEach(function () {
+                var  element = $('<div />');
+                mockViewer = {
+                    fields: [],
+                    element: element
+                };
+
+                mockViewer.on = jasmine.createSpy().andReturn(mockViewer);
+                mockViewer.hide = jasmine.createSpy().andReturn(mockViewer);
+                mockViewer.destroy = jasmine.createSpy().andReturn(mockViewer);
+                mockViewer.addField = jasmine.createSpy().andCallFake(function (options) {
+                    mockViewer.fields.push(options);
+                    return mockViewer;
+                });
+
+                spyOn(element, 'bind').andReturn(element);
+                spyOn(element, 'appendTo').andReturn(element);
+                spyOn(Annotator, 'Viewer').andReturn(mockViewer);
+
+                annotators[0]._setupViewer();
+            });
+
+            it('should create a new instance of Annotator.Viewer and set Annotator#viewer', function () {
+                expect(annotators[0].viewer).toEqual(mockViewer);
+            });
+
+            it('should hide the annotator on creation', function () {
+                expect(mockViewer.hide.callCount).toBe(1);
+            });
+
+            it('should setup the default text field', function () {
+                var args = mockViewer.addField.mostRecentCall.args[0];
+
+                expect(mockViewer.addField.callCount).toBe(1);
+                expect(_.isFunction(args.load)).toBeTruthy();
+            });
+
+            it('should set the contents of the field on load', function () {
+                var field = document.createElement('div'),
+                    annotation = {text: 'text \nwith\r\nline\n\rbreaks \r'};
+
+                annotators[0].viewer.fields[0].load(field, annotation);
+                expect($(field).html()).toBe('text <br>with<br>line<br>breaks <br>');
+            });
+
+            it('should set the contents of the field to placeholder text when empty', function () {
+                var field = document.createElement('div'),
+                    annotation = {text: ''};
+
+                annotators[0].viewer.fields[0].load(field, annotation);
+                expect($(field).html()).toBe('<i>No Comment</i>');
+            });
+
+            it('should setup the default text field to publish an event on load', function () {
+                var field = document.createElement('div'),
+                    annotation = {text: ''},
+                    callback = jasmine.createSpy();
+
+                annotators[0].on('annotationViewerTextField', callback);
+                annotators[0].viewer.fields[0].load(field, annotation);
+                expect(callback).toHaveBeenCalledWith(field, annotation);
+            });
+
+            it('should subscribe to custom events', function () {
+                expect(mockViewer.on).toHaveBeenCalledWith('edit', annotators[0].onEditAnnotation);
+                expect(mockViewer.on).toHaveBeenCalledWith('delete', annotators[0].onDeleteAnnotation);
+            });
+
+            it('should bind to browser mouseover and mouseout events', function () {
+                expect(mockViewer.element.bind).toHaveBeenCalledWith({
+                    'mouseover': annotators[0].clearViewerHideTimer,
+                    'mouseout':  annotators[0].startViewerHideTimer
+                });
+            });
+
+            it('should append the Viewer#element to the Annotator#wrapper', function () {
+                expect(mockViewer.element.appendTo).toHaveBeenCalledWith(annotators[0].wrapper);
+            });
+        });
     });
 });