From dfe8dcc7f008665a487eb1083344d71c9b9b030f Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 21 Sep 2015 14:01:56 -0700 Subject: [PATCH 01/15] [chrome] indicate which app is active in the switcher --- src/ui/public/chrome/api/nav.js | 4 ++++ src/ui/public/chrome/appSwitcher/appSwitcher.html | 2 +- src/ui/public/chrome/appSwitcher/appSwitcher.less | 14 ++++++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/ui/public/chrome/api/nav.js b/src/ui/public/chrome/api/nav.js index 0d47ba77fdfc..d82522662458 100644 --- a/src/ui/public/chrome/api/nav.js +++ b/src/ui/public/chrome/api/nav.js @@ -25,6 +25,10 @@ module.exports = function (chrome, internals) { a.setAttribute('href', link.url); link.url = a.href; link.lastSubUrl = chrome.getLastSubUrlFor(link.url); + + if (link.url === chrome.getAppUrl()) { + link.active = true; + } }); }; diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.html b/src/ui/public/chrome/appSwitcher/appSwitcher.html index 19c0aa1c42d4..e5df0ba69502 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.html +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.html @@ -1,5 +1,5 @@
-
+
diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.less b/src/ui/public/chrome/appSwitcher/appSwitcher.less index 8b888de07e4e..95ac50baa310 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.less +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.less @@ -1,6 +1,7 @@ @import (reference) "~ui/styles/variables"; @app-icon-size: 48px; +@app-icon-padding: 10px; .app-links { text-align: justify; @@ -8,9 +9,11 @@ .app-link { display: inline-block; vertical-align: top; - width: @app-icon-size; - margin: 0px 10px; text-align: left; + width: @app-icon-size + (@app-icon-padding * 2); + margin: 0px 10px; + padding: @app-icon-padding; + border-radius: @border-radius-base; .app-icon { display: block; @@ -43,6 +46,13 @@ text-decoration: underline; } + &.active { + background: @gray-lighter; + .app-title { + text-decoration: underline; + } + } + } } From 4d20063df3f35553c132894c4af469a6c7b07d69 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 21 Sep 2015 14:37:53 -0700 Subject: [PATCH 02/15] [chrome/appSwitcher] when navigating to an app, ensure the page reloads --- src/ui/public/chrome/api/angular.js | 2 +- .../chrome/appSwitcher/appSwitcher.html | 12 +++++-- .../public/chrome/appSwitcher/appSwitcher.js | 33 +++++++++++++++++++ 3 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 src/ui/public/chrome/appSwitcher/appSwitcher.js diff --git a/src/ui/public/chrome/api/angular.js b/src/ui/public/chrome/api/angular.js index 4045776f0c32..4635dd5c588e 100644 --- a/src/ui/public/chrome/api/angular.js +++ b/src/ui/public/chrome/api/angular.js @@ -1,7 +1,7 @@ var $ = require('jquery'); var _ = require('lodash'); -require('../appSwitcher/appSwitcher.less'); +require('../appSwitcher'); var modules = require('ui/modules'); var ConfigTemplate = require('ui/ConfigTemplate'); require('ui/directives/config'); diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.html b/src/ui/public/chrome/appSwitcher/appSwitcher.html index e5df0ba69502..bada78ee7550 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.html +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.html @@ -1,6 +1,12 @@ -
-
- +
+
+ +
{{app.title[0]}}
diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.js b/src/ui/public/chrome/appSwitcher/appSwitcher.js new file mode 100644 index 000000000000..f649a7fb6a89 --- /dev/null +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.js @@ -0,0 +1,33 @@ +var parse = require('url').parse; + +require('../appSwitcher/appSwitcher.less'); + +require('ui/modules') +.get('kibana') +.directive('appSwitcher', function () { + return { + restrict: 'A', + controllerAs: 'switcher', + controller: function () { + + // links don't cause full-navigation events in certain scenarios + // so we force them when needed + this.ensureNavigation = function (event, app) { + if (event.isDefaultPrevented() || event.altKey || event.metaKey || event.ctrlKey) { + return; + } + + var toParsed = parse(app.url); + var fromParsed = parse(window.location.href); + var sameProto = toParsed.protocol === fromParsed.protocol; + var sameHost = toParsed.host === fromParsed.host; + var samePath = toParsed.path === fromParsed.path; + + if (sameProto && sameHost && samePath) { + window.location.reload(true); + } + }; + + } + }; +}); From 16165d6c5bbb2f9c9775b6658fb7b384a7f2fa18 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 15:32:26 -0700 Subject: [PATCH 03/15] [appSwitcher] move app switcher template into directive --- src/ui/public/chrome/api/angular.js | 2 +- .../public/chrome/appSwitcher/appSwitcher.html | 17 +++++++++-------- src/ui/public/chrome/appSwitcher/appSwitcher.js | 3 ++- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/ui/public/chrome/api/angular.js b/src/ui/public/chrome/api/angular.js index 4635dd5c588e..91c35fb591c3 100644 --- a/src/ui/public/chrome/api/angular.js +++ b/src/ui/public/chrome/api/angular.js @@ -63,7 +63,7 @@ module.exports = function (chrome, internals) { $scope.httpActive = $http.pendingRequests; $scope.notifList = require('ui/notify')._notifs; $scope.appSwitcherTemplate = new ConfigTemplate({ - switcher: require('../appSwitcher/appSwitcher.html') + switcher: '' }); return chrome; diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.html b/src/ui/public/chrome/appSwitcher/appSwitcher.html index bada78ee7550..7613cb03142b 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.html +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.html @@ -1,17 +1,18 @@ -
+
+ ng-repeat="link in switcher.getNavLinks() | orderBy:'title'" + ng-class="{ active: link.active }"> + ng-click="switcher.ensureNavigation($event, link)" + ng-href="{{ link.active ? link.url : (link.lastSubUrl || link.url) }}" + data-test-subj="appLink"> -
-
{{app.title[0]}}
+
+
{{ link.title[0] }}
-
{{ app.title }}
+
{{ link.title }}
diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.js b/src/ui/public/chrome/appSwitcher/appSwitcher.js index f649a7fb6a89..7ba5b29488b1 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.js @@ -6,7 +6,8 @@ require('ui/modules') .get('kibana') .directive('appSwitcher', function () { return { - restrict: 'A', + restrict: 'E', + template: require('./appSwitcher.html'), controllerAs: 'switcher', controller: function () { From 344aed432d82933956695ffa7a96f821d438716c Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 15:33:45 -0700 Subject: [PATCH 04/15] [appSwitcher] check for chrome on $scope --- src/ui/public/chrome/appSwitcher/appSwitcher.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.js b/src/ui/public/chrome/appSwitcher/appSwitcher.js index 7ba5b29488b1..55ce82fe5b09 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.js @@ -1,4 +1,5 @@ var parse = require('url').parse; +var bindKey = require('lodash').bindKey; require('../appSwitcher/appSwitcher.less'); @@ -9,7 +10,15 @@ require('ui/modules') restrict: 'E', template: require('./appSwitcher.html'), controllerAs: 'switcher', - controller: function () { + controller: function ($scope, $window) { + + // since we render this in an isolate scope we can't "require: ^chrome", but + // rather than remove all helpfull checks we can just check here. + if (!$scope.chrome || !$scope.chrome.getNavLinks) { + throw new TypeError('appSwitcher directive requires the "chrome" config-object'); + } + + this.getNavLinks = bindKey($scope.chrome, 'getNavLinks'); // links don't cause full-navigation events in certain scenarios // so we force them when needed From 10f5f1a45aacd74c758cf3a5196db99abfbc8533 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 15:34:44 -0700 Subject: [PATCH 05/15] [appSwitcher] read/write to $window for test mocking --- src/ui/public/chrome/appSwitcher/appSwitcher.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.js b/src/ui/public/chrome/appSwitcher/appSwitcher.js index 55ce82fe5b09..af809ed12cef 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.js @@ -28,13 +28,14 @@ require('ui/modules') } var toParsed = parse(app.url); - var fromParsed = parse(window.location.href); + var fromParsed = parse($window.location.href); var sameProto = toParsed.protocol === fromParsed.protocol; var sameHost = toParsed.host === fromParsed.host; var samePath = toParsed.path === fromParsed.path; if (sameProto && sameHost && samePath) { - window.location.reload(true); + $window.location.reload(); + event.preventDefault(); } }; From 79968f9e31d93064c7df3c4274fe4fa466d08161 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 15:39:06 -0700 Subject: [PATCH 06/15] [jquery] add findTestSubject helper to jquery --- src/ui/public/jquery/findTestSubject.js | 19 +++++++++++++++++++ webpackShims/jquery.js | 3 ++- 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 src/ui/public/jquery/findTestSubject.js diff --git a/src/ui/public/jquery/findTestSubject.js b/src/ui/public/jquery/findTestSubject.js new file mode 100644 index 000000000000..18c33349814f --- /dev/null +++ b/src/ui/public/jquery/findTestSubject.js @@ -0,0 +1,19 @@ +module.exports = function bindToJquery($) { + + $.fn.findTestSubject = function (/* ...subjectSelectors */) { + var subjectSelectors = [].slice.apply(arguments); + var $els = $(); + var $context = this; + + subjectSelectors.forEach(function (subjects) { + var selector = subjects.split(/\s+/).map(function (subject) { + return '[data-test-subj~="' + subject + '"]'; + }).join(' '); + + $els = $els.add($context.find(selector)); + }); + + return $els; + }; + +}; diff --git a/webpackShims/jquery.js b/webpackShims/jquery.js index 4cef6e2479f3..e9ac7b7cbfda 100644 --- a/webpackShims/jquery.js +++ b/webpackShims/jquery.js @@ -1 +1,2 @@ -window.jQuery = window.$ = module.exports = require('node_modules/jquery/dist/jquery'); +var $ = window.jQuery = window.$ = module.exports = require('node_modules/jquery/dist/jquery'); +require('ui/jquery/findTestSubject')($); From 4464cfd2e49ce0ae44221ef43abfb4f5f80aa577 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 15:46:21 -0700 Subject: [PATCH 07/15] [appSwitcher] added tests --- .../appSwitcher/__tests__/appSwitcher.js | 198 ++++++++++++++++++ 1 file changed, 198 insertions(+) create mode 100644 src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js diff --git a/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js b/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js new file mode 100644 index 000000000000..ab665674861f --- /dev/null +++ b/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js @@ -0,0 +1,198 @@ +var sinon = require('auto-release-sinon'); +var ngMock = require('ngMock'); +var $ = require('jquery'); +var expect = require('expect.js'); +var constant = require('lodash').constant; +var set = require('lodash').set; +var cloneDeep = require('lodash').cloneDeep; +var indexBy = require('lodash').indexBy; + +require('ui/chrome'); +require('ui/chrome/appSwitcher'); + +describe('appSwitcher directive', function () { + var env; + + beforeEach(ngMock.module('kibana', function ($provide) { + $provide.decorator('$window', function ($delegate) { + return Object.create($delegate, set({}, 'location.value', Object.create($delegate.location))); + }); + })); + + function setup(href, links) { + return ngMock.inject(function ($window, $rootScope, $compile) { + $rootScope.chrome = { + getNavLinks: constant(cloneDeep(links)), + }; + + env = { + $scope: $rootScope, + $el: $compile($(''))($rootScope), + $links: $rootScope.chrome.getNavLinks(), + currentHref: href, + location: $window.location + }; + + Object.defineProperties($window.location, { + href: { + get: function () { return env.currentHref; }, + set: function (val) { return env.currentHref = val; }, + }, + reload: { + value: sinon.stub() + } + }); + + env.$scope.$digest(); + }); + } + + context('when one links is for the active app', function () { + var myLink = { + active: true, + title: 'myLink', + url: 'http://localhost:555/app/myApp', + lastSubUrl: 'http://localhost:555/app/myApp#/lastSubUrl' + }; + + var notMyLink = { + active: false, + title: 'notMyLink', + url: 'http://localhost:555/app/notMyApp', + lastSubUrl: 'http://localhost:555/app/notMyApp#/lastSubUrl' + }; + + beforeEach(setup('http://localhost:5555/app/myApp/', [myLink, notMyLink])); + + it('links to the active apps base url', function () { + var $myLink = env.$el.findTestSubject('appLink').eq(0); + expect($myLink.prop('href')).to.be(myLink.url); + expect($myLink.prop('href')).to.not.be(myLink.lastSubUrl); + }); + + it('links to the inactive apps last sub url', function () { + var $notMyLink = env.$el.findTestSubject('appLink').eq(1); + expect($notMyLink.prop('href')).to.be(notMyLink.lastSubUrl); + expect($notMyLink.prop('href')).to.not.be(notMyLink.url); + }); + }); + + context('when none of the links are for the active app', function () { + var myLink = { + active: false, + title: 'myLink', + url: 'http://localhost:555/app/myApp', + lastSubUrl: 'http://localhost:555/app/myApp#/lastSubUrl' + }; + + var notMyLink = { + active: false, + title: 'notMyLink', + url: 'http://localhost:555/app/notMyApp', + lastSubUrl: 'http://localhost:555/app/notMyApp#/lastSubUrl' + }; + + beforeEach(setup('http://localhost:5555/app/myApp/', [myLink, notMyLink])); + + it('links to the lastSubUrl for each', function () { + var $links = env.$el.findTestSubject('appLink'); + var $myLink = $links.eq(0); + var $notMyLink = $links.eq(1); + + expect($myLink.prop('href')).to.be(myLink.lastSubUrl); + expect($myLink.prop('href')).to.not.be(myLink.url); + + expect($notMyLink.prop('href')).to.be(notMyLink.lastSubUrl); + expect($notMyLink.prop('href')).to.not.be(notMyLink.url); + }); + }); + + context('clicking a link that matches entire url', function () { + var url = 'http://localhost:555/app/myApp#/lastSubUrl'; + beforeEach(setup(url, [ + { url: url } + ])); + + it('calls window.location.reload and prevents default behavior', function () { + var event = new $.Event('click'); + + expect(env.location.reload.callCount).to.be(0); + expect(event.isDefaultPrevented()).to.be(false); + expect(event.isPropagationStopped()).to.be(false); + + var $link = env.$el.findTestSubject('appLink'); + expect($link.prop('href')).to.be(env.currentHref); + $link.trigger(event); + + expect(event.isPropagationStopped()).to.be(false); + expect(event.isDefaultPrevented()).to.be(true); + expect(env.location.reload.callCount).to.be(1); + }); + }); + + context('clicking a link with matching href except different hash', function () { + var url = 'http://localhost:555/app/myApp?query=1'; + beforeEach(setup(url + '#/lastSubUrl', [ + { url: url } + ])); + + it('calls window.location.reload and prevents default behavior', function () { + var event = new $.Event('click'); + + expect(env.location.reload.callCount).to.be(0); + expect(event.isDefaultPrevented()).to.be(false); + expect(event.isPropagationStopped()).to.be(false); + + var $link = env.$el.findTestSubject('appLink'); + expect($link.prop('href')).to.be(url); + $link.trigger(event); + + expect(event.isPropagationStopped()).to.be(false); + expect(event.isDefaultPrevented()).to.be(true); + expect(env.location.reload.callCount).to.be(1); + }); + }); + + context('clicking a link with matching host', function () { + beforeEach(setup('http://localhost:555/someOtherPath', [ + { + active: true, + url: 'http://localhost:555/app/myApp' + } + ])); + + it('allows click through', function () { + var event = new $.Event('click'); + + expect(env.location.reload.callCount).to.be(0); + expect(event.isPropagationStopped()).to.be(false); + + env.$el.findTestSubject('appLink').trigger(event); + + expect(env.location.reload.callCount).to.be(0); + expect(event.isPropagationStopped()).to.be(false); + }); + }); + + context('clicking a link with matching host and path', function () { + beforeEach(setup('http://localhost:555/app/myApp?someQuery=true', [ + { + active: true, + url: 'http://localhost:555/app/myApp?differentQuery=true' + } + ])); + + it('allows click through', function () { + var event = new $.Event('click'); + + expect(env.location.reload.callCount).to.be(0); + expect(event.isPropagationStopped()).to.be(false); + + env.$el.findTestSubject('appLink').trigger(event); + + expect(env.location.reload.callCount).to.be(0); + expect(event.isPropagationStopped()).to.be(false); + }); + }); + +}); From d47e84c43df76ea00e5c4f916217639db4e32916 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 15:55:55 -0700 Subject: [PATCH 08/15] [appSwitcher] added tests --- .../appSwitcher/__tests__/appSwitcher.js | 30 ++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js b/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js index ab665674861f..286f931eda80 100644 --- a/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js @@ -10,6 +10,22 @@ var indexBy = require('lodash').indexBy; require('ui/chrome'); require('ui/chrome/appSwitcher'); +function findTestSubject() { + var subjectSelectors = [].slice.apply(arguments); + var $context = subjectSelectors.shift(); + var $els = $(); + + subjectSelectors.forEach(function (subjects) { + var selector = subjects.split(/\s+/).map(function (subject) { + return '[data-test-subj~="' + subject + '"]'; + }).join(' '); + + $els = $els.add($context.find(selector)); + }); + + return $els; +} + describe('appSwitcher directive', function () { var env; @@ -65,13 +81,13 @@ describe('appSwitcher directive', function () { beforeEach(setup('http://localhost:5555/app/myApp/', [myLink, notMyLink])); it('links to the active apps base url', function () { - var $myLink = env.$el.findTestSubject('appLink').eq(0); + var $myLink = findTestSubject(env.$el, 'appLink').eq(0); expect($myLink.prop('href')).to.be(myLink.url); expect($myLink.prop('href')).to.not.be(myLink.lastSubUrl); }); it('links to the inactive apps last sub url', function () { - var $notMyLink = env.$el.findTestSubject('appLink').eq(1); + var $notMyLink = findTestSubject(env.$el, 'appLink').eq(1); expect($notMyLink.prop('href')).to.be(notMyLink.lastSubUrl); expect($notMyLink.prop('href')).to.not.be(notMyLink.url); }); @@ -95,7 +111,7 @@ describe('appSwitcher directive', function () { beforeEach(setup('http://localhost:5555/app/myApp/', [myLink, notMyLink])); it('links to the lastSubUrl for each', function () { - var $links = env.$el.findTestSubject('appLink'); + var $links = findTestSubject(env.$el, 'appLink'); var $myLink = $links.eq(0); var $notMyLink = $links.eq(1); @@ -120,7 +136,7 @@ describe('appSwitcher directive', function () { expect(event.isDefaultPrevented()).to.be(false); expect(event.isPropagationStopped()).to.be(false); - var $link = env.$el.findTestSubject('appLink'); + var $link = findTestSubject(env.$el, 'appLink'); expect($link.prop('href')).to.be(env.currentHref); $link.trigger(event); @@ -143,7 +159,7 @@ describe('appSwitcher directive', function () { expect(event.isDefaultPrevented()).to.be(false); expect(event.isPropagationStopped()).to.be(false); - var $link = env.$el.findTestSubject('appLink'); + var $link = findTestSubject(env.$el, 'appLink'); expect($link.prop('href')).to.be(url); $link.trigger(event); @@ -167,7 +183,7 @@ describe('appSwitcher directive', function () { expect(env.location.reload.callCount).to.be(0); expect(event.isPropagationStopped()).to.be(false); - env.$el.findTestSubject('appLink').trigger(event); + findTestSubject(env.$el, 'appLink').trigger(event); expect(env.location.reload.callCount).to.be(0); expect(event.isPropagationStopped()).to.be(false); @@ -188,7 +204,7 @@ describe('appSwitcher directive', function () { expect(env.location.reload.callCount).to.be(0); expect(event.isPropagationStopped()).to.be(false); - env.$el.findTestSubject('appLink').trigger(event); + findTestSubject(env.$el, 'appLink').trigger(event); expect(env.location.reload.callCount).to.be(0); expect(event.isPropagationStopped()).to.be(false); From a1c7e7fe240c09c103b1b0b0bff98b2331d960c5 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 17:37:35 -0700 Subject: [PATCH 09/15] [appSwitcher] use a mockable window.location service --- src/ui/public/chrome/appSwitcher/appSwitcher.js | 8 +++++--- src/ui/public/domLocation.js | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) create mode 100644 src/ui/public/domLocation.js diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.js b/src/ui/public/chrome/appSwitcher/appSwitcher.js index af809ed12cef..c84573960a27 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.js @@ -2,6 +2,7 @@ var parse = require('url').parse; var bindKey = require('lodash').bindKey; require('../appSwitcher/appSwitcher.less'); +var DomLocationProvider = require('ui/domLocation'); require('ui/modules') .get('kibana') @@ -10,7 +11,8 @@ require('ui/modules') restrict: 'E', template: require('./appSwitcher.html'), controllerAs: 'switcher', - controller: function ($scope, $window) { + controller: function ($scope, Private) { + var domLocation = Private(DomLocationProvider); // since we render this in an isolate scope we can't "require: ^chrome", but // rather than remove all helpfull checks we can just check here. @@ -28,13 +30,13 @@ require('ui/modules') } var toParsed = parse(app.url); - var fromParsed = parse($window.location.href); + var fromParsed = parse(domLocation.href); var sameProto = toParsed.protocol === fromParsed.protocol; var sameHost = toParsed.host === fromParsed.host; var samePath = toParsed.path === fromParsed.path; if (sameProto && sameHost && samePath) { - $window.location.reload(); + domLocation.reload(); event.preventDefault(); } }; diff --git a/src/ui/public/domLocation.js b/src/ui/public/domLocation.js new file mode 100644 index 000000000000..f17926fd10a1 --- /dev/null +++ b/src/ui/public/domLocation.js @@ -0,0 +1,15 @@ +module.exports = function DomLocationProvider($window) { + return { + reload: function (forceFetch) { + $window.location.reload(forceFetch); + }, + + get href() { + return $window.location.href; + }, + + set href(val) { + return ($window.location.href = val); + } + }; +}; From a3fdda4b745acbe51c61d486e3f5b9e442318e12 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 17:38:08 -0700 Subject: [PATCH 10/15] [appSwitcher/tests] mock domLocation rather than trying to mod globals --- .../appSwitcher/__tests__/appSwitcher.js | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js b/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js index 286f931eda80..89d21f6dd9c3 100644 --- a/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js @@ -9,6 +9,7 @@ var indexBy = require('lodash').indexBy; require('ui/chrome'); require('ui/chrome/appSwitcher'); +var DomLocationProvider = require('ui/domLocation'); function findTestSubject() { var subjectSelectors = [].slice.apply(arguments); @@ -29,14 +30,12 @@ function findTestSubject() { describe('appSwitcher directive', function () { var env; - beforeEach(ngMock.module('kibana', function ($provide) { - $provide.decorator('$window', function ($delegate) { - return Object.create($delegate, set({}, 'location.value', Object.create($delegate.location))); - }); - })); + beforeEach(ngMock.module('kibana')); function setup(href, links) { - return ngMock.inject(function ($window, $rootScope, $compile) { + return ngMock.inject(function ($window, $rootScope, $compile, Private) { + var domLocation = Private(DomLocationProvider); + $rootScope.chrome = { getNavLinks: constant(cloneDeep(links)), }; @@ -44,12 +43,11 @@ describe('appSwitcher directive', function () { env = { $scope: $rootScope, $el: $compile($(''))($rootScope), - $links: $rootScope.chrome.getNavLinks(), currentHref: href, - location: $window.location + location: domLocation }; - Object.defineProperties($window.location, { + Object.defineProperties(domLocation, { href: { get: function () { return env.currentHref; }, set: function (val) { return env.currentHref = val; }, @@ -63,7 +61,7 @@ describe('appSwitcher directive', function () { }); } - context('when one links is for the active app', function () { + context('when one link is for the active app', function () { var myLink = { active: true, title: 'myLink', @@ -80,7 +78,7 @@ describe('appSwitcher directive', function () { beforeEach(setup('http://localhost:5555/app/myApp/', [myLink, notMyLink])); - it('links to the active apps base url', function () { + it('links to the inactive apps base url', function () { var $myLink = findTestSubject(env.$el, 'appLink').eq(0); expect($myLink.prop('href')).to.be(myLink.url); expect($myLink.prop('href')).to.not.be(myLink.lastSubUrl); From 72a610078ff4a47bbdaf4feaf4a218af6d28af07 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 18:47:01 -0700 Subject: [PATCH 11/15] [appSwitcher] compare to the href of the clicked element, not necessarily the apps url --- src/ui/public/chrome/appSwitcher/appSwitcher.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.js b/src/ui/public/chrome/appSwitcher/appSwitcher.js index c84573960a27..f70ea9f8d91b 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.js @@ -29,7 +29,7 @@ require('ui/modules') return; } - var toParsed = parse(app.url); + var toParsed = parse(event.delegateTarget.href); var fromParsed = parse(domLocation.href); var sameProto = toParsed.protocol === fromParsed.protocol; var sameHost = toParsed.host === fromParsed.host; From 9214b255df2d5e26332938ffeff49029d251e259 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 18:50:37 -0700 Subject: [PATCH 12/15] [appSwitcher] use event.stopPropagation() --- src/ui/public/chrome/appSwitcher/appSwitcher.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.js b/src/ui/public/chrome/appSwitcher/appSwitcher.js index f70ea9f8d91b..d4d80b40e684 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.js @@ -37,7 +37,12 @@ require('ui/modules') if (sameProto && sameHost && samePath) { domLocation.reload(); - event.preventDefault(); + + // event.preventDefault() keeps the browser from seeing the new url as an update + // and even setting window.location does not mimic that behavior, so instead + // we use stopPropagation() to prevent angular from seeing the click and + // starting a digest cycle/attempting to handle it in the router. + event.stopPropagation(); } }; From e714193175e4b0964fa672af8a7629a33d485ceb Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 18:51:05 -0700 Subject: [PATCH 13/15] [appSwitcher] only refresh when the new url uses a hash --- src/ui/public/chrome/appSwitcher/appSwitcher.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ui/public/chrome/appSwitcher/appSwitcher.js b/src/ui/public/chrome/appSwitcher/appSwitcher.js index d4d80b40e684..61d2d6311381 100644 --- a/src/ui/public/chrome/appSwitcher/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/appSwitcher.js @@ -36,7 +36,7 @@ require('ui/modules') var samePath = toParsed.path === fromParsed.path; if (sameProto && sameHost && samePath) { - domLocation.reload(); + toParsed.hash && domLocation.reload(); // event.preventDefault() keeps the browser from seeing the new url as an update // and even setting window.location does not mimic that behavior, so instead From 96a34069c46c623d5cca2ba5c630d212f2e148c5 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 22 Sep 2015 18:57:22 -0700 Subject: [PATCH 14/15] [appSwitcher/tests] update to match propagation based fixes --- .../appSwitcher/__tests__/appSwitcher.js | 77 ++++++++++++------- 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js b/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js index 89d21f6dd9c3..3135f6bcb519 100644 --- a/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js +++ b/src/ui/public/chrome/appSwitcher/__tests__/appSwitcher.js @@ -121,36 +121,13 @@ describe('appSwitcher directive', function () { }); }); - context('clicking a link that matches entire url', function () { - var url = 'http://localhost:555/app/myApp#/lastSubUrl'; - beforeEach(setup(url, [ - { url: url } - ])); - - it('calls window.location.reload and prevents default behavior', function () { - var event = new $.Event('click'); - - expect(env.location.reload.callCount).to.be(0); - expect(event.isDefaultPrevented()).to.be(false); - expect(event.isPropagationStopped()).to.be(false); - - var $link = findTestSubject(env.$el, 'appLink'); - expect($link.prop('href')).to.be(env.currentHref); - $link.trigger(event); - - expect(event.isPropagationStopped()).to.be(false); - expect(event.isDefaultPrevented()).to.be(true); - expect(env.location.reload.callCount).to.be(1); - }); - }); - - context('clicking a link with matching href except different hash', function () { + context('clicking a link with matching href but missing hash', function () { var url = 'http://localhost:555/app/myApp?query=1'; beforeEach(setup(url + '#/lastSubUrl', [ { url: url } ])); - it('calls window.location.reload and prevents default behavior', function () { + it('just prevents propogation (no reload)', function () { var event = new $.Event('click'); expect(env.location.reload.callCount).to.be(0); @@ -161,9 +138,57 @@ describe('appSwitcher directive', function () { expect($link.prop('href')).to.be(url); $link.trigger(event); + expect(env.location.reload.callCount).to.be(0); + expect(event.isDefaultPrevented()).to.be(false); + expect(event.isPropagationStopped()).to.be(true); + }); + }); + + context('clicking a link that matches entire url', function () { + var url = 'http://localhost:555/app/myApp#/lastSubUrl'; + beforeEach(setup(url, [ + { url: url } + ])); + + it('calls window.location.reload and prevents propogation', function () { + var event = new $.Event('click'); + + expect(env.location.reload.callCount).to.be(0); + expect(event.isDefaultPrevented()).to.be(false); expect(event.isPropagationStopped()).to.be(false); - expect(event.isDefaultPrevented()).to.be(true); + + var $link = findTestSubject(env.$el, 'appLink'); + expect($link.prop('href')).to.be(env.currentHref); + $link.trigger(event); + expect(env.location.reload.callCount).to.be(1); + expect(event.isDefaultPrevented()).to.be(false); + expect(event.isPropagationStopped()).to.be(true); + }); + }); + + context('clicking a link with matching href but changed hash', function () { + var rootUrl = 'http://localhost:555/app/myApp?query=1'; + var url = rootUrl + '#/lastSubUrl2'; + + beforeEach(setup(url + '#/lastSubUrl', [ + { url: url } + ])); + + it('calls window.location.reload and prevents propogation', function () { + var event = new $.Event('click'); + + expect(env.location.reload.callCount).to.be(0); + expect(event.isDefaultPrevented()).to.be(false); + expect(event.isPropagationStopped()).to.be(false); + + var $link = findTestSubject(env.$el, 'appLink'); + expect($link.prop('href')).to.be(url); + $link.trigger(event); + + expect(env.location.reload.callCount).to.be(1); + expect(event.isDefaultPrevented()).to.be(false); + expect(event.isPropagationStopped()).to.be(true); }); }); From c46cb43e7691b5f6d9f1c515cffff04ccf5bcc05 Mon Sep 17 00:00:00 2001 From: spalger Date: Wed, 23 Sep 2015 09:36:58 -0700 Subject: [PATCH 15/15] [findTestSubject] remove --- src/ui/public/jquery/findTestSubject.js | 19 ------------------- webpackShims/jquery.js | 3 +-- 2 files changed, 1 insertion(+), 21 deletions(-) delete mode 100644 src/ui/public/jquery/findTestSubject.js diff --git a/src/ui/public/jquery/findTestSubject.js b/src/ui/public/jquery/findTestSubject.js deleted file mode 100644 index 18c33349814f..000000000000 --- a/src/ui/public/jquery/findTestSubject.js +++ /dev/null @@ -1,19 +0,0 @@ -module.exports = function bindToJquery($) { - - $.fn.findTestSubject = function (/* ...subjectSelectors */) { - var subjectSelectors = [].slice.apply(arguments); - var $els = $(); - var $context = this; - - subjectSelectors.forEach(function (subjects) { - var selector = subjects.split(/\s+/).map(function (subject) { - return '[data-test-subj~="' + subject + '"]'; - }).join(' '); - - $els = $els.add($context.find(selector)); - }); - - return $els; - }; - -}; diff --git a/webpackShims/jquery.js b/webpackShims/jquery.js index e9ac7b7cbfda..4cef6e2479f3 100644 --- a/webpackShims/jquery.js +++ b/webpackShims/jquery.js @@ -1,2 +1 @@ -var $ = window.jQuery = window.$ = module.exports = require('node_modules/jquery/dist/jquery'); -require('ui/jquery/findTestSubject')($); +window.jQuery = window.$ = module.exports = require('node_modules/jquery/dist/jquery');