ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
51.09k stars 13.51k forks source link

v1.0.0-beta.14: All DelegateService function in the modal under rootScope will always fail $ionicHistory.isActiveScope() check #2754

Closed dreamershl closed 9 years ago

dreamershl commented 9 years ago

For example, create a modal page under rootScope with the slide box. We won't be able to call the slide box functions.

Because the slide box delegate service function will verify whether the current scope (modal scope) is active (not in the cache) by calling the $ionicHistory.isActiveScope(). And this "isActiveScope" will stopped at ionSideMenus scope, which is a sibling scope of the modal scope.

drewrygh commented 9 years ago

Could you put together a codepen so that this is easier to replicate?

dreamershl commented 9 years ago

Please refer to the below page.
http://codepen.io/dreamershl/pen/emzVKZ?editors=001

In home page there 2 buttons to create the modal box under the different scope. When you try to click the header button to slide the slider, the modal under the root scope will fail with the following error message.

  console:Delegate for handle "ruleSlides" could not find a corresponding element with delegate-handle="ruleSlides"! next() was not called!

Possible cause: If you are calling next() immediately, and your element with delegate-handle="ruleSlides" is a child of your controller, then your element may not be compiled yet. Put a $timeout around your call to next() and try again.

image

chris-carrington commented 9 years ago

This is messing me up too. Debugging also tracked back to $ionicHistory.isActiveScope which always returns false

billymedia commented 9 years ago

I have no idea if this is a fix, i swapped the return statement in isActiveScope around like so:

// return currentHistoryId ? currentHistoryId == 'root' : true;
return currentHistoryId ? true : currentHistoryId == 'root';

So far, this appears to be working in my app and has fixed the slidebox issue, but the internal workings of the framework is beyond me so a yay or nay from the dev team would be needed.

chris-carrington commented 9 years ago

The issue for me is if an ionContent is on the screen that is not apart of the activeScope you no longer allow us to grab it via the delegate-handle with $ionicScrollDelegate.

This really screws me up b/c I have custom modals that use ionContent but are not apart of the activeScope within your $ionicHistory b/c I am dynamically adding them to the DOM and giving them their own scope.

Within ionic.js on line 96 this if statement will allow you to grab the ionContent under the conditions it describes.

if ((!handle || handle == instance.$$delegateHandle) && instance.$$filterFn(instance)) {
   ...
}

The second piece of the if statement triggers this function which can be found on line 33 within scrollController.js which checks that the handle is apart of the active scope.

var deregisterInstance = $ionicScrollDelegate._registerInstance(
   self, scrollViewOptions.delegateHandle, function() {
      return $ionicHistory.isActiveScope($scope);
   }
);

The above lines of code are copied and pasted within several files in ionic. My issue is specifically with the line return $ionicHistory.isActiveScope($scope);

There are certain cases where an ionContent won't be apart of the activeScope so this triggers false which disallows me to grab it with something like.

$ionicScrollDelegate.$getByHandle('mainScroll').scrollTop();

@drewrygh Is there any good reason for the isActiveScope() check?

chris-carrington commented 9 years ago

@nextbigleap @drewrygh

Work around function I've built. Grabs the view no matter what the history stack or historyId is and lets you run view functions on it. If you need more functions you can just add it to the service. I like this way a little better @nextbigleap b/c we don't have to touch their source code.

angular.module('app.service.inContent', ['ionic'])

.service('inContent', ['$ionicScrollDelegate', '$timeout', 

function($ionicScrollDelegate, $timeout)
{
   function getView(name)
   {
      var instances = $ionicScrollDelegate.$getByHandle(name)._instances;
      var view = _.where(instances, { '$$delegateHandle': name })[0];

      return view;
   }

   return {
      top: function(name) 
      {
         $timeout(function()
         {
            var view = getView(name);
            view.scrollTo(0, 0, false);
         });
      },
      resize: function()
      {
         $timeout(function()
         {
            var view = getView(name);
            view.resize();
         });
      }
   };
}]);

Example HTML:

<ion-content delegate-handle="tags"></ion-content>

