From b7646a56309273f37fc73903114de5e363bbf060 Mon Sep 17 00:00:00 2001 From: Andrew Joslin Date: Fri, 6 Jun 2014 13:10:51 -0400 Subject: [PATCH] fix($ionicActionSheet): stop memory leak due to hidden element staying in dom BREAKING CHANGE: $ionicActionSheet now returns a method to hide the action sheet. Previously, it returned an object that had a `show` and `hide` method. This was undocumented, but if you used it, here is how to migrate your code: Change your code from this: ```js var sheet = $ionicActionSheet.show({...}); sheet.hide(); ``` To this: ```js var hideSheet = $ionicActionSheet.show({...}); hideSheet(); ``` --- config/build.config.js | 1 - js/angular/service/actionSheet.js | 209 ++++++++++-------- js/views/actionSheetView.js | 26 --- test/unit/angular/service/actionSheet.unit.js | 99 ++++++--- 4 files changed, 184 insertions(+), 151 deletions(-) delete mode 100644 js/views/actionSheetView.js diff --git a/config/build.config.js b/config/build.config.js index b8affc02bd2..6d2c102d73a 100644 --- a/config/build.config.js +++ b/config/build.config.js @@ -51,7 +51,6 @@ module.exports = { // Views 'js/views/view.js', 'js/views/scrollView.js', - 'js/views/actionSheetView.js', 'js/views/headerBarView.js', 'js/views/listView.js', 'js/views/modalView.js', diff --git a/js/angular/service/actionSheet.js b/js/angular/service/actionSheet.js index f0338d7db65..6cd53c056f8 100644 --- a/js/angular/service/actionSheet.js +++ b/js/angular/service/actionSheet.js @@ -16,13 +16,13 @@ * * ```js * angular.module('mySuperApp', ['ionic']) - * .controller(function($scope, $ionicActionSheet) { + * .controller(function($scope, $ionicActionSheet, $timeout) { * * // Triggered on a button click, or some other target * $scope.show = function() { * * // Show the action sheet - * $ionicActionSheet.show({ + * var hideSheet = $ionicActionSheet({ * buttons: [ * { text: 'Share This' }, * { text: 'Move' }, @@ -35,6 +35,11 @@ * } * }); * + * // For example's sake, hide the sheet after two seconds + * $timeout(function() { + * hideSheet(); + * }, 2000); + * * }; * }); * ``` @@ -52,101 +57,119 @@ IonicModule function($rootScope, $document, $compile, $animate, $timeout, $ionicTemplateLoader, $ionicPlatform) { return { - /** - * @ngdoc method - * @name $ionicActionSheet#show - * @description - * Load and return a new action sheet. - * - * A new isolated scope will be created for the - * action sheet and the new element will be appended into the body. - * - * @param {object} opts The options for this ActionSheet. Properties: - * - * - `[Object]` `buttons` Which buttons to show. Each button is an object with a `text` field. - * - `{string}` `titleText` The title to show on the action sheet. - * - `{string=}` `cancelText` The text for a 'cancel' button on the action sheet. - * - `{string=}` `destructiveText` The text for a 'danger' on the action sheet. - * - `{function=}` `cancel` Called if the cancel button is pressed, the backdrop is tapped or - * the hardware back button is pressed. - * - `{function=}` `buttonClicked` Called when one of the non-destructive buttons is clicked, - * with the index of the button that was clicked and the button object. Return true to close - * the action sheet, or false to keep it opened. - * - `{function=}` `destructiveButtonClicked` Called when the destructive button is clicked. - * Return true to close the action sheet, or false to keep it opened. - */ - show: function(opts) { - var scope = $rootScope.$new(true); - - angular.extend(scope, opts); - - // Compile the template - var element = $compile('')(scope); - - // Grab the sheet element for animation - var sheetEl = jqLite(element[0].querySelector('.action-sheet-wrapper')); - - // removes the actionSheet from the screen - var hideSheet = function(h) { - sheetEl.removeClass('action-sheet-up'); - - $animate.removeClass(element, 'active', function() { - scope.$destroy(); - }); - - $document[0].body.classList.remove('action-sheet-open'); - - scope.$deregisterBackButton && scope.$deregisterBackButton(); - }; - - // registerBackButtonAction returns a callback to deregister the action - scope.$deregisterBackButton = $ionicPlatform.registerBackButtonAction( - function(){ - scope.cancel(); // - }, - PLATFORM_BACK_BUTTON_PRIORITY_ACTION_SHEET - ); - - // called when the user presses the cancel button - scope.cancel = function() { - hideSheet(); - $timeout(function(){ - // after the animation is out, call the cancel callback - opts.cancel && opts.cancel(); - },200) - }; - - scope.buttonClicked = function(index) { - // Check if the button click event returned true, which means - // we can close the action sheet - if((opts.buttonClicked && opts.buttonClicked(index, opts.buttons[index])) === true) { - hideSheet(); - } - }; - - scope.destructiveButtonClicked = function() { - // Check if the destructive button click event returned true, which means - // we can close the action sheet - if((opts.destructiveButtonClicked && opts.destructiveButtonClicked()) === true) { - hideSheet(); - } - }; + show: actionSheet + }; - $document[0].body.appendChild(element[0]); + /** + * @ngdoc method + * @name $ionicActionSheet#show + * @description + * Load and return a new action sheet. + * + * A new isolated scope will be created for the + * action sheet and the new element will be appended into the body. + * + * @param {object} options The options for this ActionSheet. Properties: + * + * - `[Object]` `buttons` Which buttons to show. Each button is an object with a `text` field. + * - `{string}` `titleText` The title to show on the action sheet. + * - `{string=}` `cancelText` The text for a 'cancel' button on the action sheet. + * - `{string=}` `destructiveText` The text for a 'danger' on the action sheet. + * - `{function=}` `cancel` Called if the cancel button is pressed, the backdrop is tapped or + * the hardware back button is pressed. + * - `{function=}` `buttonClicked` Called when one of the non-destructive buttons is clicked, + * with the index of the button that was clicked and the button object. Return true to close + * the action sheet, or false to keep it opened. + * - `{function=}` `destructiveButtonClicked` Called when the destructive button is clicked. + * Return true to close the action sheet, or false to keep it opened. + * + * @returns {function} `hideSheet` A function which, when called, hides & cancels the action sheet. + */ + function actionSheet(opts) { + var scope = $rootScope.$new(true); + + angular.extend(scope, { + cancel: angular.noop, + destructiveButtonClicked: angular.noop, + buttonClicked: angular.noop, + $deregisterBackButton: angular.noop, + buttons: [], + }, opts || {}); + + // Compile the template + var element = scope.element = $compile('')(scope); + + // Grab the sheet element for animation + var sheetEl = jqLite(element[0].querySelector('.action-sheet-wrapper')); + + // removes the actionSheet from the screen + scope.removeSheet = function(done) { + if (scope.removed) return; + + scope.removed = true; + sheetEl.removeClass('action-sheet-up'); + $document[0].body.classList.remove('action-sheet-open'); + scope.$deregisterBackButton(); + + $animate.removeClass(element, 'active', function() { + scope.$destroy(); + element.remove(); + // scope.cancel.$scope is defined near the bottom + scope.cancel.$scope = null; + (done || angular.noop)(); + }); + }; + + scope.showSheet = function(done) { + if (scope.removed) return; + $document[0].body.appendChild(element[0]); $document[0].body.classList.add('action-sheet-open'); - var sheet = new ionic.views.ActionSheet({el: element[0] }); - scope.sheet = sheet; - - $animate.addClass(element, 'active'); - + $animate.addClass(element, 'active', function() { + if (scope.removed) return; + (done || angular.noop)(); + }); $timeout(function(){ + if (scope.removed) return; sheetEl.addClass('action-sheet-up'); - }, 20); - - return sheet; - } - }; - + }, 20, false); + }; + + // registerBackButtonAction returns a callback to deregister the action + scope.$deregisterBackButton = $ionicPlatform.registerBackButtonAction( + scope.cancel, + PLATFORM_BACK_BUTTON_PRIORITY_ACTION_SHEET + ); + + // called when the user presses the cancel button + scope.cancel = function() { + // after the animation is out, call the cancel callback + scope.removeSheet(opts.cancel); + }; + + scope.buttonClicked = function(index) { + // Check if the button click event returned true, which means + // we can close the action sheet + if (opts.buttonClicked(index, opts.buttons[index]) === true) { + scope.removeSheet(); + } + }; + + scope.destructiveButtonClicked = function() { + // Check if the destructive button click event returned true, which means + // we can close the action sheet + if (opts.destructiveButtonClicked() === true) { + scope.removeSheet(); + } + }; + + scope.showSheet(); + + // Expose the scope on $ionicActionSheet's return value for the sake + // of testing it. + scope.cancel.$scope = scope; + + return scope.cancel; + } }]); diff --git a/js/views/actionSheetView.js b/js/views/actionSheetView.js deleted file mode 100644 index f8829560e87..00000000000 --- a/js/views/actionSheetView.js +++ /dev/null @@ -1,26 +0,0 @@ -(function(ionic) { -'use strict'; - /** - * An ActionSheet is the slide up menu popularized on iOS. - * - * You see it all over iOS apps, where it offers a set of options - * triggered after an action. - */ - ionic.views.ActionSheet = ionic.views.View.inherit({ - initialize: function(opts) { - this.el = opts.el; - }, - show: function() { - // Force a reflow so the animation will actually run - this.el.offsetWidth; - - this.el.classList.add('active'); - }, - hide: function() { - // Force a reflow so the animation will actually run - this.el.offsetWidth; - this.el.classList.remove('active'); - } - }); - -})(ionic); diff --git a/test/unit/angular/service/actionSheet.unit.js b/test/unit/angular/service/actionSheet.unit.js index e315c258bcd..f8b4b7e77f2 100644 --- a/test/unit/angular/service/actionSheet.unit.js +++ b/test/unit/angular/service/actionSheet.unit.js @@ -1,44 +1,81 @@ describe('Ionic ActionSheet Service', function() { - var sheet, timeout, ionicPlatform; - beforeEach(module('ionic')); + var sheet, timeout, ionicPlatform; - beforeEach(inject(function($ionicActionSheet, $timeout, $ionicPlatform) { - sheet = $ionicActionSheet; - timeout = $timeout; - ionicPlatform = $ionicPlatform; + beforeEach(module('ionic', function($provide) { + // For the sake of this test, we don't want ionActionSheet to + // actually compile as a directive. + // We are only testing the service. + $provide.value('ionActionSheetDirective', []); })); - it('Should show', function() { - var s = sheet.show(); - expect(s.el.classList.contains('active')).toBe(true); - }); - - it('Should add .action-sheet-up to .action-sheet-wrapper', function() { - var s = sheet.show(); - var el = angular.element(s.el); - var wrapper = angular.element(s.el.querySelector('.action-sheet-wrapper')); - expect(wrapper.length).toEqual(1); - expect(wrapper.hasClass('action-sheet-up')).toEqual(false); - timeout.flush(); - expect(wrapper.hasClass('action-sheet-up')).toEqual(true); - }); + function setup(options) { + var scope; + inject(function($ionicActionSheet, $ionicPlatform, $timeout) { + var hide = $ionicActionSheet.show(options || {}); + $timeout.flush(); + scope = hide.$scope; + }); + return scope; + } - it('should handle hardware back button', function() { - var s = sheet.show(); + it('should add classes on showing', inject(function($document) { + var scope = setup(); + expect($document[0].body.classList.contains('action-sheet-open')).toBe(true); + expect(scope.element.hasClass('active')).toBe(true); + })); - ionicPlatform.hardwareBackButtonClick(); + it('removeSheet should remove classes, remove element and destroy scope', inject(function($document, $timeout) { + var scope = setup(); + spyOn(scope, '$destroy'); + spyOn(scope.element, 'remove'); + scope.removeSheet(); + expect($document[0].body.classList.contains('action-sheet-open')).toBe(false); + expect(scope.element.hasClass('active')).toBe(false); + $timeout.flush(); + expect(scope.$destroy).toHaveBeenCalled(); + expect(scope.element.remove).toHaveBeenCalled(); + })); - expect(s.el.classList.contains('active')).toBe(false); + it('destructiveButtonClicked should removeSheet if returning true', function() { + var destructiveReturnValue = false; + var scope = setup({ + destructiveButtonClicked: function() { + return destructiveReturnValue; + } + }); + spyOn(scope, 'removeSheet'); + scope.destructiveButtonClicked(); + expect(scope.removeSheet).not.toHaveBeenCalled(); + destructiveReturnValue = true; + scope.destructiveButtonClicked(); + expect(scope.removeSheet).toHaveBeenCalled(); }); - it('show & hide should add action-sheet-open to body', inject(function($animate) { - var s = sheet.show(); - - expect(angular.element(document.body).hasClass('action-sheet-open')).toBe(true); - - ionicPlatform.hardwareBackButtonClick(); + it('buttonClicked should removeSheet if returning true for index', function() { + var scope = setup({ + buttons: [{}, {}], + buttonClicked: function(index) { + return index === 0 ? false : true; + } + }); + spyOn(scope, 'removeSheet'); + scope.buttonClicked(0); + expect(scope.removeSheet).not.toHaveBeenCalled(); + scope.buttonClicked(1); + expect(scope.removeSheet).toHaveBeenCalled(); + }); - expect(angular.element(document.body).hasClass('action-sheet-open')).toBe(false); + it('cancel should removeSheet and call opts.cancel', inject(function($timeout) { + var cancelSpy = jasmine.createSpy('opts.cancel'); + var scope = setup({ + cancel: cancelSpy + }); + spyOn(scope, 'removeSheet').andCallThrough(); + scope.cancel(); + expect(scope.removeSheet).toHaveBeenCalled(); + $timeout.flush(); + expect(cancelSpy).toHaveBeenCalled(); })); + });