TNL-1116: Fix clicking bug.

common/test/acceptance/tests/lms/test_lms_edxnotes.py

@@ -699,9 +699,7 @@ class EdxNotesToggleSingleNoteTest(EdxNotesTestMixin):
         And I move mouse out of the note
         Then I see that the note is still shown
         When I click on highlighted text in the second component
-        Then I do not see any notes
-        When I click again on highlighted text in the second component
-        Then I see appropriate note
+        Then I see that the new note is shown
         """
         note_1 = self.note_unit_page.notes[0]
         note_2 = self.note_unit_page.notes[1]
@@ -712,9 +710,6 @@ class EdxNotesToggleSingleNoteTest(EdxNotesTestMixin):
 
         note_2.click_on_highlight()
         self.assertFalse(note_1.is_visible)
-        self.assertFalse(note_2.is_visible)
-
-        note_2.click_on_highlight()
         self.assertTrue(note_2.is_visible)
 
 

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

@@ -15,7 +15,7 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
     if (!$.fn.addBack) {
         $.fn.addBack = function (selector) {
             return this.add(
-              selector === null ? this.prevObject : this.prevObject.filter(selector)
+                selector === null ? this.prevObject : this.prevObject.filter(selector)
             );
         };
     }
@@ -36,11 +36,11 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
      **/
     Annotator.Plugin.Auth.prototype.haveValidToken = function() {
         return (
-          this._unsafeToken &&
-          this._unsafeToken.sub &&
-          this._unsafeToken.exp &&
-          this._unsafeToken.iat &&
-          this.timeToExpiry() > 0
+            this._unsafeToken &&
+            this._unsafeToken.sub &&
+            this._unsafeToken.exp &&
+            this._unsafeToken.iat &&
+            this.timeToExpiry() > 0
         );
     };
 
@@ -91,6 +91,7 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
             }
             // Unbind onNoteClick from click
             this.viewer.element.off('click', this.onNoteClick);
+            this.wrapper.off('click', '.annotator-hl');
         }
     );
 
@@ -115,19 +116,6 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
     ].join('');
 
     /**
-     * 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
-    );
-
-    Annotator.Editor.prototype.isShown = Annotator.Viewer.prototype.isShown;
-
-    /**
      * Modifies Annotator.onHighlightMouseover to avoid showing the viewer if the
      * editor is opened.
      **/
@@ -143,24 +131,42 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
         Annotator.prototype._setupViewer
     );
 
-    $.extend(true, Annotator.prototype, {
-        events: {
-            '.annotator-hl click': 'onHighlightClick',
-            '.annotator-viewer click': 'onNoteClick'
+    /**
+     * 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'.
+     **/
+    Annotator.prototype._setupWrapper = _.compose(
+        function () {
+            this.element.on('click', '.annotator-hl', _.bind(this.onHighlightClick, this));
+            return this;
+        },
+        Annotator.prototype._setupWrapper
+    );
+
+    Annotator.Editor.prototype.isShown = Annotator.Viewer.prototype.isShown;
+
+    $.extend(true, Annotator.prototype, {
         isFrozen: false,
         uid: _.uniqueId(),
 
         onHighlightClick: function (event) {
-            Annotator.Util.preventEventDefault(event);
-
-            if (!this.isFrozen) {
-                event.stopPropagation();
+            event.stopPropagation();
+            if (!this.editor.isShown()) {
+                this.unfreezeAll();
                 this.onHighlightMouseover.call(this, event);
+                Annotator.frozenSrc = this;
+                this.freezeAll();
             }
-            Annotator.frozenSrc = this;
-            this.freezeAll();
         },
 
         onNoteClick: function (event) {
@@ -181,6 +187,7 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
                 $(document).on('click.edxnotes:freeze' + this.uid, _.bind(this.unfreeze, this));
                 this.isFrozen = true;
             }
+            return this;
         },
 
         unfreeze: function () {
@@ -192,23 +199,27 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
                     'mouseout':  this.startViewerHideTimer
                 });
                 this.viewer.hide();
-                $(document).off('click.edxnotes:freeze'+this.uid);
+                $(document).off('click.edxnotes:freeze' + this.uid);
                 this.isFrozen = false;
                 Annotator.frozenSrc = null;
             }
+            return this;
         },
 
         freezeAll: function () {
             _.invoke(Annotator._instances, 'freeze');
+            return this;
         },
 
         unfreezeAll: function () {
             _.invoke(Annotator._instances, 'unfreeze');
+            return this;
         },
 
         showFrozenViewer: function (annotations, location) {
             this.showViewer(annotations, location);
             this.freezeAll();
+            return this;
         }
     });
 });

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

@@ -29,15 +29,15 @@ define([
             loadFixtures('js/fixtures/edxnotes/edxnotes_wrapper.html');
             highlights = [];
             annotators = [
-                NotesFactory.factory($('div#edx-notes-wrapper-123').get(0), {
+                NotesFactory.factory($('#edx-notes-wrapper-123').get(0), {
                     endpoint: 'http://example.com/'
                 }),
-                NotesFactory.factory($('div#edx-notes-wrapper-456').get(0), {
+                NotesFactory.factory($('#edx-notes-wrapper-456').get(0), {
                     endpoint: 'http://example.com/'
                 })
             ];
-            _.each(annotators, function(annotator, index) {
-                highlights.push($('<span class="annotator-hl" />').appendTo(annotators[index].element));
+            _.each(annotators, function(annotator) {
+                highlights.push($('<span class="annotator-hl" />').appendTo(annotator.element));
                 spyOn(annotator, 'onHighlightClick').andCallThrough();
                 spyOn(annotator, 'onHighlightMouseover').andCallThrough();
                 spyOn(annotator, 'startViewerHideTimer').andCallThrough();
@@ -54,6 +54,14 @@ define([
             highlights[0].mouseover();
             expect($('#edx-notes-wrapper-123 .annotator-editor')).not.toHaveClass('annotator-hide');
             expect($('#edx-notes-wrapper-123 .annotator-viewer')).toHaveClass('annotator-hide');
+
+        it('clicking on highlights does not open the viewer when the editor is opened', function() {
+            spyOn(annotators[1].editor, 'isShown').andReturn(false);
+            highlights[0].click();
+            annotators[1].editor.isShown.andReturn(true);
+            highlights[1].click();
+            expect($('#edx-notes-wrapper-123 .annotator-viewer')).not.toHaveClass('annotator-hide');
+            expect($('#edx-notes-wrapper-456 .annotator-viewer')).toHaveClass('annotator-hide');
         });
 
         it('clicking a highlight freezes mouseover and mouseout in all highlighted text', function() {
@@ -65,10 +73,7 @@ define([
             // in turn calls onHighlightMouseover.
             // To test if onHighlightMouseover is called or not on
             // mouseover, we'll have to reset onHighlightMouseover.
-            expect(annotators[0].onHighlightClick).toHaveBeenCalled();
-            expect(annotators[0].onHighlightMouseover).toHaveBeenCalled();
             annotators[0].onHighlightMouseover.reset();
-
             // Check that both instances of annotator are frozen
             _.invoke(highlights, 'mouseover');
             _.invoke(highlights, 'mouseout');
@@ -121,11 +126,13 @@ define([
             checkClickEventsNotBound('edxnotes:freeze' + annotators[1].uid);
         });
 
-        it('should unbind onNotesLoaded on destruction', function() {
+        it('should unbind events on destruction', function() {
             annotators[0].destroy();
             expect($.fn.off).toHaveBeenCalledWith(
-                'click',
-                annotators[0].onNoteClick
+                'click', annotators[0].onNoteClick
+            );
+            expect($.fn.off).toHaveBeenCalledWith(
+                'click', '.annotator-hl'
             );
         });
     });