From e665ea050a9fcdb69a8d40db4ed120fe35bb0b97 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 31 Jan 2017 14:44:01 +0530 Subject: Add Ctrl+Click support --- app/assets/javascripts/todos.js.es6 | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/todos.js.es6 b/app/assets/javascripts/todos.js.es6 index 96c7d927509..bf84d82af97 100644 --- a/app/assets/javascripts/todos.js.es6 +++ b/app/assets/javascripts/todos.js.es6 @@ -147,13 +147,28 @@ goToTodoUrl(e) { const todoLink = $(this).data('url'); + let targetLink = $(e.target).attr('href'); + + if ($(e.target).is('img')) { // See if clicked target was Avatar + targetLink = $(e.target).parent().attr('href'); // Parent of Avatar is link + } + if (!todoLink) { return; } - // Allow Meta-Click or Mouse3-click to open in a new tab - if (e.metaKey || e.which === 2) { + + // Allow Meta-Click (Cmd+Click or Ctrl+Click) + // or Mouse3-click (middle-click) + // to open in a new tab + if (e.metaKey || e.ctrlKey || e.which === 2) { e.preventDefault(); - return window.open(todoLink, '_blank'); + // Meta-Click on username leads to different URL than todoLink. + // Turbolinks can resolve that URL, but window.open requires URL manually. + if (targetLink !== todoLink) { + return window.open(targetLink, '_blank'); + } else { + return window.open(todoLink, '_blank'); + } } else { return gl.utils.visitUrl(todoLink); } -- cgit v1.2.1 From 4a8d3b2768f031eb79dde268eedc8fdb38efdb3a Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 31 Jan 2017 14:44:22 +0530 Subject: Add Ctrl+Click support for tabs --- app/assets/javascripts/merge_request_tabs.js.es6 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/app/assets/javascripts/merge_request_tabs.js.es6 b/app/assets/javascripts/merge_request_tabs.js.es6 index 107e85f1225..10c4fdf1f4f 100644 --- a/app/assets/javascripts/merge_request_tabs.js.es6 +++ b/app/assets/javascripts/merge_request_tabs.js.es6 @@ -82,12 +82,18 @@ require('./flash'); $(document) .on('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown) .on('click', '.js-show-tab', this.showTab); + + $('.merge-request-tabs a[data-toggle="tab"]') + .on('click', this.clickTab); } unbindEvents() { $(document) .off('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown) .off('click', '.js-show-tab', this.showTab); + + $('.merge-request-tabs a[data-toggle="tab"]') + .off('click', this.clickTab); } showTab(e) { @@ -95,6 +101,14 @@ require('./flash'); this.activateTab($(e.target).data('action')); } + clickTab(e) { + const targetLink = $(e.target).attr('href'); + if (e.metaKey || e.ctrlKey || e.which === 2) { + e.stopImmediatePropagation(); + window.open(targetLink, '_blank'); + } + } + tabShown(e) { const $target = $(e.target); const action = $target.data('action'); -- cgit v1.2.1 From 00018892e5f19620afb61e8f5f0cb93ec4c04cde Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 31 Jan 2017 15:03:38 +0530 Subject: Add changelog entry for !8898 --- changelogs/unreleased/24716-fix-ctrl-click-links.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/24716-fix-ctrl-click-links.yml diff --git a/changelogs/unreleased/24716-fix-ctrl-click-links.yml b/changelogs/unreleased/24716-fix-ctrl-click-links.yml new file mode 100644 index 00000000000..13de5db5e41 --- /dev/null +++ b/changelogs/unreleased/24716-fix-ctrl-click-links.yml @@ -0,0 +1,4 @@ +--- +title: Fix Ctrl+Click support for Todos and Merge Request page tabs +merge_request: 8898 +author: -- cgit v1.2.1 From 8a4da74030d32cf911f9a293f6774fc12a5e7ae4 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Mon, 6 Feb 2017 15:36:57 +0530 Subject: Tests for clickTab with Mac and PC --- spec/javascripts/merge_request_tabs_spec.js | 36 +++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index d20a59df041..1a0fcd4e33e 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -61,6 +61,42 @@ require('vendor/jquery.scrollTo'); expect($('#diffs')).toHaveClass('active'); }); }); + describe('#opensInNewTab', function () { + it('opens page tab in a new browser tab with Ctrl+Click - Windows/Linux', function () { + const commitsLink = '.commits-tab li a'; + const tabUrl = $(commitsLink).attr('href'); + + spyOn($.fn, 'attr').and.returnValue(tabUrl); + spyOn(window, 'open').and.callFake(function (url, name) { + expect(url).toEqual(tabUrl); + expect(name).toEqual('_blank'); + }); + + this.class.clickTab({ + metaKey: false, + ctrlKey: true, + which: 1, + stopImmediatePropagation: function () {} + }); + }); + it('opens page tab in a new browser tab with Cmd+Click - Mac', function () { + const commitsLink = '.commits-tab li a'; + const tabUrl = $(commitsLink).attr('href'); + + spyOn($.fn, 'attr').and.returnValue(tabUrl); + spyOn(window, 'open').and.callFake(function (url, name) { + expect(url).toEqual(tabUrl); + expect(name).toEqual('_blank'); + }); + + this.class.clickTab({ + metaKey: true, + ctrlKey: false, + which: 1, + stopImmediatePropagation: function () {} + }); + }); + }); describe('#setCurrentAction', function () { beforeEach(function () { -- cgit v1.2.1 From f9c23de6de28eaf3a90877a3db9ddd0c5f634b0c Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 7 Feb 2017 11:20:55 +0530 Subject: Use plain JS in `goToTodoUrl`, make comment more concise --- app/assets/javascripts/todos.js.es6 | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/todos.js.es6 b/app/assets/javascripts/todos.js.es6 index bf84d82af97..5b1b585dfac 100644 --- a/app/assets/javascripts/todos.js.es6 +++ b/app/assets/javascripts/todos.js.es6 @@ -146,20 +146,21 @@ } goToTodoUrl(e) { - const todoLink = $(this).data('url'); - let targetLink = $(e.target).attr('href'); + const todoLink = this.dataset.url; + let targetLink = e.target.getAttribute('href'); - if ($(e.target).is('img')) { // See if clicked target was Avatar - targetLink = $(e.target).parent().attr('href'); // Parent of Avatar is link + if (e.target.tagName === 'IMG') { // See if clicked target was Avatar + targetLink = e.target.parentElement.getAttribute('href'); // Parent of Avatar is link } if (!todoLink) { return; } - // Allow Meta-Click (Cmd+Click or Ctrl+Click) - // or Mouse3-click (middle-click) - // to open in a new tab + // Allow following special clicks to make link open in new tab + // 1) Cmd + Click on Mac (e.metaKey) + // 2) Ctrl + Click on PC (e.ctrlKey) + // 3) Middle-click or Mouse Wheel Click (e.which is 2) if (e.metaKey || e.ctrlKey || e.which === 2) { e.preventDefault(); // Meta-Click on username leads to different URL than todoLink. -- cgit v1.2.1 From ef99b5e8969c939fce576a6d25c6b8dd4ce54f5f Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 7 Feb 2017 11:22:52 +0530 Subject: Use plain JS within `clickTab`, make comment more concise --- app/assets/javascripts/merge_request_tabs.js.es6 | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js.es6 b/app/assets/javascripts/merge_request_tabs.js.es6 index 10c4fdf1f4f..43ba94ada9b 100644 --- a/app/assets/javascripts/merge_request_tabs.js.es6 +++ b/app/assets/javascripts/merge_request_tabs.js.es6 @@ -102,10 +102,16 @@ require('./flash'); } clickTab(e) { - const targetLink = $(e.target).attr('href'); - if (e.metaKey || e.ctrlKey || e.which === 2) { - e.stopImmediatePropagation(); - window.open(targetLink, '_blank'); + if (e.target) { + const targetLink = e.target.getAttribute('href'); + // Allow following special clicks to make link open in new tab + // 1) Cmd + Click on Mac (e.metaKey) + // 2) Ctrl + Click on PC (e.ctrlKey) + // 3) Middle-click or Mouse Wheel Click (e.which is 2) + if (e.metaKey || e.ctrlKey || e.which === 2) { + e.stopImmediatePropagation(); + window.open(targetLink, '_blank'); + } } } -- cgit v1.2.1 From 049ef298e9ad3e3054446870326369f40f2ab164 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 7 Feb 2017 11:34:42 +0530 Subject: Update param name, add test for `e.which = 2` --- spec/javascripts/merge_request_tabs_spec.js | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index 1a0fcd4e33e..07f9797c229 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -84,9 +84,9 @@ require('vendor/jquery.scrollTo'); const tabUrl = $(commitsLink).attr('href'); spyOn($.fn, 'attr').and.returnValue(tabUrl); - spyOn(window, 'open').and.callFake(function (url, name) { + spyOn(window, 'open').and.callFake(function (url, target) { expect(url).toEqual(tabUrl); - expect(name).toEqual('_blank'); + expect(target).toEqual('_blank'); }); this.class.clickTab({ @@ -96,6 +96,23 @@ require('vendor/jquery.scrollTo'); stopImmediatePropagation: function () {} }); }); + it('opens page tab in a new browser tab with Middle-click - Mac/PC', function () { + const commitsLink = '.commits-tab li a'; + const tabUrl = $(commitsLink).attr('href'); + + spyOn($.fn, 'attr').and.returnValue(tabUrl); + spyOn(window, 'open').and.callFake(function (url, target) { + expect(url).toEqual(tabUrl); + expect(target).toEqual('_blank'); + }); + + this.class.clickTab({ + metaKey: false, + ctrlKey: false, + which: 2, + stopImmediatePropagation: function () {} + }); + }); }); describe('#setCurrentAction', function () { -- cgit v1.2.1 From 36431288a2349878964f07a21a7b6b99dfbd2dcb Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 7 Feb 2017 21:38:54 +0530 Subject: Add utility `isMetaClick` to identify cross-platform meta-click --- app/assets/javascripts/lib/utils/common_utils.js.es6 | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/assets/javascripts/lib/utils/common_utils.js.es6 b/app/assets/javascripts/lib/utils/common_utils.js.es6 index 0ee29a75c62..15d89cffe83 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js.es6 +++ b/app/assets/javascripts/lib/utils/common_utils.js.es6 @@ -134,6 +134,14 @@ return e.metaKey || e.ctrlKey || e.altKey || e.shiftKey; }; + gl.utils.isMetaClick = function(e) { + // Identify following special clicks + // 1) Cmd + Click on Mac (e.metaKey) + // 2) Ctrl + Click on PC (e.ctrlKey) + // 3) Middle-click or Mouse Wheel Click (e.which is 2) + return e.metaKey || e.ctrlKey || e.which === 2; + }; + gl.utils.scrollToElement = function($el) { var top = $el.offset().top; gl.navBarHeight = gl.navBarHeight || $('.navbar-gitlab').height(); -- cgit v1.2.1 From 936f463bafcbe846f231f14ae29db20a920b1ca8 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 7 Feb 2017 21:39:40 +0530 Subject: Add tests for `gl.utils.isMetaClick` --- .../javascripts/lib/utils/common_utils_spec.js.es6 | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/spec/javascripts/lib/utils/common_utils_spec.js.es6 b/spec/javascripts/lib/utils/common_utils_spec.js.es6 index fbb06f3948b..d10403cfb4e 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js.es6 +++ b/spec/javascripts/lib/utils/common_utils_spec.js.es6 @@ -73,5 +73,37 @@ require('~/lib/utils/common_utils'); expect(normalized[NGINX].nginx).toBe('ok'); }); }); + + describe('gl.utils.isMetaClick', () => { + it('should identify meta click on Windows/Linux', () => { + const e = { + metaKey: false, + ctrlKey: true, + which: 1, + }; + + expect(gl.utils.isMetaClick(e)).toBe(true); + }); + + it('should identify meta click on macOS', () => { + const e = { + metaKey: true, + ctrlKey: false, + which: 1, + }; + + expect(gl.utils.isMetaClick(e)).toBe(true); + }); + + it('should identify as meta click on middle-click or Mouse-wheel click', () => { + const e = { + metaKey: false, + ctrlKey: false, + which: 2, + }; + + expect(gl.utils.isMetaClick(e)).toBe(true); + }); + }); }); })(); -- cgit v1.2.1 From a11b23046fc95fa30c6eb3d3cdb2fb994e1fbe92 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 7 Feb 2017 21:40:14 +0530 Subject: Use `gl.utils.isMetaClick` to identify meta-clicks --- app/assets/javascripts/merge_request_tabs.js.es6 | 6 +----- app/assets/javascripts/todos.js.es6 | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js.es6 b/app/assets/javascripts/merge_request_tabs.js.es6 index 43ba94ada9b..186cb3e9190 100644 --- a/app/assets/javascripts/merge_request_tabs.js.es6 +++ b/app/assets/javascripts/merge_request_tabs.js.es6 @@ -104,11 +104,7 @@ require('./flash'); clickTab(e) { if (e.target) { const targetLink = e.target.getAttribute('href'); - // Allow following special clicks to make link open in new tab - // 1) Cmd + Click on Mac (e.metaKey) - // 2) Ctrl + Click on PC (e.ctrlKey) - // 3) Middle-click or Mouse Wheel Click (e.which is 2) - if (e.metaKey || e.ctrlKey || e.which === 2) { + if (gl.utils.isMetaClick(e)) { e.stopImmediatePropagation(); window.open(targetLink, '_blank'); } diff --git a/app/assets/javascripts/todos.js.es6 b/app/assets/javascripts/todos.js.es6 index 5b1b585dfac..b07e62a8c30 100644 --- a/app/assets/javascripts/todos.js.es6 +++ b/app/assets/javascripts/todos.js.es6 @@ -157,11 +157,7 @@ return; } - // Allow following special clicks to make link open in new tab - // 1) Cmd + Click on Mac (e.metaKey) - // 2) Ctrl + Click on PC (e.ctrlKey) - // 3) Middle-click or Mouse Wheel Click (e.which is 2) - if (e.metaKey || e.ctrlKey || e.which === 2) { + if (gl.utils.isMetaClick(e)) { e.preventDefault(); // Meta-Click on username leads to different URL than todoLink. // Turbolinks can resolve that URL, but window.open requires URL manually. -- cgit v1.2.1 From 3633e04fcdc65ad8d15f47972c920c29d376362f Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 7 Feb 2017 21:40:31 +0530 Subject: Refactor tests for `#opensInNewTab` --- spec/javascripts/merge_request_tabs_spec.js | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index 07f9797c229..72a05ce882c 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -62,7 +62,7 @@ require('vendor/jquery.scrollTo'); }); }); describe('#opensInNewTab', function () { - it('opens page tab in a new browser tab with Ctrl+Click - Windows/Linux', function () { + beforeEach(function () { const commitsLink = '.commits-tab li a'; const tabUrl = $(commitsLink).attr('href'); @@ -71,7 +71,8 @@ require('vendor/jquery.scrollTo'); expect(url).toEqual(tabUrl); expect(name).toEqual('_blank'); }); - + }); + it('opens page tab in a new browser tab with Ctrl+Click - Windows/Linux', function () { this.class.clickTab({ metaKey: false, ctrlKey: true, @@ -80,15 +81,6 @@ require('vendor/jquery.scrollTo'); }); }); it('opens page tab in a new browser tab with Cmd+Click - Mac', function () { - const commitsLink = '.commits-tab li a'; - const tabUrl = $(commitsLink).attr('href'); - - spyOn($.fn, 'attr').and.returnValue(tabUrl); - spyOn(window, 'open').and.callFake(function (url, target) { - expect(url).toEqual(tabUrl); - expect(target).toEqual('_blank'); - }); - this.class.clickTab({ metaKey: true, ctrlKey: false, @@ -97,15 +89,6 @@ require('vendor/jquery.scrollTo'); }); }); it('opens page tab in a new browser tab with Middle-click - Mac/PC', function () { - const commitsLink = '.commits-tab li a'; - const tabUrl = $(commitsLink).attr('href'); - - spyOn($.fn, 'attr').and.returnValue(tabUrl); - spyOn(window, 'open').and.callFake(function (url, target) { - expect(url).toEqual(tabUrl); - expect(target).toEqual('_blank'); - }); - this.class.clickTab({ metaKey: false, ctrlKey: false, -- cgit v1.2.1 From 1278b5cea4b05eb434649915a9d575c7ec3c95a2 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 7 Feb 2017 22:15:27 +0530 Subject: ESLint: remove `expect()` from `beforeEach()` --- spec/javascripts/merge_request_tabs_spec.js | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index 72a05ce882c..92a0f1c05f7 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -62,17 +62,21 @@ require('vendor/jquery.scrollTo'); }); }); describe('#opensInNewTab', function () { + var commitsLink; + var tabUrl; + beforeEach(function () { - const commitsLink = '.commits-tab li a'; - const tabUrl = $(commitsLink).attr('href'); + commitsLink = '.commits-tab li a'; + tabUrl = $(commitsLink).attr('href'); spyOn($.fn, 'attr').and.returnValue(tabUrl); + }); + it('opens page tab in a new browser tab with Ctrl+Click - Windows/Linux', function () { spyOn(window, 'open').and.callFake(function (url, name) { expect(url).toEqual(tabUrl); expect(name).toEqual('_blank'); }); - }); - it('opens page tab in a new browser tab with Ctrl+Click - Windows/Linux', function () { + this.class.clickTab({ metaKey: false, ctrlKey: true, @@ -81,6 +85,11 @@ require('vendor/jquery.scrollTo'); }); }); it('opens page tab in a new browser tab with Cmd+Click - Mac', function () { + spyOn(window, 'open').and.callFake(function (url, name) { + expect(url).toEqual(tabUrl); + expect(name).toEqual('_blank'); + }); + this.class.clickTab({ metaKey: true, ctrlKey: false, @@ -89,6 +98,11 @@ require('vendor/jquery.scrollTo'); }); }); it('opens page tab in a new browser tab with Middle-click - Mac/PC', function () { + spyOn(window, 'open').and.callFake(function (url, name) { + expect(url).toEqual(tabUrl); + expect(name).toEqual('_blank'); + }); + this.class.clickTab({ metaKey: false, ctrlKey: false, -- cgit v1.2.1 From 82a05423e6c246dc465eb7019e310f4f8a96edf9 Mon Sep 17 00:00:00 2001 From: Kushal Pandya Date: Tue, 7 Feb 2017 22:16:10 +0530 Subject: Simplify conditions --- app/assets/javascripts/merge_request_tabs.js.es6 | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js.es6 b/app/assets/javascripts/merge_request_tabs.js.es6 index 186cb3e9190..af1ba9ecaf3 100644 --- a/app/assets/javascripts/merge_request_tabs.js.es6 +++ b/app/assets/javascripts/merge_request_tabs.js.es6 @@ -102,12 +102,10 @@ require('./flash'); } clickTab(e) { - if (e.target) { + if (e.target && gl.utils.isMetaClick(e)) { const targetLink = e.target.getAttribute('href'); - if (gl.utils.isMetaClick(e)) { - e.stopImmediatePropagation(); - window.open(targetLink, '_blank'); - } + e.stopImmediatePropagation(); + window.open(targetLink, '_blank'); } } -- cgit v1.2.1