Merge pull request #7800 from Abijeet/bug-7281

Fixes the usability issues with the comment section delete and edit

6 files changed, 242 insertions, 104 deletions

diff --git a/apps/comments/css/comments.css b/apps/comments/css/comments.scss
index 311eeebe4db..c2bc136ba66 100644
--- a/apps/comments/css/comments.css
+++ b/apps/comments/css/comments.scss
@@ -13,53 +13,40 @@
 }
 
 #commentsTabView .newCommentForm {
-	position: relative;
-	margin-bottom: 20px;
+	margin-left: 36px;
 }
 
 #commentsTabView .newCommentForm .message {
-	width: calc(100% - 81px); /* 36 (left margin) + 30 (right padding) + 15 (right padding of surrounding box) */
-	margin-left: 36px;
-	padding-right: 30px;
-	display: block;
+	/* width = 100% - (width of submit button (44px) + margin (3px) + inline-block gap) */
+	width: calc(100% - 52px);
+	display: inline-block;
 }
 
 #commentsTabView .newCommentForm .submit {
-	position: absolute;
-	bottom: 0px;
-	right: 8px;
-	width: 30px;
+	width: 44px;
 	margin: 0;
-	padding: 7px 9px;
+	padding: 13px;
 	background-color: transparent;
 	border: none;
 	opacity: .3;
+	vertical-align: bottom;
 }
-#commentsTabView .newCommentForm .submit:not(:disabled):hover,
-#commentsTabView .newCommentForm .submit:not(:disabled):focus {
-	opacity: 1;
-}
-
-#commentsTabView .newCommentForm .submitLoading {
-	background-position: left;
 
-	/* Match rules for '#commentsTabView .newCommentForm .submit' to place the
-	   loading icon at the same position as the confirm icon */
-	position: absolute;
-	bottom: 0px;
-	right: 8px;
-	width: 30px;
-	margin: 0;
-	padding: 7px 9px;
+#commentsTabView .deleteLoading {
+	float: right;
+	padding: 14px;
+	vertical-align: middle;
+}
 
-	/* Match rules for 'input[type="submit"]' to place the loading icon at the
-	   same position as the confirm icon */
-	min-height: 34px;
-	box-sizing: border-box;
+#commentsTabView .submitLoading {
+	vertical-align: bottom;
+	display: inline-block;
+	padding: 14px;
 }
 
-#commentsTabView .newCommentForm .cancel {
-	margin-right: 6px;
+#commentsTabView .newCommentForm .submit:not(:disabled):hover,
+#commentsTabView .newCommentForm .submit:not(:disabled):focus {
+	opacity: 1;
 }
 
 #commentsTabView .newCommentForm div.message {
@@ -73,11 +60,16 @@
 
 #commentsTabView .comment {
 	position: relative;
-	margin-bottom: 30px;
+	/** padding bottom is little more so that the top and bottom gap look uniform **/
+	padding: 10px 0px 15px;
 	word-wrap: break-word;
 	overflow-wrap: break-word;
 }
 
+#commentsTabView .comments .comment {
+	border-top: 1px solid $color-border;
+}
+
 #commentsTabView .comment .avatar,
 .atwho-view-ul * .avatar{
 	width: 32px;
@@ -123,19 +115,22 @@
 	background: -o-linear-gradient(rgba(255,255,255,0), rgba(255,255,255,1));
 	background: -ms-linear-gradient(rgba(255,255,255,0), rgba(255,255,255,1));
 	background: linear-gradient(rgba(255,255,255,0), rgba(255,255,255,1));
-	filter: progid:DXImageTransform.Microsoft.gradient(GradientType=0,startColorstr='#00FFFFFF', endColorstr='#FFFFFFFF');
 	background-repeat: no-repeat;
 }
 
-#commentsTabView .authorRow>div:not(.contactsmenu-popover) {
-	display: inline-block;
-	vertical-align: middle;
-}
-
-#commentsTabView .authorRow>div.hidden {
+#commentsTabView .hidden {
 	display: none !important;
 }
 