Example JS

inContent.top('tags');

chris-carrington commented 9 years ago

Actually I like this way better. Now you can just use the existing getByHandle api that has documentation! http://ionicframework.com/docs/api/service/$ionicScrollDelegate/

angular.module('app.service.inContent', ['ionic'])

.service('inContent', ['$ionicScrollDelegate', function($ionicScrollDelegate)
{
   return {
      get: function(name)
      {
         var instances = $ionicScrollDelegate.$getByHandle(name)._instances;
         return _.where(instances, { '$$delegateHandle': name })[0];
      }
   };
}]);

Example JS 1

$timeout(function()
{
   inContent.get('tags').scrollTop();
});

Example JS 2

$timeout(function()
{
   inContent.get('tags').resize();
});
rajatrocks commented 9 years ago

I'm having this same issue - trying to get a scroll delegate by handle for ion-content inside a modal in beta 14 and it returns the following:

"Delegate for handle "composer1" could not find a corresponding element with delegate-handle="composer1"! scrollTop() was not called! Possible cause: If you are calling scrollTop() immediately, and your element with delegate-handle="composer1" is a child of your controller, then your element may not be compiled yet. Put a $timeout around your call to scrollTop() and try again."

Using @chris-carrington 's solution from directly above makes everything work. Thanks Chris!

btw - I'm not using underscore, so changed the code to the following.

    .service('inContent', function($ionicScrollDelegate)
    {
       return {
          get: function(name)
          {
             var instances = $ionicScrollDelegate.$getByHandle(name)._instances;
             return instances.filter(function(element) {
                return (element['$$delegateHandle'] == name);
             })[0];
             //return _.where(instances, { '$$delegateHandle': name })[0];
          }
       };
    })
rajatrocks commented 9 years ago

Based on the code from @chris-carrington, I ended up creating a proxy scroll delegate that I can use in my code instead of $ionicScrollDelegate. When this problem is eventually fixed, I can just return "standard" instead of "custom" in the service and not have to change code throughout my app.

    // $ionicScrollDelegate not working for content in modals
    // as described here: https://github.com/driftyco/ionic/issues/2754
    // So using an intermediary service until it's fixed. 
    // Change to return standard and it should use native functionality. 
    .service('myScrollDelegate', function($ionicScrollDelegate)
    {
        var custom = {
            $getByHandle: function(name) {
                var instances = $ionicScrollDelegate.$getByHandle(name)._instances;
                return instances.filter(function(element) {
                    return (element['$$delegateHandle'] == name);
                })[0];
            }
        };

        var standard = {
            $getByHandle: function(name) {
                return $ionicScrollDelegate.$getByHandle(name);
            }
        };

        //return standard;
        return custom;
    })

Calling it looks like this:

myScrollDelegate.$getByHandle("composer1").resize();
chris-carrington commented 9 years ago

@rajatrocks Cheers mate!

adamdbradley commented 9 years ago

Ok so the main problem is that there can be many instances of the same delegate handle because of cached views. So let's say we have 10 views, each with the "myHandle" delegate handle. When you go to get "myHandle" it's going to find 10 of them, rather than just one. The active scope could makes it possible to actually find the correct modal/scroll.

In this example you show where it does and doesn't work by passing in the correct scope or root scope: http://codepen.io/ionic/pen/emyvmM?editors=101

Will the solution of passing the scope not work?

chris-carrington commented 9 years ago
meticoeus commented 9 years ago

I am still having this problem with modals created from root scope in 1.0.1.

Maybe modal scope should be treated specially to get around this problem?

I'm using the modified get function from @chris-carrington but using a provider decorator to replace the method so I don't need to change any code if/when this is fixed.

angular.module('ionic').config(['$provide', $ionicSlideBoxDelegateDecorator]);

function $ionicSlideBoxDelegateDecorator($provide) {
  $provide.decorator('$ionicSlideBoxDelegate', ['$delegate', decorateService]);

  function decorateService($delegate) {
    $delegate.$getByHandle = function $getByHandle(name) {
      var instances = this._instances;
      return instances.filter(function (element) {
        return (element['$$delegateHandle'] == name);
      })[0];
    };

    return $delegate;
  }
}
kylefox commented 9 years ago

