From fc8b317f803c6ad43d5b4f4acd1fd4d523093883 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Sun, 16 Apr 2017 11:10:33 +0100 Subject: Protected against synthetic clicks and added feature scenario --- app/assets/javascripts/comment_type_toggle.js | 1 - app/assets/javascripts/droplab/constants.js | 2 ++ app/assets/javascripts/droplab/drop_down.js | 3 ++- app/assets/javascripts/droplab/plugins/input_setter.js | 3 --- app/assets/stylesheets/framework/dropdowns.scss | 4 ++++ app/views/projects/notes/_comment_button.html.haml | 2 +- spec/javascripts/droplab/constants_spec.js | 6 ++++++ spec/support/features/discussion_comments_shared_example.rb | 8 +++++++- 8 files changed, 22 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/comment_type_toggle.js b/app/assets/javascripts/comment_type_toggle.js index c175f89c4fa..df0ba86198c 100644 --- a/app/assets/javascripts/comment_type_toggle.js +++ b/app/assets/javascripts/comment_type_toggle.js @@ -24,7 +24,6 @@ class CommentTypeToggle { InputSetter: [{ input: this.noteTypeInput, valueAttribute: 'data-value', - allowEmpty: true, }, { input: this.submitButton, diff --git a/app/assets/javascripts/droplab/constants.js b/app/assets/javascripts/droplab/constants.js index a23d914772a..8883ed9aa14 100644 --- a/app/assets/javascripts/droplab/constants.js +++ b/app/assets/javascripts/droplab/constants.js @@ -2,10 +2,12 @@ const DATA_TRIGGER = 'data-dropdown-trigger'; const DATA_DROPDOWN = 'data-dropdown'; const SELECTED_CLASS = 'droplab-item-selected'; const ACTIVE_CLASS = 'droplab-item-active'; +const IGNORE_CLASS = 'droplab-item-ignore'; export { DATA_TRIGGER, DATA_DROPDOWN, SELECTED_CLASS, ACTIVE_CLASS, + IGNORE_CLASS, }; diff --git a/app/assets/javascripts/droplab/drop_down.js b/app/assets/javascripts/droplab/drop_down.js index 9588921ebcd..1fb4d63923c 100644 --- a/app/assets/javascripts/droplab/drop_down.js +++ b/app/assets/javascripts/droplab/drop_down.js @@ -1,7 +1,7 @@ /* eslint-disable */ import utils from './utils'; -import { SELECTED_CLASS } from './constants'; +import { SELECTED_CLASS, IGNORE_CLASS } from './constants'; var DropDown = function(list) { this.currentIndex = 0; @@ -36,6 +36,7 @@ Object.assign(DropDown.prototype, { clickEvent: function(e) { if (e.target.tagName === 'UL') return; + if (e.target.classList.contains(IGNORE_CLASS)) return; var selected = utils.closest(e.target, 'LI'); if (!selected) return; diff --git a/app/assets/javascripts/droplab/plugins/input_setter.js b/app/assets/javascripts/droplab/plugins/input_setter.js index 0736d525f40..d01fbc5830d 100644 --- a/app/assets/javascripts/droplab/plugins/input_setter.js +++ b/app/assets/javascripts/droplab/plugins/input_setter.js @@ -35,9 +35,6 @@ const InputSetter = { const newValue = selectedItem.getAttribute(config.valueAttribute); const inputAttribute = config.inputAttribute; - console.log('newValue', newValue, 'allowEmpty', config.allowEmpty); - if (!newValue && !config.allowEmpty) return; - if (input.hasAttribute(inputAttribute)) return input.setAttribute(inputAttribute, newValue); if (input.tagName === 'INPUT') return input.value = newValue; return input.textContent = newValue; diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 7767826b033..b87e712c763 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -564,3 +564,7 @@ color: $gl-text-color-secondary; } } + +.droplab-item-ignore { + pointer-events: none; +} diff --git a/app/views/projects/notes/_comment_button.html.haml b/app/views/projects/notes/_comment_button.html.haml index 6bb55f04b6e..29cf5825292 100644 --- a/app/views/projects/notes/_comment_button.html.haml +++ b/app/views/projects/notes/_comment_button.html.haml @@ -16,7 +16,7 @@ %p Add a general comment to this #{noteable_name}. - %li.divider + %li.divider.droplab-item-ignore %li#discussion{ data: { value: 'DiscussionNote', 'submit-text' => 'Start discussion', 'close-text' => "Start discussion & close #{noteable_name}", 'reopen-text' => "Start discussion & reopen #{noteable_name}" } } %a{ href: '#' } diff --git a/spec/javascripts/droplab/constants_spec.js b/spec/javascripts/droplab/constants_spec.js index 35239e4fb8e..fd153a49fcd 100644 --- a/spec/javascripts/droplab/constants_spec.js +++ b/spec/javascripts/droplab/constants_spec.js @@ -26,4 +26,10 @@ describe('constants', function () { expect(constants.ACTIVE_CLASS).toBe('droplab-item-active'); }); }); + + describe('IGNORE_CLASS', function () { + it('should be `droplab-item-ignore`', function() { + expect(constants.IGNORE_CLASS).toBe('droplab-item-ignore'); + }); + }); }); diff --git a/spec/support/features/discussion_comments_shared_example.rb b/spec/support/features/discussion_comments_shared_example.rb index 1a061ef069e..bb4542b1683 100644 --- a/spec/support/features/discussion_comments_shared_example.rb +++ b/spec/support/features/discussion_comments_shared_example.rb @@ -73,9 +73,15 @@ shared_examples 'discussion comments' do |resource_name| expect(page).not_to have_selector menu_selector end - it 'clicking the ul padding should not change the text' do + it 'clicking the ul padding or divider should not change the text' do find(menu_selector).trigger 'click' + expect(page).to have_selector menu_selector + expect(find(dropdown_selector)).to have_content 'Comment' + + find("#{menu_selector} .divider").trigger 'click' + + expect(page).to have_selector menu_selector expect(find(dropdown_selector)).to have_content 'Comment' end -- cgit v1.2.1