+/** set min-height as 44px to ensure that it fits the button sizes. **/
+#commentsTabView .comment .authorRow {
+	min-height: 44px;
+}
+#commentsTabView .comment .authorRow .tooltip {
+	/** because of the padding on the element, the tooltip appear too far up,
+	 adding this brings them closer to the element**/
+	margin-top: 5px;
+}
 #commentsTabView .comments li .message .avatar-name-wrapper,
 .atwho-view-ul * .avatar-name-wrapper,
 #commentsTabView .comment .authorRow {
@@ -164,47 +159,48 @@
 .atwho-view-ul * .avatar-name-wrapper {
 	white-space: nowrap;
 }
-
 #commentsTabView .comment .author,
 #commentsTabView .comment .date {
 	opacity: .5;
 }
+#commentsTabView .comment .author {
+	max-width: 210px;
+	text-overflow: ellipsis;
+	overflow: hidden;
+	white-space: nowrap;
+}
 #commentsTabView .comment .date {
 	margin-left: auto;
+	/** this is to fix the tooltip being too close due to the margin-top applied
+	 to bring the tooltip closer for the action icons **/
+	padding: 10px 0px;
 }
 
-#commentsTabView .comments li .message {
+#commentsTabView .comments > li:not(.newCommentRow) .message {
 	padding-left: 40px;
-	display: inline-flex;
-	flex-wrap: wrap;
-	align-items: center;
 }
 
 #commentsTabView .comment .action {
-	opacity: 0;
-	padding: 5px;
-}
-
-#commentsTabView .comment:hover .action {
 	opacity: 0.3;
+	padding: 14px;
+	display: block;
 }
 
-#commentsTabView .comment .action:hover {
+#commentsTabView .comment .action:hover,
+#commentsTabView .comment .action:focus {
 	opacity: 1;
 }
 
-#commentsTabView .comment .action.delete,
-#commentsTabView .comment .deleteLoading {
-	position: absolute;
-	right: 0;
+#commentsTabView .newCommentRow .action-container {
+	margin-left: auto;
 }
 
-#commentsTabView .comment.disabled {
+#commentsTabView .comment.disabled .message {
 	opacity: 0.3;
 }
 
 #commentsTabView .comment.disabled .action {
-	visibility: hidden;
+	display: none;
 }
 
 #commentsTabView .message.error {
