Bug 1605761 - It's not clear at all that you need to hit -enter- to save a comment tag

diff --git a/extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl b/extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl
index 715f58c580d41ecc8f8342eb05c152b46c842e53..6b379092b708c09b1182d3553dcfc34cf334169f 100644 (file)
--- a/extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl
+++ b/extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl
@@ -84,11 +84,12 @@
   <div id="ctag" style="display:none">
     <input id="ctag-add" size="10" placeholder="add tag"
       maxlength="[% constants.MAX_COMMENT_TAG_LENGTH FILTER html %]">
-    <button type="button" id="ctag-close" class="minor" aria-label="Close">x</button>
+    <button type="button" id="ctag-save" class="minor">Save</button>
+    <button type="button" id="ctag-cancel" class="minor">Cancel</button>
     <a href="https://wiki.mozilla.org/BMO/comment_tagging" target="_blank" rel="noopener noreferrer" title="About Comment Tagging">Help</a>
   </div>
   <div id="ctag-error" style="display:none">
-    <a href="#" class="close-btn" data-for="ctag-error" aria-label="Close">x</a>
+    <a href="#" class="close-btn" data-for="ctag-error" aria-label="Dismiss">×</a>
     <span id="ctag-error-message"></span>
   </div>
 [% END %]
@@ -174,7 +175,7 @@
         <td colspan="2" class="comment-tags">
           [% FOREACH tag IN comment.tags ~%]
             <span class="comment-tag">
-              [%~ '<a role="button" aria-label="Remove">x</a>' IF user.can_tag_comments %][% tag FILTER html ~%]
+              [%~ '<a role="button" aria-label="Remove" class="remove">×</a>' IF user.can_tag_comments %][% tag FILTER html ~%]
             </span>
           [%~ END %]
         </td>

diff --git a/extensions/BugModal/web/bug_modal.css b/extensions/BugModal/web/bug_modal.css
index 77dff1578ca95cf40334e0a0410ff434a5a6fdc2..2fadf4f0103ef43b3df4f9b89254b472fdc1cccc 100644 (file)
--- a/extensions/BugModal/web/bug_modal.css
+++ b/extensions/BugModal/web/bug_modal.css
@@ -823,6 +823,13 @@ h3.change-name a {
   cursor: pointer;
 }
 
+.comment-tags .remove[role="button"],
+.comment-tags .close-btn {
+  font-size: 1.8em;
+  line-height: .7em;
+  vertical-align: top;
+}
+
 #ctag {
   margin-bottom: 4px;
 }

diff --git a/extensions/BugModal/web/comments.js b/extensions/BugModal/web/comments.js
index 35f35e6e4cdafdd54838021b13e5bed858b4e183..f1123ce12ef793ea7c0c0f7bd31eab9d879968e4 100644 (file)
--- a/extensions/BugModal/web/comments.js
+++ b/extensions/BugModal/web/comments.js
@@ -305,7 +305,7 @@ $(function() {
         $.each(tags, function() {
             var span = $('<span/>').addClass('comment-tag').text(this);
             if (BUGZILLA.user.can_tag) {
-                span.prepend($('<a role="button" aria-label="Remove">x</a>').click(deleteTag));
+                span.prepend($('<a role="button" aria-label="Remove" class="remove">×</a>').click(deleteTag));
             }
             root.append(span);
         });
@@ -343,6 +343,55 @@ $(function() {
         }
     }
 
+    const saveTag = async () => {
+        hideTaggingUI();
+        $('#ctag-error').hide();
+
+        const $comment = $('#ctag').parents('.comment');
+        const commentNo = $comment.data('no');
+        const commentID = $comment.data('id');
+        const tags = $comment.data('tags').split(/[ ,]/);
+        const newTags = $('#ctag-add').val().trim().split(/[ ,]/).filter(tag => !tags.includes(tag));
+        const { min_comment_tag_length: min, max_comment_tag_length: max } = BUGZILLA.constant;
+
+        if (!newTags.length) {
+            return;
+        }
+
+        // validate
+        try {
+            newTags.forEach(tag => {
+                if (tag.length < min) {
+                    throw `Comment tags must be at least ${min} characters.`;
+                }
+                if (tag.length > max) {
+                    throw `Comment tags cannot be longer than ${max} characters.`;
+                }
+            });
+        } catch(ex) {
+            taggingError(commentNo, ex);
+            return;
+        }
+
+        // update ui
+        tags.push(...newTags);
+        tags.sort();
+        renderTags(commentNo, tags);
+
+        // update Bugzilla
+        try {
+            renderTags(commentNo, await Bugzilla.API.put(`bug/comment/${commentID}/tags`, { add: newTags }));
+            updateTagsMenu();
+        } catch ({ message }) {
+            taggingError(commentNo, message);
+            refreshTags(commentNo, commentID);
+        }
+    };
+
+    const hideTaggingUI = () => {
+        $('#ctag').hide().data('commentNo', '');
+    };
+
     $('#ctag-add')
         .devbridgeAutocomplete({
             appendTo: $('#main-inner'),
@@ -364,69 +413,27 @@ $(function() {
                 return suggestion.value.htmlEncode();
             }
         })
-        .keydown(async event => {
+        .keydown(event => {
             if (event.which === 27) {
                 event.preventDefault();
-                $('#ctag-close').click();
+                hideTaggingUI();
             }
             else if (event.which === 13) {
                 event.preventDefault();
-                $('#ctag-error').hide();
-
-                var ctag = $('#ctag');
-                var newTags = $('#ctag-add').val().trim().split(/[ ,]/);
-                var commentNo = ctag.data('commentNo');
-                var commentID = ctag.data('commentID');
-
-                $('#ctag-close').click();
-
-                // update ui
-                var tags = tagsFromDom($(this).parents('.comment-tags'));
-                var dirty = false;
-                var addTags = [];
-                $.each(newTags, function(index, value) {
-                    if ($.inArrayIn(value, tags) == -1)
-                        addTags.push(value);
-                });
-                if (addTags.length === 0)
-                    return;
-
-                // validate
-                try {
-                    $.each(addTags, function(index, value) {
-                        if (value.length < BUGZILLA.constant.min_comment_tag_length) {
-                            throw 'Comment tags must be at least ' +
-                                BUGZILLA.constant.min_comment_tag_length + ' characters.';
-                        }
-                        if (value.length > BUGZILLA.constant.max_comment_tag_length) {
-                            throw 'Comment tags cannot be longer than ' +
-                                BUGZILLA.constant.min_comment_tag_length + ' characters.';
-                        }
-                    });
-                } catch(ex) {
-                    taggingError(commentNo, ex);
-                    return;
-                }
-
-                Array.prototype.push.apply(tags, addTags);
-                tags.sort();
-                renderTags(commentNo, tags);
-
-                // update Bugzilla
-                try {
-                    renderTags(commentNo, await Bugzilla.API.put(`bug/comment/${commentID}/tags`, { add: addTags }));
-                    updateTagsMenu();
-                } catch ({ message }) {
-                    taggingError(commentNo, message);
-                    refreshTags(commentNo, commentID);
-                }
+                saveTag();
             }
         });
 
-    $('#ctag-close')
-        .click(function(event) {
+    $('#ctag-save')
+        .click(event => {
+            event.preventDefault();
+            saveTag();
+        });
+
+    $('#ctag-cancel')
+        .click(event => {
             event.preventDefault();
-            $('#ctag').hide().data('commentNo', '');
+            hideTaggingUI();
         });
 
     $('.tag-btn')