diff --git a/cms/envs/bok_choy.py b/cms/envs/bok_choy.py index 0f0cdb85cb..a49d673fb7 100644 --- a/cms/envs/bok_choy.py +++ b/cms/envs/bok_choy.py @@ -65,6 +65,9 @@ FEATURES['MILESTONES_APP'] = True # Enable pre-requisite course FEATURES['ENABLE_PREREQUISITE_COURSES'] = True +# Enable student notes +FEATURES['ENABLE_EDXNOTES'] = True + ########################### Entrance Exams ################################# FEATURES['ENTRANCE_EXAMS'] = True diff --git a/common/test/acceptance/pages/lms/edxnotes.py b/common/test/acceptance/pages/lms/edxnotes.py index e4c6cadaff..5b52da6adc 100644 --- a/common/test/acceptance/pages/lms/edxnotes.py +++ b/common/test/acceptance/pages/lms/edxnotes.py @@ -1,5 +1,5 @@ from bok_choy.page_object import PageObject, PageLoadError, unguarded -from bok_choy.promise import BrokenPromise +from bok_choy.promise import BrokenPromise, EmptyPromise from .course_page import CoursePage from ...tests.helpers import disable_animations from selenium.webdriver.common.action_chains import ActionChains @@ -523,7 +523,7 @@ class EdxNoteHighlight(NoteChild): Returns text of the note. """ self.show() - element = self.q(css=self._bounded_selector(".annotator-annotation > div")) + element = self.q(css=self._bounded_selector(".annotator-annotation > div.annotator-note")) if element: text = element.text[0].strip() else: @@ -538,3 +538,47 @@ class EdxNoteHighlight(NoteChild): Sets text for the note. """ self.q(css=self._bounded_selector(".annotator-item textarea")).first.fill(value) + + @property + def tags(self): + """ + Returns the tags associated with the note. + + Tags are returned as a list of strings, with each tag as an individual string. + """ + tag_text = [] + self.show() + tags = self.q(css=self._bounded_selector(".annotator-annotation > div.annotator-tags > span.annotator-tag")) + if tags: + for tag in tags: + tag_text.append(tag.text) + self.q(css="body").first.click() + self.wait_for_notes_invisibility() + return tag_text + + @tags.setter + def tags(self, tags): + """ + Sets tags for the note. Tags should be supplied as a list of strings, with each tag as an individual string. + """ + self.q(css=self._bounded_selector(".annotator-item input")).first.fill(" ".join(tags)) + + def has_sr_label(self, sr_index, field_index, expected_text): + """ + Returns true iff a screen reader label (of index sr_index) exists for the annotator field with + the specified field_index and text. + """ + label_exists = False + EmptyPromise( + lambda: len(self.q(css=self._bounded_selector("li.annotator-item > label.sr"))) > sr_index, + "Expected more than '{}' sr labels".format(sr_index) + ).fulfill() + annotator_field_label = self.q(css=self._bounded_selector("li.annotator-item > label.sr"))[sr_index] + for_attrib_correct = annotator_field_label.get_attribute("for") == "annotator-field-" + str(field_index) + if for_attrib_correct and (annotator_field_label.text == expected_text): + label_exists = True + + self.q(css="body").first.click() + self.wait_for_notes_invisibility() + + return label_exists diff --git a/common/test/acceptance/tests/lms/test_lms_edxnotes.py b/common/test/acceptance/tests/lms/test_lms_edxnotes.py index a366c7763d..715cbdbac4 100644 --- a/common/test/acceptance/tests/lms/test_lms_edxnotes.py +++ b/common/test/acceptance/tests/lms/test_lms_edxnotes.py @@ -1,7 +1,6 @@ import os from uuid import uuid4 from datetime import datetime -from unittest import skipUnless from ..helpers import UniqueCourseTest from ...fixtures.course import CourseFixture, XBlockFixtureDesc from ...pages.lms.auto_auth import AutoAuthPage @@ -11,7 +10,6 @@ from ...pages.lms.edxnotes import EdxNotesUnitPage, EdxNotesPage from ...fixtures.edxnotes import EdxNotesFixture, Note, Range -@skipUnless(os.environ.get("FEATURE_EDXNOTES"), "Requires Student Notes feature to be enabled") class EdxNotesTestMixin(UniqueCourseTest): """ Creates a course with initial data and contains useful helper methods. @@ -140,6 +138,16 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin): note.text = "TEST TEXT {}".format(index) index += 1 + def edit_tags_in_notes(self, components, tags): + self.assertGreater(len(components), 0) + index = 0 + for component in components: + self.assertGreater(len(component.notes), 0) + for note in component.edit_note(): + note.tags = tags[index] + index += 1 + self.assertEqual(index, len(tags), "Number of supplied tags did not match components") + def remove_notes(self, components): self.assertGreater(len(components), 0) for component in components: @@ -155,6 +163,11 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin): expected = ["TEST TEXT {}".format(i) for i in xrange(len(notes))] self.assertEqual(expected, actual) + def assert_tags_in_notes(self, notes, expected_tags): + actual = [note.tags for note in notes] + expected = [expected_tags[i] for i in xrange(len(notes))] + self.assertEqual(expected, actual) + def test_can_create_notes(self): """ Scenario: User can create notes. @@ -255,6 +268,69 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin): components = self.note_unit_page.components self.assert_notes_are_removed(components) + def test_can_create_note_with_tags(self): + """ + Scenario: a user of notes can define one with tags + Given I have a course with 3 annotatable components + And I open the unit with 2 annotatable components + When I add a note with tags for the first component + And I refresh the page + Then I see that note was correctly stored with its tags + """ + self.note_unit_page.visit() + + components = self.note_unit_page.components + for note in components[0].create_note(".{}".format(self.selector)): + note.tags = ["fruit", "tasty"] + + self.note_unit_page.refresh() + self.assertEqual(["fruit", "tasty"], self.note_unit_page.notes[0].tags) + + def test_can_change_tags(self): + """ + Scenario: a user of notes can edit tags on notes + Given I have a course with 3 components with notes + When I open the unit with 2 annotatable components + And I edit tags on the notes for the 2 annotatable components + Then I see that the tags were correctly changed + And I again edit tags on the notes for the 2 annotatable components + And I refresh the page + Then I see that the tags were correctly changed + """ + self._add_notes() + self.note_unit_page.visit() + + components = self.note_unit_page.components + self.edit_tags_in_notes(components, [["hard"], ["apple", "pear"]]) + self.assert_tags_in_notes(self.note_unit_page.notes, [["hard"], ["apple", "pear"]]) + + self.edit_tags_in_notes(components, [[], ["avocado"]]) + self.assert_tags_in_notes(self.note_unit_page.notes, [[], ["avocado"]]) + + self.note_unit_page.refresh() + self.assert_tags_in_notes(self.note_unit_page.notes, [[], ["avocado"]]) + + def test_sr_labels(self): + """ + Scenario: screen reader labels exist for text and tags fields + Given I have a course with 3 components with notes + When I open the unit with 2 annotatable components + And I open the editor for each note + Then the text and tags fields both have screen reader labels + """ + self._add_notes() + self.note_unit_page.visit() + + # First note is in the first annotatable component, will have field indexes 0 and 1. + for note in self.note_unit_page.components[0].edit_note(): + self.assertTrue(note.has_sr_label(0, 0, "Note")) + self.assertTrue(note.has_sr_label(1, 1, "Tags (space-separated)")) + + # Second note is in the second annotatable component, will have field indexes 2 and 3. + for note in self.note_unit_page.components[1].edit_note(): + self.assertTrue(note.has_sr_label(0, 2, "Note")) + self.assertTrue(note.has_sr_label(1, 3, "Tags (space-separated)")) + class EdxNotesPageTest(EdxNotesTestMixin): """ diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index 0070b1513f..b44824e765 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -94,6 +94,9 @@ FEATURES['MILESTONES_APP'] = True # Enable pre-requisite course FEATURES['ENABLE_PREREQUISITE_COURSES'] = True +# Enable student notes +FEATURES['ENABLE_EDXNOTES'] = True + # Unfortunately, we need to use debug mode to serve staticfiles DEBUG = True diff --git a/lms/static/js/edxnotes/plugins/accessibility.js b/lms/static/js/edxnotes/plugins/accessibility.js index 4044160182..79972261d8 100644 --- a/lms/static/js/edxnotes/plugins/accessibility.js +++ b/lms/static/js/edxnotes/plugins/accessibility.js @@ -131,19 +131,27 @@ define(['jquery', 'underscore', 'annotator_1.2.9'], function ($, _, Annotator) { }, getEditorTabControls: function () { - var editor, editorControls, textArea, saveButton, cancelButton, tabControls = []; + var editor, editorControls, textArea, saveButton, cancelButton, tabControls = [], annotatorItems, + tagInput = null; // Editor elements editor = this.annotator.element.find('.annotator-editor'); editorControls = editor.find('.annotator-controls'); - textArea = editor.find('.annotator-listing') - .find('.annotator-item') - .first() - .children('textarea'); + annotatorItems = editor.find('.annotator-listing').find('.annotator-item'); + textArea = annotatorItems.first().children('textarea'); saveButton = editorControls.find('.annotator-save'); cancelButton = editorControls.find('.annotator-cancel'); - tabControls.push(textArea, saveButton, cancelButton); + // If the tags plugin is enabled, add the ability to tab into it. + if (annotatorItems.length > 1) { + tagInput = annotatorItems.first().next().children('input'); + } + + tabControls.push(textArea); + if (tagInput){ + tabControls.push(tagInput); + } + tabControls.push(saveButton, cancelButton); return tabControls; }, diff --git a/lms/static/js/edxnotes/views/notes_factory.js b/lms/static/js/edxnotes/views/notes_factory.js index 8180543c78..4bb521c0bc 100644 --- a/lms/static/js/edxnotes/views/notes_factory.js +++ b/lms/static/js/edxnotes/views/notes_factory.js @@ -6,7 +6,7 @@ define([ 'js/edxnotes/plugins/events', 'js/edxnotes/plugins/accessibility', 'js/edxnotes/plugins/caret_navigation' ], function ($, _, Annotator, NotesLogger) { - var plugins = ['Auth', 'Store', 'Scroller', 'Events', 'Accessibility', 'CaretNavigation'], + var plugins = ['Auth', 'Store', 'Scroller', 'Events', 'Accessibility', 'CaretNavigation', 'Tags'], getOptions, setupPlugins, updateHeaders, getAnnotator; /** diff --git a/lms/static/js/edxnotes/views/shim.js b/lms/static/js/edxnotes/views/shim.js index 8fcdcafdc0..7595cef755 100644 --- a/lms/static/js/edxnotes/views/shim.js +++ b/lms/static/js/edxnotes/views/shim.js @@ -58,6 +58,34 @@ define([ return (timeToExpiry > 0) ? timeToExpiry : 0; }; + + Annotator.Plugin.Tags.prototype.updateField = _.compose( + function() { + // Add screen reader label for edit mode. Note that the id of the tags element will not always be "1". + // It depends on the number of annotatable components on the page. + var tagsField = $("li.annotator-item >input", this.annotator.editor.element).attr('id'); + if ($("label.sr[for='"+ tagsField + "']", this.annotator.editor.element).length === 0) { + $('').insertBefore( + $('#'+tagsField, this.annotator.editor.element) + ); + } + return this; + }, + Annotator.Plugin.Tags.prototype.updateField + ); + + Annotator.Plugin.Tags.prototype.updateViewer = _.compose( + function() { + // Add ARIA information for viewing mode. + $('div.annotator-tags', this.wrapper).attr({ + 'role': 'region', + 'aria-label': 'tags' + }); + return this; + }, + Annotator.Plugin.Tags.prototype.updateViewer + ); + /** * Modifies Annotator.highlightRange to add "tabindex=0" and role="link" * attributes to the markup that encloses the @@ -186,25 +214,24 @@ define([ '' ].join(''); - /** - * Modifies Annotator._setupEditor to add a label for textarea#annotator-field-0. - **/ - Annotator.prototype._setupEditor = _.compose( - function () { - $('').insertBefore( - $('#annotator-field-0', this.wrapper) - ); - return this; - }, - Annotator.prototype._setupEditor - ); /** * Modifies Annotator.Editor.show, in the case of a keydown event, to remove * focus from Save button and put it on form.annotator-widget instead. + * + * Also add a sr label for note textarea. **/ Annotator.Editor.prototype.show = _.compose( function (event) { + // Add screen reader label for the note area. Note that the id of the tags element will not always be "0". + // It depends on the number of annotatable components on the page. + var noteField = $("li.annotator-item >textarea", this.element).attr('id'); + if ($("label.sr[for='"+ noteField + "']", this.element).length === 0) { + $('').insertBefore( + $('#'+noteField, this.element) + ); + } + if (event.type === 'keydown') { this.element.find('.annotator-save').removeClass(this.classes.focus); this.element.find('form.annotator-widget').focus(); diff --git a/lms/static/js/spec/edxnotes/plugins/accessibility_spec.js b/lms/static/js/spec/edxnotes/plugins/accessibility_spec.js index 9174fbdb0b..b135be8163 100644 --- a/lms/static/js/spec/edxnotes/plugins/accessibility_spec.js +++ b/lms/static/js/spec/edxnotes/plugins/accessibility_spec.js @@ -168,12 +168,12 @@ define([ }; highlight.data('annotation', annotation); this.annotator.viewer.load([annotation]); - listing = this.annotator.element.find('.annotator-listing').first(), + listing = this.annotator.element.find('.annotator-listing').first(); note = this.annotator.element.find('.annotator-note').first(); edit= this.annotator.element.find('.annotator-edit').first(); del = this.annotator.element.find('.annotator-delete').first(); close = this.annotator.element.find('.annotator-close').first(); - spyOn(this.annotator.viewer, 'hide').andCallThrough();; + spyOn(this.annotator.viewer, 'hide').andCallThrough(); }); it('should give focus to Note on Listing TAB keydown', function () { @@ -224,7 +224,7 @@ define([ }); describe('keydown events on editor', function () { - var highlight, annotation, form, textArea, save, cancel; + var highlight, annotation, form, annotatorItems, textArea, tags, save, cancel; beforeEach(function() { highlight = $('').appendTo(this.annotator.element); @@ -236,7 +236,9 @@ define([ highlight.data('annotation', annotation); this.annotator.editor.show(annotation, {'left': 0, 'top': 0}); form = this.annotator.element.find('form.annotator-widget'); - textArea = this.annotator.element.find('.annotator-item').first().children('textarea'); + annotatorItems = this.annotator.element.find('.annotator-item'); + textArea = annotatorItems.first().children('textarea'); + tags = annotatorItems.first().next().children('input'); save = this.annotator.element.find('.annotator-save'); cancel = this.annotator.element.find('.annotator-cancel'); spyOn(this.annotator.editor, 'submit').andCallThrough(); @@ -255,9 +257,11 @@ define([ expect(cancel).toBeFocused(); }); - it('should cycle forward through texarea, save, and cancel on TAB keydown', function () { + it('should cycle forward through textarea, tags, save, and cancel on TAB keydown', function () { textArea.focus(); textArea.trigger(tabForwardEvent()); + expect(tags).toBeFocused(); + tags.trigger(tabForwardEvent()); expect(save).toBeFocused(); save.trigger(tabForwardEvent()); expect(cancel).toBeFocused(); @@ -265,13 +269,15 @@ define([ expect(textArea).toBeFocused(); }); - it('should cycle back through texarea, save, and cancel on SHIFT + TAB keydown', function () { + it('should cycle back through textarea, tags, save, and cancel on SHIFT + TAB keydown', function () { textArea.focus(); textArea.trigger(tabBackwardEvent()); expect(cancel).toBeFocused(); cancel.trigger(tabBackwardEvent()); expect(save).toBeFocused(); save.trigger(tabBackwardEvent()); + expect(tags).toBeFocused(); + tags.trigger(tabBackwardEvent()); expect(textArea).toBeFocused(); }); diff --git a/lms/static/js/spec/edxnotes/views/shim_spec.js b/lms/static/js/spec/edxnotes/views/shim_spec.js index c4197496c4..dfd69c6f06 100644 --- a/lms/static/js/spec/edxnotes/views/shim_spec.js +++ b/lms/static/js/spec/edxnotes/views/shim_spec.js @@ -233,5 +233,37 @@ define([ expect(mockViewer.element.appendTo).toHaveBeenCalledWith(annotators[0].wrapper); }); }); + + describe('TagsPlugin', function () { + it('should add ARIA label information to the viewer', function() { + var tagDiv, + annotation = { + id: '01', + text: "Test text", + tags: ["tag1", "tag2", "tag3"], + highlights: [highlights[0].get(0)] + }; + + annotators[0].viewer.load([annotation]); + tagDiv = annotators[0].viewer.element.find('.annotator-tags'); + expect($(tagDiv).attr('role')).toEqual('region'); + expect($(tagDiv).attr('aria-label')).toEqual('tags'); + + // Three children for the individual tags. + expect($(tagDiv).children().length).toEqual(3); + }); + + it('should add screen reader label to the editor', function() { + var srLabel, editor, inputId; + + // We don't know exactly what the input ID will be (depends on number of annotatable components shown), + // but the sr label "for" attribute should match the ID of the element immediately following it. + annotators[0].showEditor({}, {}); + editor = annotators[0].editor; + srLabel = editor.element.find("label.sr"); + inputId = srLabel.next().attr('id'); + expect(srLabel.attr('for')).toEqual(inputId); + }); + }); }); }); diff --git a/lms/static/sass/course/_student-notes.scss b/lms/static/sass/course/_student-notes.scss index 2ec0a96369..ef7f2a2707 100644 --- a/lms/static/sass/course/_student-notes.scss +++ b/lms/static/sass/course/_student-notes.scss @@ -212,7 +212,7 @@ $divider-visual-tertiary: ($baseline/20) solid $gray-l4; .reference-title { @extend %t-title8; @extend %t-weight3; - margin-top: $baseline; + margin-top: ($baseline/2); text-transform: uppercase; letter-spacing: ($baseline/20); color: $gray-l2;