@@ -215,4 +211,4 @@
 
 .app-files .action-comment {
 	padding: 16px 14px;
-}
+}
\ No newline at end of file
diff --git a/apps/comments/js/commentsmodifymenu.js b/apps/comments/js/commentsmodifymenu.js
new file mode 100644
index 00000000000..4b17cbbfbf0
--- /dev/null
+++ b/apps/comments/js/commentsmodifymenu.js
@@ -0,0 +1,124 @@
+/*
+ * Copyright (c) 2018
+ *
+ * This file is licensed under the Affero General Public License version 3
+ * or later.
+ *
+ * See the COPYING-README file.
+ *
+ */
+
+/* global Handlebars */
+(function() {
+	var TEMPLATE_MENU =
+		'<ul>' +
+		'{{#each items}}' +
+		'<li>' +
+		'<a href="#" class="menuitem action {{name}} permanent" data-action="{{name}}">' +
+			'{{#if iconClass}}' +
+				'<span class="icon {{iconClass}}"></span>' +
+			'{{else}}' +
+				'<span class="no-icon"></span>' +
+			'{{/if}}' +
+			'<span>{{displayName}}</span>' +
+		'</li>' +
+		'{{/each}}' +
+		'</ul>';
+
+	/**
+	 * Construct a new CommentsModifyMenuinstance
+	 * @constructs CommentsModifyMenu
+	 * @memberof OC.Comments
+	 */
+	var CommentsModifyMenu = OC.Backbone.View.extend({
+		tagName: 'div',
+		className: 'commentsModifyMenu popovermenu bubble menu',
+		_scopes: [
+			{
+				name: 'edit',
+				displayName:  t('comments', 'Edit comment'),
+				iconClass: 'icon-rename'
+			},
+			{
+				name: 'delete',
+				displayName: t('comments', 'Delete comment'),
+				iconClass: 'icon-delete'
+			}
+		],
+		initialize: function() {
+
+		},
+		events: {
+			'click a.action': '_onClickAction'
+		},
+
+		template: Handlebars.compile(TEMPLATE_MENU),
+
+		/**
+		 * Event handler whenever an action has been clicked within the menu
+		 *
+		 * @param {Object} event event object
+		 */
+		_onClickAction: function(event) {
+			var $target = $(event.currentTarget);
+			if (!$target.hasClass('menuitem')) {
+				$target = $target.closest('.menuitem');
+			}
+
+			OC.hideMenus();
+
+			this.trigger('select:menu-item-clicked', event, $target.data('action'));
+		},
+
+		/**
+		 * Renders the menu with the currently set items
+		 */
+		render: function() {
+			this.$el.html(this.template({
+				items: this._scopes
+			}));
+		},
+
+		/**
+		 * Displays the menu
+		 */
+		show: function(context) {
+			this._context = context;
+
+			for(var i in this._scopes) {
+				this._scopes[i].active = false;
+			}
+
+
+			var $el = $(context.target);
+			var offsetIcon = $el.offset();
+			var offsetContainer = $el.closest('.authorRow').offset();
+
+			// adding some extra top offset to push the menu below the button.
+			var position = {
+				top: offsetIcon.top - offsetContainer.top + 48,
+				left: '',
+				right: ''
+			};
+
+			position.left = offsetIcon.left - offsetContainer.left;
+
+			if (position.left > 200) {
+				// we need to position the menu to the right.
+				position.left = '';
+				position.right = this.$el.closest('.comment').find('.date').width();
+				this.$el.removeClass('menu-left').addClass('menu-right');
+			} else {
+				this.$el.removeClass('menu-right').addClass('menu-left');
+			}
+			this.$el.css(position);
+			this.render();
+			this.$el.removeClass('hidden');
+
+			OC.showMenu(null, this.$el);
+		}
+	});
+
+	OCA.Comments = OCA.Comments || {};
+	OCA.Comments.CommentsModifyMenu = CommentsModifyMenu;
+})(OC, OCA);
\ No newline at end of file
diff --git a/apps/comments/js/commentstabview.js b/apps/comments/js/commentstabview.js
index 20f1f590a28..9477cb0c301 100644
--- a/apps/comments/js/commentstabview.js
+++ b/apps/comments/js/commentstabview.js
@@ -21,24 +21,22 @@
 		'<div class="loading hidden" style="height: 50px"></div>';
 
 	var EDIT_COMMENT_TEMPLATE =
-		'<div class="newCommentRow comment" data-id="{{id}}">' +
+		'<{{tag}} class="newCommentRow comment" data-id="{{id}}">' +
 		'    <div class="authorRow">' +
 		'        <div class="avatar currentUser" data-username="{{actorId}}"></div>' +
 		'        <div class="author currentUser">{{actorDisplayName}}</div>' +
 		'{{#if isEditMode}}' +
-		'        <a href="#" class="action delete icon icon-delete has-tooltip" title="{{deleteTooltip}}"></a>' +
-		'        <div class="deleteLoading icon-loading-small hidden"></div>'+
+		'        <div class="action-container">' +
+		'            <a href="#" class="action cancel icon icon-close has-tooltip" title="{{cancelText}}"></a>' +
+		'        </div>' +
 		'{{/if}}' +
 		'    </div>' +
 		'    <form class="newCommentForm">' +
 		'        <div contentEditable="true" class="message" data-placeholder="{{newMessagePlaceholder}}">{{message}}</div>' +
-		'        <input class="submit icon-confirm" type="submit" value="" />' +
-		'{{#if isEditMode}}' +
-		'        <input class="cancel pull-right" type="button" value="{{cancelText}}" />' +
-		'{{/if}}' +
+		'        <input class="submit icon-confirm has-tooltip" type="submit" value="" title="{{submitText}}"/>' +
 		'        <div class="submitLoading icon-loading-small hidden"></div>'+
 		'    </form>' +