I have spent several hours struggling to get $ionicScrollDelegate.$getByHandle working inside an $ionicModal and finally stumbled upon this thread.

The patch @rajatrocks posted on Jan 4 fixed the issue — thank you! Used it as a drop in replacement for $ionicScrollDelegate and it works perfectly. :+1:

Running ionic info shows:

Cordova CLI: 5.3.3
Gulp version:  CLI version 3.9.0
Gulp local:   Local version 3.9.0
Ionic Version: 1.1.0
Ionic CLI Version: 1.6.5
Ionic App Lib Version: 0.3.9
ios-deploy version: Not installed
ios-sim version: 3.1.1 
OS: Mac OS X Yosemite
Node Version: v4.1.1
Xcode version: Xcode 7.0.1 Build version 7A1001 
kylefox commented 9 years ago

Quick note — ionic info appears to show I'm on "Mac OS X Yosemite", but I'm definitely on El Capitan:

image

mhartington commented 9 years ago

@kylefox could you open an issue for this on https://github.com/driftyco/ionic-app-lib

kylefox commented 9 years ago

@mhartington You bet, simple fix — https://github.com/driftyco/ionic-app-lib/pull/31

Mr-Luo commented 9 years ago

Possible cause: If you are calling slide() immediately, and your element with delegate-handle="propertySlide" is a child of your controller, then your element may not be compiled yet

@meticoeus I have the same problem with the version 1.1 .Could you tell me how to fix it?thanks.

tamakisquare commented 9 years ago

@chris-carrington, @rajatrocks, @meticoeus: Great way to work around the issue. Thanks.

@adamdbradley: I see your point. But leaving the filter function (ie. $ionicHistory.isActiveScope) as it is, means we wouldn't be able to use $getByHandle with a couple essential ionic delegate services (namely $ionicListDelegate, $ionicScrollDelegate, $ionicSideMenuDelegate, and $ionicSlideBoxDelegate) in modals whose scopes are children of $rootScope, which is actually the default behaviour of ionicModal. It doesn't make much sense to me that a default behaviour would lead to such pitfall. I believe this issue worths a second look.

So let's say we have 10 views, each with the "myHandle" delegate handle.

Admittedly, I was a little surprised when I saw this line. The reason was that I always had the impression that each delegate handle should be unique across all the view of the app. I guess that impression was influenced by the use of the word "handle" in other contexts, such as file handle. Anyways, my question to you is what is the use case of having one delegate handle being used across multiple views? As someone who always keep his delegate handles unique across the app, I am curious to learn from you. Cheers.

meticoeus commented 9 years ago

someone correct me if I'm misunderstanding the way $ionicHistory works but @tamakisquare I believe the typical use case that you run into is when you have a view defined as follows and multiple versions cached with different id parameters:

// in your ui-router config file
$stateProvider.state('app', {
  url: "/channels/:id",
  views: {
    'app-view': {
      templateUrl: "feeds.html",
      controller: "FeedController"
    }
  },
  resolve: {
    feedList: ['feedService', '$stateParams', function (feedService, $stateParams) {
      return feedService.getFeedList($stateParams.id);
    }]
  }
});

//feeds.html
<ion-scroll delegate-handle="feedScrollHandle">
  <ion-list>
    <ion-item ng-repeat="feedEntry in feedList">
      {{feedEntry.name}}
    </ion-item>
  </ion-list>
</ion-list>

//FeedController.js
angular.module('app')
  .controller('FeedController', ['$scope', '$ionScrollDelegate', 'feedList', function FeedController($scope, ionScrollDelegate, feedList) {
    $scope.feedList = feedList;

    // do something with $ionScrollDelegate.getHandle('feedScrollHandle')
  }])
tamakisquare commented 9 years ago

@meticoeus - Thanks for illustrating the use case of same delegate handle being used by multiple views. With that bear in mind, I think the direction you mentioned earlier, with a question, would be the easiest way to patch up the issue without inflicting issues elsewhere in the code.

