From 8a7baa2880e08d6c16b6eb7ca966dc799b580f9f Mon Sep 17 00:00:00 2001 From: jboyson Date: Tue, 6 Apr 2021 13:02:05 -0500 Subject: [PATCH 1/6] Replace button with div Due to a bug in Firefox buttons are not draggable: https://bugzilla.mozilla.org/show_bug.cgi?id=568313 This changes the button to a div and uses "role" to keep the semantics for ARIA --- app/assets/javascripts/diffs/components/diff_row.vue | 5 +++-- ...-comment-icon-for-multiline-comment-broken-on-firefox.yml | 5 +++++ 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/327013-draggable-comment-icon-for-multiline-comment-broken-on-firefox.yml diff --git a/app/assets/javascripts/diffs/components/diff_row.vue b/app/assets/javascripts/diffs/components/diff_row.vue index 8d398a2ded4692..0898a6968ac960 100644 --- a/app/assets/javascripts/diffs/components/diff_row.vue +++ b/app/assets/javascripts/diffs/components/diff_row.vue @@ -215,7 +215,8 @@ export default { class="add-diff-note tooltip-wrapper" :title="addCommentTooltipLeft" > - + > Date: Mon, 12 Apr 2021 11:38:53 -0500 Subject: [PATCH 2/6] Add keyboard support --- app/assets/javascripts/diffs/components/diff_row.vue | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/assets/javascripts/diffs/components/diff_row.vue b/app/assets/javascripts/diffs/components/diff_row.vue index 0898a6968ac960..3a225c66f48dc9 100644 --- a/app/assets/javascripts/diffs/components/diff_row.vue +++ b/app/assets/javascripts/diffs/components/diff_row.vue @@ -217,6 +217,7 @@ export default { >
-- GitLab From 6511e9fdfe2736829b082e7366136c41e39cbb94 Mon Sep 17 00:00:00 2001 From: jboyson Date: Wed, 21 Apr 2021 15:24:57 -0500 Subject: [PATCH 3/6] Update to be more ARIA friendly --- .../javascripts/diffs/components/diff_row.vue | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_row.vue b/app/assets/javascripts/diffs/components/diff_row.vue index 3a225c66f48dc9..6a19b3b4f084b8 100644 --- a/app/assets/javascripts/diffs/components/diff_row.vue +++ b/app/assets/javascripts/diffs/components/diff_row.vue @@ -157,8 +157,8 @@ export default { table.classList.add(`${lineClass}-selected`); } }, - handleCommentButton(line) { - this.showCommentForm({ lineCode: line.line_code, fileHash: this.fileHash }); + handleCommentButton(line, disabled) { + if (!disabled) this.showCommentForm({ lineCode: line.line_code, fileHash: this.fileHash }); }, conflictText(line) { return line.type === CONFLICT_MARKER_THEIR @@ -223,10 +223,10 @@ export default { class="add-diff-note unified-diff-components-diff-note-button note-button js-add-diff-note-button qa-diff-comment" data-qa-selector="diff_comment_button" :class="{ 'gl-cursor-grab': dragging }" - :disabled="line.left.commentsDisabled" - @click="handleCommentButton(line.left)" - @keydown.enter="handleCommentButton(line.left)" - @keydown.space="handleCommentButton(line.left)" + :aria-disabled="line.left.commentsDisabled" + @click="handleCommentButton(line.left, line.left.commentsDisabled)" + @keydown.enter="handleCommentButton(line.left, line.left.commentsDisabled)" + @keydown.space="handleCommentButton(line.left, line.left.commentsDisabled)" @dragstart="onDragStart({ ...line.left, index })" > @@ -321,15 +321,19 @@ export default { class="add-diff-note tooltip-wrapper" :title="addCommentTooltipRight" > - + > Date: Thu, 22 Apr 2021 15:44:36 -0500 Subject: [PATCH 4/6] Apply patch fix disabled and event handling --- .../javascripts/diffs/components/diff_row.vue | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_row.vue b/app/assets/javascripts/diffs/components/diff_row.vue index 6a19b3b4f084b8..7ca595b0b2e23c 100644 --- a/app/assets/javascripts/diffs/components/diff_row.vue +++ b/app/assets/javascripts/diffs/components/diff_row.vue @@ -157,8 +157,8 @@ export default { table.classList.add(`${lineClass}-selected`); } }, - handleCommentButton(line, disabled) { - if (!disabled) this.showCommentForm({ lineCode: line.line_code, fileHash: this.fileHash }); + handleCommentButton(line) { + this.showCommentForm({ lineCode: line.line_code, fileHash: this.fileHash }); }, conflictText(line) { return line.type === CONFLICT_MARKER_THEIR @@ -218,16 +218,17 @@ export default {
@@ -324,15 +325,16 @@ export default {
-- GitLab From f33003a7c319f63bbffc1a06ae18aec9ace57206 Mon Sep 17 00:00:00 2001 From: jboyson Date: Thu, 22 Apr 2021 16:24:20 -0500 Subject: [PATCH 5/6] Update tests to assert events --- .../javascripts/diffs/components/diff_row.vue | 4 +- .../diffs/components/diff_row_spec.js | 60 ++++++++++++++++--- 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_row.vue b/app/assets/javascripts/diffs/components/diff_row.vue index 7ca595b0b2e23c..d6fffc64b90938 100644 --- a/app/assets/javascripts/diffs/components/diff_row.vue +++ b/app/assets/javascripts/diffs/components/diff_row.vue @@ -211,11 +211,11 @@ export default {
{ }, ]; - const createWrapper = ({ props, state, isLoggedIn = true }) => { - const localVue = createLocalVue(); - localVue.use(Vuex); + const createWrapper = ({ props, state, actions, isLoggedIn = true }) => { + Vue.use(Vuex); const diffs = diffsModule(); diffs.state = { ...diffs.state, ...state }; + diffs.actions = { ...diffs.actions, ...actions }; const getters = { isLoggedIn: () => isLoggedIn }; @@ -54,7 +55,7 @@ describe('DiffRow', () => { glFeatures: { dragCommentSelection: true }, }; - return shallowMount(DiffRow, { propsData, localVue, store, provide }); + return shallowMount(DiffRow, { propsData, store, provide }); }; it('isHighlighted returns true given line.left', () => { @@ -95,6 +96,8 @@ describe('DiffRow', () => { expect(wrapper.vm.isHighlighted).toBe(false); }); + const getCommentButton = (wrapper, side) => wrapper.find(`[data-testid="${side}CommentButton"]`); + describe.each` side ${'left'} @@ -106,9 +109,50 @@ describe('DiffRow', () => { expect(wrapper.find(`[data-testid="${side}EmptyCell"]`).exists()).toBe(true); }); - it('renders comment button', () => { - const wrapper = createWrapper({ props: { line: testLines[3], inline: false } }); - expect(wrapper.find(`[data-testid="${side}CommentButton"]`).exists()).toBe(true); + describe('comment button', () => { + const showCommentForm = jest.fn(); + let line; + + beforeEach(() => { + showCommentForm.mockReset(); + // https://eslint.org/docs/rules/prefer-destructuring#when-not-to-use-it + // eslint-disable-next-line prefer-destructuring + line = testLines[3]; + }); + + it('renders', () => { + const wrapper = createWrapper({ props: { line, inline: false } }); + expect(getCommentButton(wrapper, side).exists()).toBe(true); + }); + + it('responds to click and keyboard events', async () => { + const wrapper = createWrapper({ + props: { line, inline: false }, + actions: { showCommentForm }, + }); + const commentButton = getCommentButton(wrapper, side); + + await commentButton.trigger('click'); + await commentButton.trigger('keydown.enter'); + await commentButton.trigger('keydown.space'); + + expect(showCommentForm).toHaveBeenCalledTimes(3); + }); + + it('ignores click and keyboard events when comments are disabled', async () => { + line[side].commentsDisabled = true; + const wrapper = createWrapper({ + props: { line, inline: false }, + actions: { showCommentForm }, + }); + const commentButton = getCommentButton(wrapper, side); + + await commentButton.trigger('click'); + await commentButton.trigger('keydown.enter'); + await commentButton.trigger('keydown.space'); + + expect(showCommentForm).not.toHaveBeenCalled(); + }); }); it('renders avatars', () => { -- GitLab From 8abcb94f5eca6d8e034f06699ab733c113e39d8a Mon Sep 17 00:00:00 2001 From: jboyson Date: Fri, 23 Apr 2021 09:56:10 -0500 Subject: [PATCH 6/6] Update test ids to be kebab case --- .../javascripts/diffs/components/diff_row.vue | 16 ++++++++-------- spec/frontend/diffs/components/diff_row_spec.js | 9 +++++---- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_row.vue b/app/assets/javascripts/diffs/components/diff_row.vue index d6fffc64b90938..c5770a35b42b7f 100644 --- a/app/assets/javascripts/diffs/components/diff_row.vue +++ b/app/assets/javascripts/diffs/components/diff_row.vue @@ -204,7 +204,7 @@ export default {