-		'</div>';
+		'</{{tag}}>';
 
 	var COMMENT_TEMPLATE =
 		'<li class="comment{{#if isUnread}} unread{{/if}}{{#if isLong}} collapsed{{/if}}" data-id="{{id}}">' +
@@ -46,7 +44,8 @@
 		'        <div class="avatar{{#if isUserAuthor}} currentUser{{/if}}" {{#if actorId}}data-username="{{actorId}}"{{/if}}> </div>' +
 		'        <div class="author{{#if isUserAuthor}} currentUser{{/if}}">{{actorDisplayName}}</div>' +
 		'{{#if isUserAuthor}}' +
-		'        <a href="#" class="action edit icon icon-rename has-tooltip" title="{{editTooltip}}"></a>' +
+		'        <a href="#" class="action more icon icon-more has-tooltip"></a>' +
+		'        <div class="deleteLoading icon-loading-small hidden"></div>' +
 		'{{/if}}' +
 		'        <div class="date has-tooltip live-relative-timestamp" data-timestamp="{{timestamp}}" title="{{altDate}}">{{date}}</div>' +
 		'    </div>' +
@@ -64,12 +63,11 @@
 		id: 'commentsTabView',
 		className: 'tab commentsTabView',
 		_autoCompleteData: undefined,
+		_commentsModifyMenu: undefined,
 
 		events: {
 			'submit .newCommentForm': '_onSubmitComment',
 			'click .showMore': '_onClickShowMore',
-			'click .action.edit': '_onClickEditComment',
-			'click .action.delete': '_onClickDeleteComment',
 			'click .cancel': '_onClickCloseComment',
 			'click .comment': '_onClickComment',
 			'keyup div.message': '_onTextChange',
@@ -114,9 +112,9 @@
 				actorId: currentUser.uid,
 				actorDisplayName: currentUser.displayName,
 				newMessagePlaceholder: t('comments', 'New comment …'),
-				deleteTooltip: t('comments', 'Delete comment'),
 				submitText: t('comments', 'Post'),
-				cancelText: t('comments', 'Cancel')
+				cancelText: t('comments', 'Cancel'),
+				tag: 'li'
 			}, params));
 		},
 
@@ -166,7 +164,7 @@
 				emptyResultLabel: t('comments', 'No comments yet, start the conversation!'),
 				moreLabel: t('comments', 'More comments …')
 			}));
-			this.$el.find('.comments').before(this.editCommentTemplate({}));
+			this.$el.find('.comments').before(this.editCommentTemplate({ tag: 'div'}));
 			this.$el.find('.has-tooltip').tooltip();
 			this.$container = this.$el.find('ul.comments');
 			this.$el.find('.avatar').avatar(OC.getCurrentUser().uid, 32);
@@ -401,6 +399,24 @@
 				// it is the case when writing a comment and mentioning a person
 				$message = $el;
 			}
+
+
+			if (!editionMode) {
+				var self = this;
+				// add the dropdown menu to display the edit and delete option
+				var modifyCommentMenu = new OCA.Comments.CommentsModifyMenu();
+				$el.find('.authorRow').append(modifyCommentMenu.$el);
+				$el.find('.more').on('click', _.bind(modifyCommentMenu.show, modifyCommentMenu));
+
+				self.listenTo(modifyCommentMenu, 'select:menu-item-clicked', function(ev, action) {
+					if (action === 'edit') {
+						self._onClickEditComment(ev);
+					} else if (action === 'delete') {
+						self._onClickDeleteComment(ev);
+					}
+				});
+			}
+
 			this._postRenderMessage($message, editionMode);
 		},
 
@@ -567,15 +583,13 @@
 			var $comment = $(ev.target).closest('.comment');
 			var commentId = $comment.data('id');
 			var $loading = $comment.find('.deleteLoading');
-			var $commentField = $comment.find('.message');
-			var $submit = $comment.find('.submit');
-			var $cancel = $comment.find('.cancel');
+			var $moreIcon = $comment.find('.more');
 
-			$commentField.prop('contenteditable', false);
-			$submit.prop('disabled', true);
-			$cancel.prop('disabled', true);
 			$comment.addClass('disabled');
 			$loading.removeClass('hidden');