Maybe modal scope should be treated specially to get around this problem?

I went ahead and made the patch, only for $ionicSlideBox as that's the only affected Ionic module that I am using for the time being, to my own fork of Ionic. The patch is based on the fact that for $ionicModal instances that are initialized without specifying a scope, the instances themselves are assigned to their isolated scopes (see the code below).

// If this wasn't a defined scope, we can assign the viewType to the isolated scope
// we created
if (!options.scope) {
  scope[ options.viewType ] = modal;
}

For those who are interested, here is my change to $ionicSlideBox (Ionic v1.1.1)

Changed from

var deregisterInstance = $ionicSlideBoxDelegate._registerInstance(
  slider, $attrs.delegateHandle, function() {
    return $ionicHistory.isActiveScope($scope);
  }
);

To

var deregisterInstance = $ionicSlideBoxDelegate._registerInstance(
  slider, $attrs.delegateHandle, function() {
    return $ionicHistory.isActiveScope($scope) || isInRootScopeModal($scope);

    function isInRootScopeModal(scope) {
      var s = scope;

      // If `$parent` is `null`, then the root scope has been reached, at which point the loop should stop.
      while (s.$parent !== null) {
        if ('modal' in s) { return true; }
        s = s.$parent;
      }

      return false;
    }
  }
);

If there's enough interest, I will create a PR and apply this solution on all the other affected delegate services, such as $ionicScroll.

integry commented 8 years ago

I couldn't make the dynamically generated ion-scrolls to be referenced by handle, so I borrowed a code snippet from above and mixed in the worst ideas from PHP with stunning results.

angular.module('ionic').config(['$provide', $ionicScrollDelegateDecorator]);

function $ionicScrollDelegateDecorator($provide) {
  $provide.decorator('$ionicScrollDelegate', ['$delegate', decorateService]);

  function decorateService($delegate) {
    $delegate.$getByRealHandle = function $getByRealHandle(name) {
      var instances = this._instances;
      return instances.filter(function (instance) {
        return (angular.element(instance.element).attr('delegate-handle') == name);
      }).shift();
    };

    return $delegate;
  }
};

So now if calling $getByHandle() doesn't work, just use $getByRealHandle() :D

$ionicScrollDelegate.$getByRealHandle(myHandle);
jrencz commented 8 years ago

@integry mind that (if I'm not mistaken) handle is not required to be globally unique. It only has to be unique on a single ionView. The implementation you suggested may return incorrect instance in case the same handle has been used in both modal and view. I suppose the following implementation is safer because it will only work different way than $getByHandle if handle is globally unique. Otherwise it will show the error anyway.

$provide.decorator('$ionicScrollDelegate', function ($delegate) {
  return Object.assign($delegate, {
    $getByAbsoluteHandle(delegateHandle) {
      const instancesByHandle = this._instances
        .filter(instance => instance.$$delegateHandle === delegateHandle);

      return instancesByHandle.length === 1 ?
        instancesByHandle[0] :
        this.$getByHandle(delegateHandle);
    },
  });
});

Too bad there's no "official" way of decorating the ionic.DelegateService so all delegates have this method. This decorator has to be applied one-by-one to delegates used on modals.

meticoeus commented 8 years ago

As an alternative to filtering, I've modified the ionic-angular.js source to support a dynamic handle by letting the scope bind it. So far its working fine for dynamic-but-set-once-immediately cases. This approach does not support changing the handle later.

My use case is generating a small list of sliders so I'm using the ids/indexes in the list to create handles to each item.

I changed the scope definition to include

scope: {
...,
delegateHandle: '@'
}

and modified the register method from

var deregisterInstance = $ionicSlideBoxDelegate._registerInstance(
  slider, $attrs.delegateHandle, function() {
    return $ionicHistory.isActiveScope($scope);
  }
);

to

var deregisterInstance = $ionicSlideBoxDelegate._registerInstance(
  slider, $scope.delegateHandle, function() {
    return $ionicHistory.isActiveScope($scope);
  }
);