-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix($ionicActionSheet): stop memory leak due to hidden element stayin…
…g 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();
```- Loading branch information
Showing
4 changed files
with
184 additions
and
151 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
| })); | ||
|
|
||
| }); |