+			$moreIcon.addClass('hidden');
+
+			$comment.data('commentEl', $comment);
 
 			this.collection.get(commentId).destroy({
 				success: function() {
@@ -584,10 +598,8 @@
 				},
 				error: function() {
 					$loading.addClass('hidden');
+					$moreIcon.removeClass('hidden');
 					$comment.removeClass('disabled');
-					$commentField.prop('contenteditable', true);
-					$submit.prop('disabled', false);
-					$cancel.prop('disabled', false);
 
 					OC.Notification.showTemporary(t('comments', 'Error occurred while retrieving comment with ID {id}', {id: commentId}));
 				}
@@ -668,6 +680,12 @@
 				}, {
 					success: function(model) {
 						self._onSubmitSuccess(model, $form);
+						if(model.get('message').trim() === model.previous('message').trim()) {
+							// model change event doesn't trigger, manually remove the row.
+							var $row = $form.closest('.comment');
+							$row.data('commentEl').removeClass('hidden');
+							$row.remove();
+						}
 					},
 					error: function() {
 						self._onSubmitError($form, commentId);
diff --git a/apps/comments/js/merged.json b/apps/comments/js/merged.json
index 0202c7ff55a..6e77d9cf80a 100644
--- a/apps/comments/js/merged.json
+++ b/apps/comments/js/merged.json
@@ -4,6 +4,7 @@
   "commentcollection.js",
   "commentsummarymodel.js",
   "commentstabview.js",
+  "commentsmodifymenu.js",
   "filesplugin.js",
   "activitytabviewplugin.js",
   "vendor/Caret.js/dist/jquery.caret.min.js",
diff --git a/apps/comments/tests/js/commentstabviewSpec.js b/apps/comments/tests/js/commentstabviewSpec.js
index 0131bc7bce3..c90ad04e419 100644
--- a/apps/comments/tests/js/commentstabviewSpec.js
+++ b/apps/comments/tests/js/commentstabviewSpec.js
@@ -190,7 +190,7 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 
 			expect(fetchStub.notCalled).toEqual(true);
 
-			view.$el.find('.showMore').click();
+			view.$el.find('.showMore').trigger('click');
 
 			expect(fetchStub.calledOnce).toEqual(true);
 		});
@@ -398,10 +398,10 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 				$message = $newCommentForm.find('.message');
 				$submitButton = $newCommentForm.find('.submit');
 			});
-			afterEach(function() { 
-				tooltipStub.restore(); 
+			afterEach(function() {
+				tooltipStub.restore();
 			});
-			
+
 			it('does not displays tooltip when limit is far away', function() {
 				$message.val(createMessageWithLength(3));
 				$message.trigger('change');
@@ -490,18 +490,21 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 		it('shows edit link for owner comments', function() {
 			var $comment = view.$el.find('.comment[data-id=1]');
 			expect($comment.length).toEqual(1);
+			$comment.find('.action.more').trigger('click');
 			expect($comment.find('.action.edit').length).toEqual(1);
 		});
 
 		it('does not show edit link for other user\'s comments', function() {
 			var $comment = view.$el.find('.comment[data-id=2]');
 			expect($comment.length).toEqual(1);
+			$comment.find('.action.more').trigger('click');
 			expect($comment.find('.action.edit').length).toEqual(0);
 		});
 
 		it('shows edit form when clicking edit', function() {
 			var $comment = view.$el.find('.comment[data-id=1]');
-			$comment.find('.action.edit').click();
+			$comment.find('.action.more').trigger('click');
+			$comment.find('.action.edit').trigger('click');
 
 			expect($comment.hasClass('hidden')).toEqual(true);
 			var $formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
@@ -510,7 +513,8 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 
 		it('saves message and updates comment item when clicking save', function() {
 			var $comment = view.$el.find('.comment[data-id=1]');
-			$comment.find('.action.edit').click();
+			$comment.find('.action.more').trigger('click');
+			$comment.find('.action.edit').trigger('click');
 
 			var $formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
 			expect($formRow.length).toEqual(1);
@@ -544,7 +548,8 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 
 		it('saves message and updates comment item with mentions when clicking save', function() {
 			var $comment = view.$el.find('.comment[data-id=3]');
-			$comment.find('.action.edit').click();
+			$comment.find('.action.more').trigger('click');
+			$comment.find('.action.edit').trigger('click');
 
 			var $formRow = view.$el.find('.newCommentRow.comment[data-id=3]');
 			expect($formRow.length).toEqual(1);
@@ -591,13 +596,14 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 
 		it('restores original comment when cancelling', function() {
 			var $comment = view.$el.find('.comment[data-id=1]');
-			$comment.find('.action.edit').click();
+			$comment.find('.action.more').trigger('click');
+			$comment.find('.action.edit').trigger('click');
 
 			var $formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
 			expect($formRow.length).toEqual(1);
 
 			$formRow.find('textarea').val('modified\nmessage');
-			$formRow.find('.cancel').click();
+			$formRow.find('.cancel').trigger('click');
 
 			expect(saveStub.notCalled).toEqual(true);
 
@@ -614,12 +620,8 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 		it('destroys model when clicking delete', function() {
 			var destroyStub = sinon.stub(OCA.Comments.CommentModel.prototype, 'destroy');
 			var $comment = view.$el.find('.comment[data-id=1]');
-			$comment.find('.action.edit').click();
-
-			var $formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
-			expect($formRow.length).toEqual(1);
-
-			$formRow.find('.delete').click();
+			$comment.find('.action.more').trigger('click');
+			$comment.find('.action.delete').trigger('click');
 
 			expect(destroyStub.calledOnce).toEqual(true);
 			expect(destroyStub.thisValues[0].id).toEqual(1);
@@ -630,15 +632,11 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 			$comment = view.$el.find('.comment[data-id=1]');
 			expect($comment.length).toEqual(0);
 
-			// form row is gone
-			$formRow = view.$el.find('.newCommentRow.comment[data-id=1]');
-			expect($formRow.length).toEqual(0);
-
 			destroyStub.restore();
 		});
 		it('does not submit comment if the field is empty', function() {
 			var $comment = view.$el.find('.comment[data-id=1]');
-			$comment.find('.action.edit').click();
+			$comment.find('.action.edit').trigger('click');
 			$comment.find('.message').val('   ');
 			$comment.find('form').submit();
 
@@ -646,7 +644,7 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 		});
 		it('does not submit comment if the field length is too large', function() {
 			var $comment = view.$el.find('.comment[data-id=1]');
-			$comment.find('.action.edit').click();
+			$comment.find('.action.edit').trigger('click');
 			$comment.find('.message').val(createMessageWithLength(view._commentMaxLength * 2));
 			$comment.find('form').submit();
 
@@ -659,7 +657,7 @@ describe('OCA.Comments.CommentsTabView tests', function() {
 		beforeEach(function() {
 			updateMarkerStub = sinon.stub(OCA.Comments.CommentCollection.prototype, 'updateReadMarker');
 		});
-		afterEach(function() { 
+		afterEach(function() {
 			updateMarkerStub.restore();
 		});
 
diff --git a/tests/karma.config.js b/tests/karma.config.js
index 0254d6a3335..3758636f074 100644
--- a/tests/karma.config.js
+++ b/tests/karma.config.js
@@ -93,6 +93,7 @@ module.exports = function(config) {
 					'apps/comments/js/commentmodel.js',
 					'apps/comments/js/commentcollection.js',
 					'apps/comments/js/commentsummarymodel.js',
+					'apps/comments/js/commentsmodifymenu.js',
 					'apps/comments/js/commentstabview.js',
 					'apps/comments/js/filesplugin.js'
 				],
@@ -223,7 +224,7 @@ module.exports = function(config) {
 	// serve images to avoid warnings
 	files.push({pattern: 'core/img/**/*', watched: false, included: false, served: true});
 	files.push({pattern: 'core/css/images/*', watched: false, included: false, served: true});
-	
+
 	// include core CSS
 	files.push({pattern: 'core/css/*.css', watched: true, included: true, served: true});
 	files.push({pattern: 'tests/css/*.css', watched: true, included: true, served: true});